diff mbox series

[4/4] drm: rcar-du: Add support for R8A779H0

Message ID 20240619102219.138927-5-jacopo.mondi@ideasonboard.com (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series drm: rcar-du: Add support for R8A779H0 | expand

Commit Message

Jacopo Mondi June 19, 2024, 10:22 a.m. UTC
Add support for R-Car R8A779H0 V4M which has similar characteristics
as the already supported R-Car V4H R8A779G0, but with a single output
channel.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

---
BSP patch
https://github.com/renesas-rcar/linux-bsp/commit/f2fc3314dab2052240653c1a31ba3d7c7190038e
---
---
 .../bindings/display/renesas,du.yaml           |  1 +
 drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c  | 18 ++++++++++++++++++
 .../gpu/drm/renesas/rcar-du/rcar_du_group.c    | 17 ++++++++++++-----
 3 files changed, 31 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart June 19, 2024, 7:44 p.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Wed, Jun 19, 2024 at 12:22:18PM +0200, Jacopo Mondi wrote:
> Add support for R-Car R8A779H0 V4M which has similar characteristics
> as the already supported R-Car V4H R8A779G0, but with a single output
> channel.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> ---
> BSP patch
> https://github.com/renesas-rcar/linux-bsp/commit/f2fc3314dab2052240653c1a31ba3d7c7190038e
> ---
> ---
>  .../bindings/display/renesas,du.yaml           |  1 +
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c  | 18 ++++++++++++++++++
>  .../gpu/drm/renesas/rcar-du/rcar_du_group.c    | 17 ++++++++++++-----
>  3 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.yaml b/Documentation/devicetree/bindings/display/renesas,du.yaml
> index c5b9e6812bce..d369953f16f7 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.yaml
> +++ b/Documentation/devicetree/bindings/display/renesas,du.yaml
> @@ -41,6 +41,7 @@ properties:
>        - renesas,du-r8a77995 # for R-Car D3 compatible DU
>        - renesas,du-r8a779a0 # for R-Car V3U compatible DU
>        - renesas,du-r8a779g0 # for R-Car V4H compatible DU
> +      - renesas,du-r8a779h0 # for R-Car V4M compatible DU
>  
>    reg:
>      maxItems: 1

This should be split to a separate patch.

You need to add a conditional validation rule below to address the
clocks, interrupts, ports, ...

> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
> index dee530e4c8b2..a1d174b0b00b 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
> @@ -545,6 +545,23 @@ static const struct rcar_du_device_info rcar_du_r8a779g0_info = {
>  	.dsi_clk_mask =  BIT(1) | BIT(0),
>  };
>  
> +static const struct rcar_du_device_info rcar_du_r8a779h0_info = {
> +	.gen = 4,
> +	.features = RCAR_DU_FEATURE_CRTC_IRQ
> +		  | RCAR_DU_FEATURE_VSP1_SOURCE
> +		  | RCAR_DU_FEATURE_NO_BLENDING,
> +	.channels_mask = BIT(0),
> +	.routes = {
> +		/* R8A779H0 has a single MIPI DSI output. */
> +		[RCAR_DU_OUTPUT_DSI0] = {
> +			.possible_crtcs = BIT(0),
> +			.port = 0,
> +		},
> +	},
> +	.num_rpf = 5,
> +	.dsi_clk_mask = BIT(0),
> +};

This looks good.

> +
>  static const struct of_device_id rcar_du_of_table[] = {
>  	{ .compatible = "renesas,du-r8a7742", .data = &rcar_du_r8a7790_info },
>  	{ .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
> @@ -571,6 +588,7 @@ static const struct of_device_id rcar_du_of_table[] = {
>  	{ .compatible = "renesas,du-r8a77995", .data = &rcar_du_r8a7799x_info },
>  	{ .compatible = "renesas,du-r8a779a0", .data = &rcar_du_r8a779a0_info },
>  	{ .compatible = "renesas,du-r8a779g0", .data = &rcar_du_r8a779g0_info },
> +	{ .compatible = "renesas,du-r8a779h0", .data = &rcar_du_r8a779h0_info },
>  	{ }
>  };
>  
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> index 2ccd2581f544..361e1d01b817 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> @@ -111,6 +111,8 @@ static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp)
>  		/*
>  		 * On Gen3 dot clocks are setup through per-group registers,
>  		 * only available when the group has two channels.
> +		 *
> +		 * R-Car V4M (R8A779H0) has only one channel, index is == 0.

Is it relevant here ?

>  		 */
>  		rcrtc = &rcdu->crtcs[rgrp->index * 2];
>  		num_crtcs = rgrp->num_crtcs;
> @@ -185,11 +187,16 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  		dorcr |= DORCR_PG1T | DORCR_DK1S | DORCR_PG1D_DS1;
>  	rcar_du_group_write(rgrp, DORCR, dorcr);
>  
> -	/* Apply planes to CRTCs association. */
> -	mutex_lock(&rgrp->lock);
> -	rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> -			    rgrp->dptsr_planes);
> -	mutex_unlock(&rgrp->lock);
> +	/*
> +	 * Apply planes to CRTCs association, skip for V4M which has a single
> +	 * channel.

" and doesn't implement the DPTSR register."

I'm pretty sure writing it is still harmless, but...

> +	 */
> +	if (rcdu->info->gen < 4 || rgrp->num_crtcs > 1) {
> +		mutex_lock(&rgrp->lock);
> +		rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> +				    rgrp->dptsr_planes);
> +		mutex_unlock(&rgrp->lock);
> +	}
>  }
>  
>  /*
Geert Uytterhoeven June 20, 2024, 12:48 p.m. UTC | #2
Hi Laurent, Jacopo,

On Wed, Jun 19, 2024 at 9:46 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Jun 19, 2024 at 12:22:18PM +0200, Jacopo Mondi wrote:
> > Add support for R-Car R8A779H0 V4M which has similar characteristics
> > as the already supported R-Car V4H R8A779G0, but with a single output
> > channel.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > @@ -185,11 +187,16 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
> >               dorcr |= DORCR_PG1T | DORCR_DK1S | DORCR_PG1D_DS1;
> >       rcar_du_group_write(rgrp, DORCR, dorcr);
> >
> > -     /* Apply planes to CRTCs association. */
> > -     mutex_lock(&rgrp->lock);
> > -     rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> > -                         rgrp->dptsr_planes);
> > -     mutex_unlock(&rgrp->lock);
> > +     /*
> > +      * Apply planes to CRTCs association, skip for V4M which has a single
> > +      * channel.
>
> " and doesn't implement the DPTSR register."
>
> I'm pretty sure writing it is still harmless, but...
>
> > +      */
> > +     if (rcdu->info->gen < 4 || rgrp->num_crtcs > 1) {

Looking at the R-Car Gen3 docs, this check seems to be wrong, and the
lack of a check might have been an issue before?

Seems like the register (per pair) is only present if the second CRTC
of a CRTC pair is present, so R-Car V3M and V3H (single CRTC) do not
have DPTSR at all, and M3-W (triple CRTC) does not have it on the
second pair.  M3-N does have both, as it lacks the first CRTC of
second pair, but does have the second CRTC of the second pair.

> > +             mutex_lock(&rgrp->lock);
> > +             rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> > +                                 rgrp->dptsr_planes);
> > +             mutex_unlock(&rgrp->lock);
> > +     }
> >  }

