[PATCH 0/2] drm/tilcdc: A fix and a clean-up for tilcdc

[PATCH 0/2] drm/tilcdc: A fix and a clean-up for tilcdc

[Jyri Sarha via dri-devel]

- "drm/tilcdc: use drm_fbdev_generic_setup"
 - a simple bufix, which also takes the code to the right direction

- "drm/tilcdc: Remove unnecessary struct tilcdc_panel_info members"
 - Code clean up, where the final target is to get rid of
   "struct tilcdc_panel_info" completely and eventually use
   "enum drm_bus_flags" instead. However, before the final step I'll
   wait for the latest bus_flags related patches to land.

Jyri Sarha (2):
  drm/tilcdc: use drm_fbdev_generic_setup
  drm/tilcdc: Remove unnecessary struct tilcdc_panel_info members

 .../bindings/display/tilcdc/panel.txt         | 20 +++++---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c          | 50 +++++--------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c           |  9 ++--
 drivers/gpu/drm/tilcdc/tilcdc_drv.h           | 27 ----------
 drivers/gpu/drm/tilcdc/tilcdc_external.c      | 16 ------
 drivers/gpu/drm/tilcdc/tilcdc_panel.c         |  9 ----
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c        |  8 ---
 7 files changed, 27 insertions(+), 112 deletions(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/tilcdc: use drm_fbdev_generic_setup

[Jyri Sarha via dri-devel]

Calling drm_fbdev_cma_fini() after drm_dev_unregister() started to
cause a crash when unloading tilcdc some time between 4.14 and
4.19. Instead of changing the unload order it looks like using
drm_fbdev_generic_setup() is the direction to go.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 33e533268488..6d84491f2a4f 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -192,8 +192,6 @@ static void tilcdc_fini(struct drm_device *dev)
 
 	drm_kms_helper_poll_fini(dev);
 
-	drm_fb_cma_fbdev_fini(dev);
-
 	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
 	tilcdc_remove_external_device(dev);
@@ -396,10 +394,6 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 
 	drm_mode_config_reset(ddev);
 
-	ret = drm_fb_cma_fbdev_init(ddev, bpp, 0);
-	if (ret)
-		goto init_failed;
-
 	drm_kms_helper_poll_init(ddev);
 
 	ret = drm_dev_register(ddev, 0);
@@ -407,6 +401,9 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 		goto init_failed;
 
 	priv->is_registered = true;
+
+	drm_fbdev_generic_setup(ddev, bpp);
+
 	return 0;
 
 init_failed:
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/tilcdc: use drm_fbdev_generic_setup

[Tomi Valkeinen via dri-devel]

On 15/02/2019 10:13, Jyri Sarha wrote:
> Calling drm_fbdev_cma_fini() after drm_dev_unregister() started to
> cause a crash when unloading tilcdc some time between 4.14 and
> 4.19. Instead of changing the unload order it looks like using
> drm_fbdev_generic_setup() is the direction to go.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

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

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/tilcdc: use drm_fbdev_generic_setup

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Fri, Feb 15, 2019 at 10:13:15AM +0200, Jyri Sarha wrote:
> Calling drm_fbdev_cma_fini() after drm_dev_unregister() started to
> cause a crash when unloading tilcdc some time between 4.14 and
> 4.19. Instead of changing the unload order it looks like using
> drm_fbdev_generic_setup() is the direction to go.

Yes, it's the way to go, but I'm afraid you're a bit too late:

commit 45cf87566e5686c919247610200af7b365e86a1f
Author: Noralf Trønnes <noralf@tronnes.org>
Date:   Thu Oct 25 22:13:39 2018 +0200

    drm/tilcdc: Use drm_fbdev_generic_setup()

got merged in v5.0.

> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 33e533268488..6d84491f2a4f 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -192,8 +192,6 @@ static void tilcdc_fini(struct drm_device *dev)
>  
>  	drm_kms_helper_poll_fini(dev);
>  
> -	drm_fb_cma_fbdev_fini(dev);
> -
>  	drm_irq_uninstall(dev);
>  	drm_mode_config_cleanup(dev);
>  	tilcdc_remove_external_device(dev);
> @@ -396,10 +394,6 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
>  
>  	drm_mode_config_reset(ddev);
>  
> -	ret = drm_fb_cma_fbdev_init(ddev, bpp, 0);
> -	if (ret)
> -		goto init_failed;
> -
>  	drm_kms_helper_poll_init(ddev);
>  
>  	ret = drm_dev_register(ddev, 0);
> @@ -407,6 +401,9 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
>  		goto init_failed;
>  
>  	priv->is_registered = true;
> +
> +	drm_fbdev_generic_setup(ddev, bpp);
> +
>  	return 0;
>  
>  init_failed:

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/tilcdc: Remove unnecessary struct tilcdc_panel_info members

[Jyri Sarha via dri-devel]

Most of the struct tilcdc_panel_info data members, that are also
exposed in dts binding, are essentially display IP register bits that
should not need customization per connected display basis. This patch
removes them, both from the binding and the struct. The removed data
members have sane static values and there should be no need to expose
them in the device tree, certainly not in the panel node.

The removed data members are: ac_bias, ac_bias_intrpt, dma_burst_sz,
bpp, fdd, sync-ctrl, tft_alt_mode, and raster_order. All of those are
removed from the driver implementation and from bundled tilcdc panel
binding. The driver now configures the tilcdc with the sane static
values, instead of whatever the driver finds from the struct
tilcdc_panel_info.

The patch does does not cause any functional change to any platform
supported by Linux mainline kernel and devicetree files.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../bindings/display/tilcdc/panel.txt         | 20 +++++---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c          | 50 +++++--------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h           | 27 ----------
 drivers/gpu/drm/tilcdc/tilcdc_external.c      | 16 ------
 drivers/gpu/drm/tilcdc/tilcdc_panel.c         |  9 ----
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c        |  8 ---
 6 files changed, 24 insertions(+), 106 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/tilcdc/panel.txt b/Documentation/devicetree/bindings/display/tilcdc/panel.txt
index 808216310ea2..b3d63e3db714 100644
--- a/Documentation/devicetree/bindings/display/tilcdc/panel.txt
+++ b/Documentation/devicetree/bindings/display/tilcdc/panel.txt
@@ -3,15 +3,19 @@ Device-Tree bindings for tilcdc DRM generic panel output driver
 Required properties:
  - compatible: value should be "ti,tilcdc,panel".
  - panel-info: configuration info to configure LCDC correctly for the panel
-   - ac-bias: AC Bias Pin Frequency
-   - ac-bias-intrpt: AC Bias Pin Transitions per Interrupt
-   - dma-burst-sz: DMA burst size
-   - bpp: Bits per pixel
-   - fdd: FIFO DMA Request Delay
    - sync-edge: Horizontal and Vertical Sync Edge: 0=rising 1=falling
-   - sync-ctrl: Horizontal and Vertical Sync: Control: 0=ignore
-   - raster-order: Raster Data Order Select: 1=Most-to-least 0=Least-to-most
-   - fifo-th: DMA FIFO threshold
+
+   - For compatibility with the older driver versions it is still good
+     keep these obsolete properties (with their recommended vaules):
+     - ac-bias           = <255>;
+     - ac-bias-intrpt    = <0>;
+     - dma-burst-sz      = <16>;
+     - bpp               = <16>;
+     - fdd               = <0x80>;
+     - sync-ctrl         = <1>;
+     - raster-order      = <0>;
+     - fifo-th           = <0>;
+
  - display-timings: typical videomode of lcd panel.  Multiple video modes
    can be listed if the panel supports multiple timings, but the 'native-mode'
    should be the preferred/default resolution.  Refer to
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 1067e702c22c..ea2e4afad75c 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -295,29 +295,11 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 		return;
 
 	/* Configure the Burst Size and fifo threshold of DMA: */
-	reg = tilcdc_read(dev, LCDC_DMA_CTRL_REG) & ~0x00000770;
-	switch (info->dma_burst_sz) {
-	case 1:
-		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_1);
-		break;
-	case 2:
-		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_2);
-		break;
-	case 4:
-		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_4);
-		break;
-	case 8:
-		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_8);
-		break;
-	case 16:
-		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16);
-		break;
-	default:
-		dev_err(dev->dev, "invalid burst size\n");
-		return;
-	}
-	reg |= (info->fifo_th << 8);
-	tilcdc_write(dev, LCDC_DMA_CTRL_REG, reg);
+	tilcdc_write_mask(dev, LCDC_DMA_CTRL_REG,
+			  LCDC_DMA_FIFO_THRESHOLD(0) |
+			  LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16),
+			  LCDC_DMA_FIFO_THRESHOLD_MASK |
+			  LCDC_DMA_BURST_SIZE_MASK);
 
 	/* Configure timings: */
 	hbp = mode->htotal - mode->hsync_end;
