[RESEND] drm/tilcdc: fix raster control register setting
diff mbox series

Message ID 20210216202225.12861-1-dariobin@libero.it
State Accepted
Commit def26cd4bae27efdfcf95559ddf64db91d65670f
Headers show
Series
  • [RESEND] drm/tilcdc: fix raster control register setting
Related show

Commit Message

Dario Binacchi Feb. 16, 2021, 8:22 p.m. UTC
The fdd property of the tilcdc_panel_info structure must set the reqdly
bit field  (bit 12 to 19) of the raster control register. The previous
statement set the least significant bit instead.

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tomi Valkeinen Feb. 17, 2021, 6:41 a.m. UTC | #1
On 16/02/2021 22:22, Dario Binacchi wrote:
> The fdd property of the tilcdc_panel_info structure must set the reqdly
> bit field  (bit 12 to 19) of the raster control register. The previous
> statement set the least significant bit instead.
> 
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> 
> ---
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 30213708fc99..238068e28729 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -393,7 +393,7 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>  			return;
>  		}
>  	}
> -	reg |= info->fdd < 12;
> +	reg |= info->fdd << 12;
>  	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
>  
>  	if (info->invert_pxl_clk)
> 

This is interesting, looks like this has always been broken, and in many
cases sets bits 0, which is the enable bit. So we enable LCDC before
even setting the fb address. How does this not blow up LCDC totally?

The fix looks correct to me, but it will change the register value for
boards that have apparently been working for years.

Dario, did you test this on actual HW, or did you just spot the error?

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

 Tomi
Dario Binacchi Feb. 17, 2021, 5:45 p.m. UTC | #2
Hi Tomi,

> Il 17/02/2021 07:41 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> ha scritto:
> 
>  
> On 16/02/2021 22:22, Dario Binacchi wrote:
> > The fdd property of the tilcdc_panel_info structure must set the reqdly
> > bit field  (bit 12 to 19) of the raster control register. The previous
> > statement set the least significant bit instead.
> > 
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > 
> > ---
> > 
> >  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > index 30213708fc99..238068e28729 100644
> > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > @@ -393,7 +393,7 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
> >  			return;
> >  		}
> >  	}
> > -	reg |= info->fdd < 12;
> > +	reg |= info->fdd << 12;
> >  	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
> >  
> >  	if (info->invert_pxl_clk)
> > 
> 
> This is interesting, looks like this has always been broken, and in many
> cases sets bits 0, which is the enable bit. So we enable LCDC before
> even setting the fb address. How does this not blow up LCDC totally?
> 
> The fix looks correct to me, but it will change the register value for
> boards that have apparently been working for years.
> 
> Dario, did you test this on actual HW, or did you just spot the error?

I tested it on Beaglebone Black + LCD cape (4.3inch).
I also checked the register value with devmem.

Regards,
Dario

> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
>  Tomi
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jyri Sarha Feb. 18, 2021, 4 p.m. UTC | #3
On 2021-02-16 22:22, Dario Binacchi wrote:
> The fdd property of the tilcdc_panel_info structure must set the reqdly
> bit field  (bit 12 to 19) of the raster control register. The previous
> statement set the least significant bit instead.
> 
> Signed-off-by: Dario Binacchi <dariobin@libero.it>

Reviewed-by: Jyri Sarha <jyri.sarha@iki.fi>
Tested-by: Jyri Sarha <jyri.sarha@iki.fi>

Thanks for a good catch. I'll merge to this drm-misc-next soon.

Best regards,
Jyri

> 
> ---
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 30213708fc99..238068e28729 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -393,7 +393,7 @@ static void tilcdc_crtc_set_mode(struct drm_crtc 
> *crtc)
>  			return;
>  		}
>  	}
> -	reg |= info->fdd < 12;
> +	reg |= info->fdd << 12;
>  	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
> 
>  	if (info->invert_pxl_clk)

Patch
diff mbox series

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 30213708fc99..238068e28729 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -393,7 +393,7 @@  static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 			return;
 		}
 	}
-	reg |= info->fdd < 12;
+	reg |= info->fdd << 12;
 	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
 
 	if (info->invert_pxl_clk)