Gr{oetje,eeting}s,

                        Geert
Jacopo Mondi June 20, 2024, 4:48 p.m. UTC | #3
Hi Geert

On Thu, Jun 20, 2024 at 02:48:49PM GMT, Geert Uytterhoeven wrote:
> Hi Laurent, Jacopo,
>
> On Wed, Jun 19, 2024 at 9:46 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Wed, Jun 19, 2024 at 12:22:18PM +0200, Jacopo Mondi wrote:
> > > Add support for R-Car R8A779H0 V4M which has similar characteristics
> > > as the already supported R-Car V4H R8A779G0, but with a single output
> > > channel.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> > > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > > @@ -185,11 +187,16 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
> > >               dorcr |= DORCR_PG1T | DORCR_DK1S | DORCR_PG1D_DS1;
> > >       rcar_du_group_write(rgrp, DORCR, dorcr);
> > >
> > > -     /* Apply planes to CRTCs association. */
> > > -     mutex_lock(&rgrp->lock);
> > > -     rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> > > -                         rgrp->dptsr_planes);
> > > -     mutex_unlock(&rgrp->lock);
> > > +     /*
> > > +      * Apply planes to CRTCs association, skip for V4M which has a single
> > > +      * channel.
> >
> > " and doesn't implement the DPTSR register."
> >
> > I'm pretty sure writing it is still harmless, but...
> >
> > > +      */
> > > +     if (rcdu->info->gen < 4 || rgrp->num_crtcs > 1) {
>
> Looking at the R-Car Gen3 docs, this check seems to be wrong, and the
> lack of a check might have been an issue before?


Not sure I got from your comment what part is wrong.

Reading below it seems you're suggesting that writes to DPTSR should
be skipped for some Gen3 boards as well ?

>
> Seems like the register (per pair) is only present if the second CRTC
> of a CRTC pair is present, so R-Car V3M and V3H (single CRTC) do not
> have DPTSR at all, and M3-W (triple CRTC) does not have it on the
> second pair.  M3-N does have both, as it lacks the first CRTC of
> second pair, but does have the second CRTC of the second pair.
>

/o\

So far however, all Gen3 SoCs you mentioned seem to work with DPTSR
being written and the BSP [1] only actually skips it for V4M.

What would you suggesting in this case ? Addressing gen3 as well ?
That's something that would require testing on all the above boards
thought.

Thanks
  j


[1] https://github.com/renesas-rcar/linux-bsp/commit/f2fc3314dab2052240653c1a31ba3d7c7190038e#diff-8bce6f4032dc891042e2561163754f49723ac119ae63df2425cc3487b432ee1cR206
> > > +             mutex_lock(&rgrp->lock);
> > > +             rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> > > +                                 rgrp->dptsr_planes);
> > > +             mutex_unlock(&rgrp->lock);
> > > +     }
> > >  }
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Geert Uytterhoeven June 20, 2024, 4:57 p.m. UTC | #4
Hi Jacopo,