@@ -331,9 +313,11 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 	    mode->hdisplay, mode->vdisplay, hbp, hfp, hsw, vbp, vfp, vsw);
 
 	/* Set AC Bias Period and Number of Transitions per Interrupt: */
-	reg = tilcdc_read(dev, LCDC_RASTER_TIMING_2_REG) & ~0x000fff00;
-	reg |= LCDC_AC_BIAS_FREQUENCY(info->ac_bias) |
-		LCDC_AC_BIAS_TRANSITIONS_PER_INT(info->ac_bias_intrpt);
+	reg = tilcdc_read(dev, LCDC_RASTER_TIMING_2_REG);
+	reg &= ~LCDC_AC_BIAS_FREQUENCY_MASK;
+	reg |= LCDC_AC_BIAS_FREQUENCY(255);
+	reg &= ~LCDC_AC_BIAS_TRANSITIONS_PER_INT_MASK;
+	reg |= LCDC_AC_BIAS_TRANSITIONS_PER_INT(0);
 
 	/*
 	 * subtract one from hfp, hbp, hsw because the hardware uses
@@ -383,8 +367,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 		  LCDC_V2_TFT_24BPP_MODE | LCDC_V2_TFT_24BPP_UNPACK |
 		  0x000ff000 /* Palette Loading Delay bits */);
 	reg |= LCDC_TFT_MODE; /* no monochrome/passive support */
