diff mbox

drm/imx: use bus_flags for pixel clock polarity

Message ID 1462892861-10449-1-git-send-email-p.zabel@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Philipp Zabel May 10, 2016, 3:07 p.m. UTC
This patch allows panels to set pixel clock and data enable pin polarity
other than the default of driving data at the rising pixel clock edge and
active high display enable.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-drm-core.c     | 9 ++++++---
 drivers/gpu/drm/imx/imx-drm.h          | 5 +++--
 drivers/gpu/drm/imx/imx-tve.c          | 4 +++-
 drivers/gpu/drm/imx/ipuv3-crtc.c       | 9 ++++++---
 drivers/gpu/drm/imx/parallel-display.c | 4 ++--
 5 files changed, 20 insertions(+), 11 deletions(-)

Comments

Stefan Agner May 10, 2016, 7:41 p.m. UTC | #1
On 2016-05-10 08:07, Philipp Zabel wrote:
> This patch allows panels to set pixel clock and data enable pin polarity
> other than the default of driving data at the rising pixel clock edge and
> active high display enable.




> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c     | 9 ++++++---
>  drivers/gpu/drm/imx/imx-drm.h          | 5 +++--
>  drivers/gpu/drm/imx/imx-tve.c          | 4 +++-
>  drivers/gpu/drm/imx/ipuv3-crtc.c       | 9 ++++++---
>  drivers/gpu/drm/imx/parallel-display.c | 4 ++--
>  5 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
> b/drivers/gpu/drm/imx/imx-drm-core.c
> index 2453fb1..6e5891a 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -97,7 +97,7 @@ static struct imx_drm_crtc *imx_drm_find_crtc(struct
> drm_crtc *crtc)
>  }
>  
>  int imx_drm_set_bus_format_pins(struct drm_encoder *encoder, u32 bus_format,
> -		int hsync_pin, int vsync_pin)
> +		int hsync_pin, int vsync_pin, u32 bus_flags)
>  {
>  	struct imx_drm_crtc_helper_funcs *helper;
>  	struct imx_drm_crtc *imx_crtc;
> @@ -109,14 +109,17 @@ int imx_drm_set_bus_format_pins(struct
> drm_encoder *encoder, u32 bus_format,
>  	helper = &imx_crtc->imx_drm_helper_funcs;
>  	if (helper->set_interface_pix_fmt)
>  		return helper->set_interface_pix_fmt(encoder->crtc,
> -					bus_format, hsync_pin, vsync_pin);
> +					bus_format, hsync_pin, vsync_pin,
> +					bus_flags);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(imx_drm_set_bus_format_pins);
>  
>  int imx_drm_set_bus_format(struct drm_encoder *encoder, u32 bus_format)
>  {
> -	return imx_drm_set_bus_format_pins(encoder, bus_format, 2, 3);
> +	return imx_drm_set_bus_format_pins(encoder, bus_format, 2, 3,
> +					   DRM_BUS_FLAG_DE_HIGH |
> +					   DRM_BUS_FLAG_PIXDATA_POSEDGE);
>  }
>  EXPORT_SYMBOL_GPL(imx_drm_set_bus_format);
>  
> diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
> index b0241b9..1400664 100644
> --- a/drivers/gpu/drm/imx/imx-drm.h
> +++ b/drivers/gpu/drm/imx/imx-drm.h
> @@ -19,7 +19,8 @@ struct imx_drm_crtc_helper_funcs {
>  	int (*enable_vblank)(struct drm_crtc *crtc);
>  	void (*disable_vblank)(struct drm_crtc *crtc);
>  	int (*set_interface_pix_fmt)(struct drm_crtc *crtc,
> -			u32 bus_format, int hsync_pin, int vsync_pin);
> +			u32 bus_format, int hsync_pin, int vsync_pin,
> +			u32 bus_flags);
>  	const struct drm_crtc_helper_funcs *crtc_helper_funcs;
>  	const struct drm_crtc_funcs *crtc_funcs;
>  };
> @@ -42,7 +43,7 @@ void imx_drm_mode_config_init(struct drm_device *drm);
>  struct drm_gem_cma_object *imx_drm_fb_get_obj(struct drm_framebuffer *fb);
>  
>  int imx_drm_set_bus_format_pins(struct drm_encoder *encoder,
> -		u32 bus_format, int hsync_pin, int vsync_pin);
> +		u32 bus_format, int hsync_pin, int vsync_pin, u32 bus_flags);
>  int imx_drm_set_bus_format(struct drm_encoder *encoder,
>  		u32 bus_format);
>  
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index ae7a9fb..cb25582 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -295,7 +295,9 @@ static void imx_tve_encoder_prepare(struct
> drm_encoder *encoder)
>  	switch (tve->mode) {
>  	case TVE_MODE_VGA:
>  		imx_drm_set_bus_format_pins(encoder, MEDIA_BUS_FMT_GBR888_1X24,
> -					    tve->hsync_pin, tve->vsync_pin);
> +					    tve->hsync_pin, tve->vsync_pin,
> +					    DRM_BUS_FLAG_DE_HIGH |
> +					    DRM_BUS_FLAG_PIXDATA_POSEDGE);
>  		break;
>  	case TVE_MODE_TVOUT:
>  		imx_drm_set_bus_format(encoder, MEDIA_BUS_FMT_YUV8_1X24);
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index dee8e8b..d8b58b0 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -66,6 +66,7 @@ struct ipu_crtc {
>  	struct ipu_flip_work	*flip_work;
>  	int			irq;
>  	u32			bus_format;
> +	u32			bus_flags;
>  	int			di_hsync_pin;
>  	int			di_vsync_pin;
>  };
> @@ -271,8 +272,9 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc,
>  	else
>  		sig_cfg.clkflags = 0;
>  
> -	sig_cfg.enable_pol = 1;
> -	sig_cfg.clk_pol = 0;
> +	sig_cfg.enable_pol = !(ipu_crtc->bus_flags & DRM_BUS_FLAG_DE_LOW);
> +	sig_cfg.clk_pol = !!(ipu_crtc->bus_flags &
> +			     DRM_BUS_FLAG_PIXDATA_NEGEDGE);

I think this polarity is actually wrong...

My data sheet says: IPU_DI0_GENERAL di0_polarity_disp_clk:
- 1 The output clock is active high
- 0 The output clock is active low

What active high/low means in this context is totally unclear to me, so
I looked at the actual signals. 
- 1 => The controller drives the data signal on rising edge
- 0 => The controller drives the data signal on falling edge

https://drive.google.com/file/d/0B8pBTl0mP98PVEpCRUJrNjJJakE/view
https://drive.google.com/file/d/0B8pBTl0mP98POTBvRW9ITVpQN00/view

Afaik, most displays sample on rising edge. Hence controllers should
drive on falling edge. This actually also matches the old default (0).

Since most display do not explicitly specify a polarity, it should
default to DRM_BUS_FLAG_PIXDATA_NEGEDGE (the flags are controller
centric, negedge means controller drives on falling edge, display
samples on positive/rising edge).

You should set clk_pol only if a display explicitly requests the
controller to drive on rising edge:
+	sig_cfg.clk_pol = !!(ipu_crtc->bus_flags &
+			     DRM_BUS_FLAG_PIXDATA_POSEDGE);

...and call the function in the other places using the
DRM_BUS_FLAG_PIXDATA_NEGEDGE flag.

--
Stefan


>  	sig_cfg.bus_format = ipu_crtc->bus_format;
>  	sig_cfg.v_to_h_sync = 0;
>  	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
> @@ -396,11 +398,12 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
>  }
>  
>  static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc,
> -		u32 bus_format, int hsync_pin, int vsync_pin)
> +		u32 bus_format, int hsync_pin, int vsync_pin, u32 bus_flags)
>  {
>  	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
>  
>  	ipu_crtc->bus_format = bus_format;
> +	ipu_crtc->bus_flags = bus_flags;
>  	ipu_crtc->di_hsync_pin = hsync_pin;
>  	ipu_crtc->di_vsync_pin = vsync_pin;
>  
> diff --git a/drivers/gpu/drm/imx/parallel-display.c
> b/drivers/gpu/drm/imx/parallel-display.c
> index 363e2c7..030235f 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -115,8 +115,8 @@ static void imx_pd_encoder_dpms(struct drm_encoder
> *encoder, int mode)
>  static void imx_pd_encoder_prepare(struct drm_encoder *encoder)
>  {
>  	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
> -
> -	imx_drm_set_bus_format(encoder, imxpd->bus_format);
> +	imx_drm_set_bus_format_pins(encoder, imxpd->bus_format, 2, 3,
> +				    imxpd->connector.display_info.bus_flags);
>  }
>  
>  static void imx_pd_encoder_commit(struct drm_encoder *encoder)
diff mbox

Patch

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 2453fb1..6e5891a 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -97,7 +97,7 @@  static struct imx_drm_crtc *imx_drm_find_crtc(struct drm_crtc *crtc)
 }
 
 int imx_drm_set_bus_format_pins(struct drm_encoder *encoder, u32 bus_format,
-		int hsync_pin, int vsync_pin)
+		int hsync_pin, int vsync_pin, u32 bus_flags)
 {
 	struct imx_drm_crtc_helper_funcs *helper;
 	struct imx_drm_crtc *imx_crtc;
@@ -109,14 +109,17 @@  int imx_drm_set_bus_format_pins(struct drm_encoder *encoder, u32 bus_format,
 	helper = &imx_crtc->imx_drm_helper_funcs;
 	if (helper->set_interface_pix_fmt)
 		return helper->set_interface_pix_fmt(encoder->crtc,
-					bus_format, hsync_pin, vsync_pin);
+					bus_format, hsync_pin, vsync_pin,
+					bus_flags);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(imx_drm_set_bus_format_pins);
 
 int imx_drm_set_bus_format(struct drm_encoder *encoder, u32 bus_format)
 {
-	return imx_drm_set_bus_format_pins(encoder, bus_format, 2, 3);
+	return imx_drm_set_bus_format_pins(encoder, bus_format, 2, 3,
+					   DRM_BUS_FLAG_DE_HIGH |
+					   DRM_BUS_FLAG_PIXDATA_POSEDGE);
 }
 EXPORT_SYMBOL_GPL(imx_drm_set_bus_format);
 
diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
index b0241b9..1400664 100644
--- a/drivers/gpu/drm/imx/imx-drm.h
+++ b/drivers/gpu/drm/imx/imx-drm.h
@@ -19,7 +19,8 @@  struct imx_drm_crtc_helper_funcs {
 	int (*enable_vblank)(struct drm_crtc *crtc);
 	void (*disable_vblank)(struct drm_crtc *crtc);
 	int (*set_interface_pix_fmt)(struct drm_crtc *crtc,
-			u32 bus_format, int hsync_pin, int vsync_pin);
+			u32 bus_format, int hsync_pin, int vsync_pin,
+			u32 bus_flags);
 	const struct drm_crtc_helper_funcs *crtc_helper_funcs;
 	const struct drm_crtc_funcs *crtc_funcs;
 };