On Thu, Jun 20, 2024 at 6:48 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
> On Thu, Jun 20, 2024 at 02:48:49PM GMT, Geert Uytterhoeven wrote:
> > On Wed, Jun 19, 2024 at 9:46 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > > On Wed, Jun 19, 2024 at 12:22:18PM +0200, Jacopo Mondi wrote:
> > > > Add support for R-Car R8A779H0 V4M which has similar characteristics
> > > > as the already supported R-Car V4H R8A779G0, but with a single output
> > > > channel.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > > > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > > > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > > > @@ -185,11 +187,16 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
> > > >               dorcr |= DORCR_PG1T | DORCR_DK1S | DORCR_PG1D_DS1;
> > > >       rcar_du_group_write(rgrp, DORCR, dorcr);
> > > >
> > > > -     /* Apply planes to CRTCs association. */
> > > > -     mutex_lock(&rgrp->lock);
> > > > -     rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> > > > -                         rgrp->dptsr_planes);
> > > > -     mutex_unlock(&rgrp->lock);
> > > > +     /*
> > > > +      * Apply planes to CRTCs association, skip for V4M which has a single
> > > > +      * channel.
> > >
> > > " and doesn't implement the DPTSR register."
> > >
> > > I'm pretty sure writing it is still harmless, but...
> > >
> > > > +      */
> > > > +     if (rcdu->info->gen < 4 || rgrp->num_crtcs > 1) {
> >
> > Looking at the R-Car Gen3 docs, this check seems to be wrong, and the
> > lack of a check might have been an issue before?
>
> Not sure I got from your comment what part is wrong.
>
> Reading below it seems you're suggesting that writes to DPTSR should
> be skipped for some Gen3 boards as well ?

Indeed.

> > Seems like the register (per pair) is only present if the second CRTC
> > of a CRTC pair is present, so R-Car V3M and V3H (single CRTC) do not
> > have DPTSR at all, and M3-W (triple CRTC) does not have it on the
> > second pair.  M3-N does have both, as it lacks the first CRTC of
> > second pair, but does have the second CRTC of the second pair.
> >
>
> /o\
>
> So far however, all Gen3 SoCs you mentioned seem to work with DPTSR
> being written and the BSP [1] only actually skips it for V4M.

I don't doubt it works, I was just reading the documentation.
Many nonexistent registers can be written zero to without ill effects...

> What would you suggesting in this case ? Addressing gen3 as well ?
> That's something that would require testing on all the above boards
> thought.

Ah, what if we could do without all this pesky testing? ;-)

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/renesas,du.yaml b/Documentation/devicetree/bindings/display/renesas,du.yaml
index c5b9e6812bce..d369953f16f7 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.yaml
+++ b/Documentation/devicetree/bindings/display/renesas,du.yaml
@@ -41,6 +41,7 @@  properties:
       - renesas,du-r8a77995 # for R-Car D3 compatible DU
       - renesas,du-r8a779a0 # for R-Car V3U compatible DU
       - renesas,du-r8a779g0 # for R-Car V4H compatible DU