-	if (info->tft_alt_mode)
-		reg |= LCDC_TFT_ALT_ENABLE;
 	if (priv->rev == 2) {
 		switch (fb->format->format) {
 		case DRM_FORMAT_BGR565:
@@ -403,7 +385,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 			return;
 		}
 	}
-	reg |= info->fdd < 12;
 	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
 
 	if (info->invert_pxl_clk)
@@ -411,10 +392,8 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 	else
 		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_PIXEL_CLOCK);
 
-	if (info->sync_ctrl)
-		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
-	else
-		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
+	/* Take sync polarity from LCDC_SYNC_EDGE bit */
+	tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
 
 	if (info->sync_edge)
 		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
@@ -431,11 +410,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 	else
 		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_VSYNC);
 
-	if (info->raster_order)
-		tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
-	else
-		tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
-
 	tilcdc_crtc_set_clk(crtc);
 
 	tilcdc_crtc_load_palette(crtc);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 62cea5ff5558..acf27e22c409 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -125,38 +125,11 @@ void tilcdc_module_cleanup(struct tilcdc_module *mod);
  */
 struct tilcdc_panel_info {
 
-	/* AC Bias Pin Frequency */
-	uint32_t ac_bias;
-
-	/* AC Bias Pin Transitions per Interrupt */
-	uint32_t ac_bias_intrpt;
-
-	/* DMA burst size */
-	uint32_t dma_burst_sz;
-
-	/* Bits per pixel */
-	uint32_t bpp;
-
-	/* FIFO DMA Request Delay */
-	uint32_t fdd;
-
-	/* TFT Alternative Signal Mapping (Only for active) */
-	bool tft_alt_mode;
-
 	/* Invert pixel clock */
 	bool invert_pxl_clk;
 
 	/* Horizontal and Vertical Sync Edge: 0=rising 1=falling */
 	uint32_t sync_edge;
-
-	/* Horizontal and Vertical Sync: Control: 0=ignore */
-	uint32_t sync_ctrl;
-
-	/* Raster Data Order Select: 1=Most-to-least 0=Least-to-most */
-	uint32_t raster_order;
-
-	/* DMA FIFO threshold */
-	uint32_t fifo_th;
 };
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
index b4eaf9bc87f8..9626cccf0d17 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
@@ -16,28 +16,12 @@
 #include "tilcdc_external.h"
 
 static const struct tilcdc_panel_info panel_info_tda998x = {
-		.ac_bias                = 255,
-		.ac_bias_intrpt         = 0,
-		.dma_burst_sz           = 16,
-		.bpp                    = 16,
-		.fdd                    = 0x80,
-		.tft_alt_mode           = 0,
 		.invert_pxl_clk		= 1,
 		.sync_edge              = 1,
-		.sync_ctrl              = 1,
-		.raster_order           = 0,
 };
 
 static const struct tilcdc_panel_info panel_info_default = {
-		.ac_bias                = 255,
-		.ac_bias_intrpt         = 0,
-		.dma_burst_sz           = 16,
-		.bpp                    = 16,
-		.fdd                    = 0x80,
-		.tft_alt_mode           = 0,
 		.sync_edge              = 0,
-		.sync_ctrl              = 1,
-		.raster_order           = 0,
 };
 
 static int tilcdc_external_mode_valid(struct drm_connector *connector,
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index a1acab39d87f..1f788171cdbb 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -292,18 +292,9 @@ static struct tilcdc_panel_info *of_get_panel_info(struct device_node *np)
 	if (!info)
 		goto put_node;
 
-	ret |= of_property_read_u32(info_np, "ac-bias", &info->ac_bias);
-	ret |= of_property_read_u32(info_np, "ac-bias-intrpt", &info->ac_bias_intrpt);
-	ret |= of_property_read_u32(info_np, "dma-burst-sz", &info->dma_burst_sz);
-	ret |= of_property_read_u32(info_np, "bpp", &info->bpp);
-	ret |= of_property_read_u32(info_np, "fdd", &info->fdd);
 	ret |= of_property_read_u32(info_np, "sync-edge", &info->sync_edge);
-	ret |= of_property_read_u32(info_np, "sync-ctrl", &info->sync_ctrl);
-	ret |= of_property_read_u32(info_np, "raster-order", &info->raster_order);
-	ret |= of_property_read_u32(info_np, "fifo-th", &info->fifo_th);
 
 	/* optional: */
-	info->tft_alt_mode      = of_property_read_bool(info_np, "tft-alt-mode");
 	info->invert_pxl_clk    = of_property_read_bool(info_np, "invert-pxl-clk");
 
 	if (ret) {
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index daebf1aa6b0a..12404401b337 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -34,15 +34,7 @@ struct tfp410_module {
 
 
 static const struct tilcdc_panel_info dvi_info = {
-		.ac_bias                = 255,
-		.ac_bias_intrpt         = 0,
-		.dma_burst_sz           = 16,
-		.bpp                    = 16,
-		.fdd                    = 0x80,
-		.tft_alt_mode           = 0,
 		.sync_edge              = 0,
-		.sync_ctrl              = 1,
-		.raster_order           = 0,
 };
 
 /*
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/tilcdc: Remove unnecessary struct tilcdc_panel_info members

[Tomi Valkeinen via dri-devel]

On 15/02/2019 10:13, Jyri Sarha wrote:
> Most of the struct tilcdc_panel_info data members, that are also
> exposed in dts binding, are essentially display IP register bits that
> should not need customization per connected display basis. This patch
> removes them, both from the binding and the struct. The removed data
> members have sane static values and there should be no need to expose
> them in the device tree, certainly not in the panel node.
> 
> The removed data members are: ac_bias, ac_bias_intrpt, dma_burst_sz,
> bpp, fdd, sync-ctrl, tft_alt_mode, and raster_order. All of those are
> removed from the driver implementation and from bundled tilcdc panel
> binding. The driver now configures the tilcdc with the sane static
> values, instead of whatever the driver finds from the struct
> tilcdc_panel_info.
> 
> The patch does does not cause any functional change to any platform
> supported by Linux mainline kernel and devicetree files.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../bindings/display/tilcdc/panel.txt         | 20 +++++---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c          | 50 +++++--------------
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h           | 27 ----------
>  drivers/gpu/drm/tilcdc/tilcdc_external.c      | 16 ------
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c         |  9 ----
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c        |  8 ---
>  6 files changed, 24 insertions(+), 106 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/tilcdc/panel.txt b/Documentation/devicetree/bindings/display/tilcdc/panel.txt
> index 808216310ea2..b3d63e3db714 100644
> --- a/Documentation/devicetree/bindings/display/tilcdc/panel.txt
> +++ b/Documentation/devicetree/bindings/display/tilcdc/panel.txt
> @@ -3,15 +3,19 @@ Device-Tree bindings for tilcdc DRM generic panel output driver
>  Required properties:
>   - compatible: value should be "ti,tilcdc,panel".
>   - panel-info: configuration info to configure LCDC correctly for the panel
> -   - ac-bias: AC Bias Pin Frequency
> -   - ac-bias-intrpt: AC Bias Pin Transitions per Interrupt
> -   - dma-burst-sz: DMA burst size
> -   - bpp: Bits per pixel
> -   - fdd: FIFO DMA Request Delay
>     - sync-edge: Horizontal and Vertical Sync Edge: 0=rising 1=falling
> -   - sync-ctrl: Horizontal and Vertical Sync: Control: 0=ignore
> -   - raster-order: Raster Data Order Select: 1=Most-to-least 0=Least-to-most
> -   - fifo-th: DMA FIFO threshold

Removing all these sounds ok to me, as long as no existing board breaks.
Many of these are clearly something that's not supposed to be in DT. I
think only the bpp and sync-ctrl are something that might have a place
in DT.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/tilcdc: Remove unnecessary struct tilcdc_panel_info members

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Fri, Feb 15, 2019 at 10:13:16AM +0200, Jyri Sarha wrote:
> Most of the struct tilcdc_panel_info data members, that are also
> exposed in dts binding, are essentially display IP register bits that
> should not need customization per connected display basis. This patch
> removes them, both from the binding and the struct. The removed data
> members have sane static values and there should be no need to expose
> them in the device tree, certainly not in the panel node.
> 
> The removed data members are: ac_bias, ac_bias_intrpt, dma_burst_sz,
> bpp, fdd, sync-ctrl, tft_alt_mode, and raster_order. All of those are
> removed from the driver implementation and from bundled tilcdc panel
> binding. The driver now configures the tilcdc with the sane static
> values, instead of whatever the driver finds from the struct
> tilcdc_panel_info.
> 
> The patch does does not cause any functional change to any platform
> supported by Linux mainline kernel and devicetree files.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../bindings/display/tilcdc/panel.txt         | 20 +++++---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c          | 50 +++++--------------
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h           | 27 ----------
>  drivers/gpu/drm/tilcdc/tilcdc_external.c      | 16 ------
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c         |  9 ----
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c        |  8 ---
>  6 files changed, 24 insertions(+), 106 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/tilcdc/panel.txt b/Documentation/devicetree/bindings/display/tilcdc/panel.txt
> index 808216310ea2..b3d63e3db714 100644
> --- a/Documentation/devicetree/bindings/display/tilcdc/panel.txt
> +++ b/Documentation/devicetree/bindings/display/tilcdc/panel.txt
> @@ -3,15 +3,19 @@ Device-Tree bindings for tilcdc DRM generic panel output driver
>  Required properties:
>   - compatible: value should be "ti,tilcdc,panel".
>   - panel-info: configuration info to configure LCDC correctly for the panel
> -   - ac-bias: AC Bias Pin Frequency
> -   - ac-bias-intrpt: AC Bias Pin Transitions per Interrupt
> -   - dma-burst-sz: DMA burst size
> -   - bpp: Bits per pixel
> -   - fdd: FIFO DMA Request Delay
>     - sync-edge: Horizontal and Vertical Sync Edge: 0=rising 1=falling
> -   - sync-ctrl: Horizontal and Vertical Sync: Control: 0=ignore
> -   - raster-order: Raster Data Order Select: 1=Most-to-least 0=Least-to-most
> -   - fifo-th: DMA FIFO threshold
> +
> +   - For compatibility with the older driver versions it is still good
> +     keep these obsolete properties (with their recommended vaules):
> +     - ac-bias           = <255>;
> +     - ac-bias-intrpt    = <0>;
> +     - dma-burst-sz      = <16>;
> +     - bpp               = <16>;
> +     - fdd               = <0x80>;
> +     - sync-ctrl         = <1>;
> +     - raster-order      = <0>;
> +     - fifo-th           = <0>;
> +

I don't think you need to keep this text, the properties will be
available from the git history. We actually tend to do it the other way
around, and remove properties from the bindings and keep them in the
driver for backward-compatibility purpose. If removal of the properties
don't cause issues (I'll trust you on that) then backward-compatibility
code isn't mandatory.

>   - display-timings: typical videomode of lcd panel.  Multiple video modes
>     can be listed if the panel supports multiple timings, but the 'native-mode'
>     should be the preferred/default resolution.  Refer to
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 1067e702c22c..ea2e4afad75c 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -295,29 +295,11 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>  		return;
>  
>  	/* Configure the Burst Size and fifo threshold of DMA: */
> -	reg = tilcdc_read(dev, LCDC_DMA_CTRL_REG) & ~0x00000770;
> -	switch (info->dma_burst_sz) {
> -	case 1:
> -		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_1);
> -		break;
> -	case 2:
> -		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_2);
> -		break;
> -	case 4:
> -		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_4);
> -		break;
> -	case 8:
> -		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_8);
> -		break;
> -	case 16:
> -		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16);
> -		break;
> -	default:
> -		dev_err(dev->dev, "invalid burst size\n");
> -		return;
> -	}
> -	reg |= (info->fifo_th << 8);
> -	tilcdc_write(dev, LCDC_DMA_CTRL_REG, reg);
> +	tilcdc_write_mask(dev, LCDC_DMA_CTRL_REG,
> +			  LCDC_DMA_FIFO_THRESHOLD(0) |
> +			  LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16),
> +			  LCDC_DMA_FIFO_THRESHOLD_MASK |
> +			  LCDC_DMA_BURST_SIZE_MASK);
>  
>  	/* Configure timings: */
>  	hbp = mode->htotal - mode->hsync_end;
> @@ -331,9 +313,11 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>  	    mode->hdisplay, mode->vdisplay, hbp, hfp, hsw, vbp, vfp, vsw);
>  
>  	/* Set AC Bias Period and Number of Transitions per Interrupt: */
> -	reg = tilcdc_read(dev, LCDC_RASTER_TIMING_2_REG) & ~0x000fff00;
> -	reg |= LCDC_AC_BIAS_FREQUENCY(info->ac_bias) |
> -		LCDC_AC_BIAS_TRANSITIONS_PER_INT(info->ac_bias_intrpt);
> +	reg = tilcdc_read(dev, LCDC_RASTER_TIMING_2_REG);
> +	reg &= ~LCDC_AC_BIAS_FREQUENCY_MASK;
> +	reg |= LCDC_AC_BIAS_FREQUENCY(255);
> +	reg &= ~LCDC_AC_BIAS_TRANSITIONS_PER_INT_MASK;
> +	reg |= LCDC_AC_BIAS_TRANSITIONS_PER_INT(0);
>  
>  	/*
>  	 * subtract one from hfp, hbp, hsw because the hardware uses
> @@ -383,8 +367,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>  		  LCDC_V2_TFT_24BPP_MODE | LCDC_V2_TFT_24BPP_UNPACK |
>  		  0x000ff000 /* Palette Loading Delay bits */);
>  	reg |= LCDC_TFT_MODE; /* no monochrome/passive support */
> -	if (info->tft_alt_mode)
> -		reg |= LCDC_TFT_ALT_ENABLE;
>  	if (priv->rev == 2) {
>  		switch (fb->format->format) {
>  		case DRM_FORMAT_BGR565:
> @@ -403,7 +385,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>  			return;
>  		}
>  	}
> -	reg |= info->fdd < 12;
>  	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
>  
>  	if (info->invert_pxl_clk)
> @@ -411,10 +392,8 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>  	else
>  		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_PIXEL_CLOCK);
>  
> -	if (info->sync_ctrl)
> -		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
> -	else
> -		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
> +	/* Take sync polarity from LCDC_SYNC_EDGE bit */
> +	tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
>  
>  	if (info->sync_edge)
>  		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
> @@ -431,11 +410,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>  	else
>  		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_VSYNC);
>  
> -	if (info->raster_order)
> -		tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
> -	else
> -		tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
> -
>  	tilcdc_crtc_set_clk(crtc);
>  
>  	tilcdc_crtc_load_palette(crtc);
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index 62cea5ff5558..acf27e22c409 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -125,38 +125,11 @@ void tilcdc_module_cleanup(struct tilcdc_module *mod);
>   */
>  struct tilcdc_panel_info {
>  
> -	/* AC Bias Pin Frequency */
> -	uint32_t ac_bias;
> -
> -	/* AC Bias Pin Transitions per Interrupt */
> -	uint32_t ac_bias_intrpt;
> -
> -	/* DMA burst size */
> -	uint32_t dma_burst_sz;
> -
> -	/* Bits per pixel */
> -	uint32_t bpp;
> -
> -	/* FIFO DMA Request Delay */
> -	uint32_t fdd;
> -
> -	/* TFT Alternative Signal Mapping (Only for active) */
> -	bool tft_alt_mode;
> -
>  	/* Invert pixel clock */
>  	bool invert_pxl_clk;
>  
>  	/* Horizontal and Vertical Sync Edge: 0=rising 1=falling */
>  	uint32_t sync_edge;
> -
> -	/* Horizontal and Vertical Sync: Control: 0=ignore */
> -	uint32_t sync_ctrl;
> -
> -	/* Raster Data Order Select: 1=Most-to-least 0=Least-to-most */
> -	uint32_t raster_order;
> -
> -	/* DMA FIFO threshold */
> -	uint32_t fifo_th;
>  };
>  
>  #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> index b4eaf9bc87f8..9626cccf0d17 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> @@ -16,28 +16,12 @@
>  #include "tilcdc_external.h"
>  
>  static const struct tilcdc_panel_info panel_info_tda998x = {
> -		.ac_bias                = 255,
> -		.ac_bias_intrpt         = 0,
> -		.dma_burst_sz           = 16,
> -		.bpp                    = 16,
> -		.fdd                    = 0x80,
> -		.tft_alt_mode           = 0,
>  		.invert_pxl_clk		= 1,
>  		.sync_edge              = 1,
> -		.sync_ctrl              = 1,
> -		.raster_order           = 0,
>  };
>  
>  static const struct tilcdc_panel_info panel_info_default = {
> -		.ac_bias                = 255,
> -		.ac_bias_intrpt         = 0,
> -		.dma_burst_sz           = 16,
> -		.bpp                    = 16,
> -		.fdd                    = 0x80,
> -		.tft_alt_mode           = 0,
>  		.sync_edge              = 0,
> -		.sync_ctrl              = 1,
> -		.raster_order           = 0,
>  };
>  
>  static int tilcdc_external_mode_valid(struct drm_connector *connector,
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> index a1acab39d87f..1f788171cdbb 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> @@ -292,18 +292,9 @@ static struct tilcdc_panel_info *of_get_panel_info(struct device_node *np)
>  	if (!info)
>  		goto put_node;
>  
> -	ret |= of_property_read_u32(info_np, "ac-bias", &info->ac_bias);
> -	ret |= of_property_read_u32(info_np, "ac-bias-intrpt", &info->ac_bias_intrpt);
> -	ret |= of_property_read_u32(info_np, "dma-burst-sz", &info->dma_burst_sz);
> -	ret |= of_property_read_u32(info_np, "bpp", &info->bpp);
> -	ret |= of_property_read_u32(info_np, "fdd", &info->fdd);
>  	ret |= of_property_read_u32(info_np, "sync-edge", &info->sync_edge);
> -	ret |= of_property_read_u32(info_np, "sync-ctrl", &info->sync_ctrl);
> -	ret |= of_property_read_u32(info_np, "raster-order", &info->raster_order);
> -	ret |= of_property_read_u32(info_np, "fifo-th", &info->fifo_th);
>  
>  	/* optional: */
> -	info->tft_alt_mode      = of_property_read_bool(info_np, "tft-alt-mode");
>  	info->invert_pxl_clk    = of_property_read_bool(info_np, "invert-pxl-clk");
>  
>  	if (ret) {
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> index daebf1aa6b0a..12404401b337 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> @@ -34,15 +34,7 @@ struct tfp410_module {
>  
>  
>  static const struct tilcdc_panel_info dvi_info = {
> -		.ac_bias                = 255,
> -		.ac_bias_intrpt         = 0,
> -		.dma_burst_sz           = 16,
> -		.bpp                    = 16,
> -		.fdd                    = 0x80,
> -		.tft_alt_mode           = 0,
>  		.sync_edge              = 0,
> -		.sync_ctrl              = 1,
> -		.raster_order           = 0,
>  };
>  
>  /*

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel