All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Dario Binacchi <dariobin@libero.it>,
	linux-kernel@vger.kernel.org, Jyri Sarha <jyri.sarha@iki.fi>
Cc: Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	dri-devel@lists.freedesktop.org
Subject: Re: [RESEND PATCH] drm/tilcdc: fix raster control register setting
Date: Wed, 17 Feb 2021 08:41:15 +0200	[thread overview]
Message-ID: <07f7a7c0-8016-bf32-92ad-b9de4aaed84c@ideasonboard.com> (raw)
In-Reply-To: <20210216202225.12861-1-dariobin@libero.it>

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

WARNING: multiple messages have this Message-ID (diff)
From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Dario Binacchi <dariobin@libero.it>,
	linux-kernel@vger.kernel.org, Jyri Sarha <jyri.sarha@iki.fi>
Cc: David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org
Subject: Re: [RESEND PATCH] drm/tilcdc: fix raster control register setting
Date: Wed, 17 Feb 2021 08:41:15 +0200	[thread overview]
Message-ID: <07f7a7c0-8016-bf32-92ad-b9de4aaed84c@ideasonboard.com> (raw)
In-Reply-To: <20210216202225.12861-1-dariobin@libero.it>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2021-02-17  6:42 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-16 20:22 [RESEND PATCH] drm/tilcdc: fix raster control register setting Dario Binacchi
2021-02-16 20:22 ` Dario Binacchi
2021-02-17  6:41 ` Tomi Valkeinen [this message]
2021-02-17  6:41   ` Tomi Valkeinen
2021-02-17 17:45   ` Dario Binacchi
2021-02-17 17:45     ` Dario Binacchi
2021-02-18 16:00 ` Jyri Sarha
2021-02-18 16:00   ` Jyri Sarha
  -- strict thread matches above, loose matches on Subject: below --
2021-02-16 20:17 Dario Binacchi
2021-02-16 20:17 ` Dario Binacchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=07f7a7c0-8016-bf32-92ad-b9de4aaed84c@ideasonboard.com \
    --to=tomi.valkeinen@ideasonboard.com \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=dariobin@libero.it \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jyri.sarha@iki.fi \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.