+      - renesas,du-r8a779h0 # for R-Car V4M compatible DU
 
   reg:
     maxItems: 1
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
index dee530e4c8b2..a1d174b0b00b 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
@@ -545,6 +545,23 @@  static const struct rcar_du_device_info rcar_du_r8a779g0_info = {
 	.dsi_clk_mask =  BIT(1) | BIT(0),
 };
 
+static const struct rcar_du_device_info rcar_du_r8a779h0_info = {
+	.gen = 4,
+	.features = RCAR_DU_FEATURE_CRTC_IRQ
+		  | RCAR_DU_FEATURE_VSP1_SOURCE
+		  | RCAR_DU_FEATURE_NO_BLENDING,
+	.channels_mask = BIT(0),
+	.routes = {
+		/* R8A779H0 has a single MIPI DSI output. */
+		[RCAR_DU_OUTPUT_DSI0] = {
+			.possible_crtcs = BIT(0),
+			.port = 0,
+		},
+	},
+	.num_rpf = 5,
+	.dsi_clk_mask = BIT(0),
+};
+
 static const struct of_device_id rcar_du_of_table[] = {
 	{ .compatible = "renesas,du-r8a7742", .data = &rcar_du_r8a7790_info },
 	{ .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
@@ -571,6 +588,7 @@  static const struct of_device_id rcar_du_of_table[] = {
 	{ .compatible = "renesas,du-r8a77995", .data = &rcar_du_r8a7799x_info },
 	{ .compatible = "renesas,du-r8a779a0", .data = &rcar_du_r8a779a0_info },
 	{ .compatible = "renesas,du-r8a779g0", .data = &rcar_du_r8a779g0_info },
+	{ .compatible = "renesas,du-r8a779h0", .data = &rcar_du_r8a779h0_info },
 	{ }
 };
 
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
index 2ccd2581f544..361e1d01b817 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
@@ -111,6 +111,8 @@  static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp)
 		/*
 		 * On Gen3 dot clocks are setup through per-group registers,
 		 * only available when the group has two channels.
+		 *
+		 * R-Car V4M (R8A779H0) has only one channel, index is == 0.
 		 */
 		rcrtc = &rcdu->crtcs[rgrp->index * 2];
 		num_crtcs = rgrp->num_crtcs;
@@ -185,11 +187,16 @@  static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 		dorcr |= DORCR_PG1T | DORCR_DK1S | DORCR_PG1D_DS1;
 	rcar_du_group_write(rgrp, DORCR, dorcr);
 
-	/* Apply planes to CRTCs association. */
-	mutex_lock(&rgrp->lock);
-	rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
-			    rgrp->dptsr_planes);
-	mutex_unlock(&rgrp->lock);
+	/*
+	 * Apply planes to CRTCs association, skip for V4M which has a single
+	 * channel.
+	 */
+	if (rcdu->info->gen < 4 || rgrp->num_crtcs > 1) {
+		mutex_lock(&rgrp->lock);
+		rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
+				    rgrp->dptsr_planes);
+		mutex_unlock(&rgrp->lock);
+	}
 }
 
 /*