@@ -42,7 +43,7 @@  void imx_drm_mode_config_init(struct drm_device *drm);
 struct drm_gem_cma_object *imx_drm_fb_get_obj(struct drm_framebuffer *fb);
 
 int imx_drm_set_bus_format_pins(struct drm_encoder *encoder,
-		u32 bus_format, int hsync_pin, int vsync_pin);
+		u32 bus_format, int hsync_pin, int vsync_pin, u32 bus_flags);
 int imx_drm_set_bus_format(struct drm_encoder *encoder,
 		u32 bus_format);
 
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index ae7a9fb..cb25582 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -295,7 +295,9 @@  static void imx_tve_encoder_prepare(struct drm_encoder *encoder)
 	switch (tve->mode) {
 	case TVE_MODE_VGA:
 		imx_drm_set_bus_format_pins(encoder, MEDIA_BUS_FMT_GBR888_1X24,
-					    tve->hsync_pin, tve->vsync_pin);
+					    tve->hsync_pin, tve->vsync_pin,
+					    DRM_BUS_FLAG_DE_HIGH |
+					    DRM_BUS_FLAG_PIXDATA_POSEDGE);
 		break;
 	case TVE_MODE_TVOUT:
 		imx_drm_set_bus_format(encoder, MEDIA_BUS_FMT_YUV8_1X24);
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index dee8e8b..d8b58b0 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -66,6 +66,7 @@  struct ipu_crtc {
 	struct ipu_flip_work	*flip_work;
 	int			irq;
 	u32			bus_format;
+	u32			bus_flags;
 	int			di_hsync_pin;
 	int			di_vsync_pin;
 };
@@ -271,8 +272,9 @@  static int ipu_crtc_mode_set(struct drm_crtc *crtc,
 	else
 		sig_cfg.clkflags = 0;
 
-	sig_cfg.enable_pol = 1;
-	sig_cfg.clk_pol = 0;
+	sig_cfg.enable_pol = !(ipu_crtc->bus_flags & DRM_BUS_FLAG_DE_LOW);
+	sig_cfg.clk_pol = !!(ipu_crtc->bus_flags &
+			     DRM_BUS_FLAG_PIXDATA_NEGEDGE);
 	sig_cfg.bus_format = ipu_crtc->bus_format;
 	sig_cfg.v_to_h_sync = 0;
 	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
@@ -396,11 +398,12 @@  static void ipu_disable_vblank(struct drm_crtc *crtc)
 }
 
 static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc,
-		u32 bus_format, int hsync_pin, int vsync_pin)
+		u32 bus_format, int hsync_pin, int vsync_pin, u32 bus_flags)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 
 	ipu_crtc->bus_format = bus_format;
+	ipu_crtc->bus_flags = bus_flags;
 	ipu_crtc->di_hsync_pin = hsync_pin;
 	ipu_crtc->di_vsync_pin = vsync_pin;
 
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 363e2c7..030235f 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -115,8 +115,8 @@  static void imx_pd_encoder_dpms(struct drm_encoder *encoder, int mode)
 static void imx_pd_encoder_prepare(struct drm_encoder *encoder)
 {
 	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
-
-	imx_drm_set_bus_format(encoder, imxpd->bus_format);
+	imx_drm_set_bus_format_pins(encoder, imxpd->bus_format, 2, 3,
+				    imxpd->connector.display_info.bus_flags);
 }
 
 static void imx_pd_encoder_commit(struct drm_encoder *encoder)