drm/tilcdc: LCDC Revision 1 related fixes

Revision 1 LCDC support also sync lost errors and can benefit from sync lost recovery routine.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +++++++++++++++++----------------
 drivers/gpu/drm/tilcdc/tilcdc_regs.h |  1 +
 2 files changed, 18 insertions(+), 16 deletions(-)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +6 -1

index c787349..5260eb2 100644
@@ -113,6 +113,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
 
        if (priv->rev == 1) {
                tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
+                       LCDC_V1_SYNC_LOST_ENA |
                        LCDC_V1_UNDERFLOW_INT_ENA);
                tilcdc_set(dev, LCDC_DMA_CTRL_REG,
                        LCDC_V1_END_OF_FRAME_INT_ENA);

@@ -130,7 +131,7 @@ static void tilcdc_crtc_disable_irqs(struct drm_device *dev)
 
        /* disable irqs that we might have enabled: */
        if (priv->rev == 1) {
-               tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
+               tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_SYNC_LOST_ENA |
                        LCDC_V1_UNDERFLOW_INT_ENA | LCDC_V1_PL_INT_ENA);
                tilcdc_clear(dev, LCDC_DMA_CTRL_REG,
                        LCDC_V1_END_OF_FRAME_INT_ENA);

@@ -845,6 +846,21 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
                dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underflow",
                                    __func__, stat);
 
+       if (stat & LCDC_SYNC_LOST) {
+               dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",
+                                   __func__, stat);
+               tilcdc_crtc->frame_intact = false;

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +1 -15

+
        /* For revision 2 only */
        if (priv->rev == 2) {
                if (stat & LCDC_FRAME_DONE) {

@@ -852,21 +868,6 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
                        wake_up(&tilcdc_crtc->frame_done_wq);
                }
 
-               if (stat & LCDC_SYNC_LOST) {
-                       dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",
-                                           __func__, stat);
-                       tilcdc_crtc->frame_intact = false;
-                       if (tilcdc_crtc->sync_lost_count++ >
-                           SYNC_LOST_COUNT_LIMIT) {
-                               dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat);
-                               queue_work(system_wq,
-                                          &tilcdc_crtc->recover_work);
-                               tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
-                                            LCDC_SYNC_LOST);
-                               tilcdc_crtc->sync_lost_count = 0;
-                       }
-               }
-
                /* Indicate to LCDC that the interrupt service routine has
                 * completed, see 13.3.6.1.6 in AM335x TRM.
                 */

drivers/gpu/drm/tilcdc/tilcdc_regs.h +0 -0

index f57c0d6..beb8c21 100644

drivers/gpu/drm/tilcdc/tilcdc_regs.h +0 -1

 #define LCDC_MONOCHROME_MODE                     BIT(1)
 #define LCDC_RASTER_ENABLE                       BIT(0)
 #define LCDC_TFT_ALT_ENABLE                      BIT(23)
--

1.9.1

From: Bartosz Golaszewski <[email protected]>

Revision 1 of the IP doesn't work if we don't load the palette (even if it's not used, which is the case for the RGB565 format).

Add a function called from tilcdc_crtc_enable() which performs all required actions if we're dealing with a rev1 chip.

Signed-off-by Bartosz Golaszewski <[email protected]>

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 88 +++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +62 -1

index 5260eb2..0bfd7dd 100644
@@ -21,11 +21,15 @@
 #include <drm/drm_flip_work.h>
 #include <drm/drm_plane_helper.h>
 #include <linux/workqueue.h>
+#include <linux/completion.h>
+#include <linux/dma-mapping.h>
 
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
 
-#define TILCDC_VBLANK_SAFETY_THRESHOLD_US 1000
+#define TILCDC_VBLANK_SAFETY_THRESHOLD_US	1000
+#define TILCDC_REV1_PALETTE_SIZE		32
+#define TILCDC_REV1_PALETTE_FIRST_ENTRY		0x4000
 
 struct tilcdc_crtc {
 	struct drm_crtc base;

@@ -56,6 +60,10 @@ struct tilcdc_crtc {
 	int sync_lost_count;
 	bool frame_intact;
 	struct work_struct recover_work;
+
+	dma_addr_t palette_dma_handle;
+	void *palette_base;
+	struct completion palette_loaded;
 };
 #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
 

@@ -105,6 +113,55 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
 	tilcdc_crtc->curr_fb = fb;
 }
 
+/*
+ * The driver currently only supports the RGB565 format for revision 1. For
+ * 16 bits-per-pixel the palette block is bypassed, but the first 32 bytes of
+ * the framebuffer are still considered palette. The first 16-bit entry must
+ * be 0x4000 while all other entries must be zeroed.
+ */
+static void tilcdc_crtc_load_palette(struct drm_crtc *crtc)
+{
+	u32 dma_fb_base, dma_fb_ceiling, raster_ctl;
+	struct tilcdc_crtc *tilcdc_crtc;
+	struct drm_device *dev;
+	u16 *first_entry;
+
+	dev = crtc->dev;
+	tilcdc_crtc = to_tilcdc_crtc(crtc);
+	first_entry = tilcdc_crtc->palette_base;
+
+	*first_entry = TILCDC_REV1_PALETTE_FIRST_ENTRY;
+
+	dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG);
+	dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG);
+	raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG);
+
+	/* Tell the LCDC where the palette is located. */
+	tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG,
+		     tilcdc_crtc->palette_dma_handle);
+	tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG,
+		     (u32)tilcdc_crtc->palette_dma_handle
+				+ TILCDC_REV1_PALETTE_SIZE - 1);
+
+	/* Load it. */
+	tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
+		     LCDC_PALETTE_LOAD_MODE(DATA_ONLY));
+	tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
+		   LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY));
+
+	/* Enable the LCDC and wait for palette to be loaded. */
+	tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
+	tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+
+	wait_for_completion(&tilcdc_crtc->palette_loaded);
+
+	/* Restore the registers. */
+	tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+	tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base);
+	tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling);
+	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl);
+}
+
 static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;

@@ -160,6 +217,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct tilcdc_drm_private *priv = dev->dev_private;
 
 	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
 	mutex_lock(&tilcdc_crtc->enable_lock);

@@ -172,6 +230,9 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 
 	reset(crtc);
 
+	if (priv->rev == 1 && !completion_done(&tilcdc_crtc->palette_loaded))
+		tilcdc_crtc_load_palette(crtc);
+
 	tilcdc_crtc_enable_irqs(dev);
 
 	tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE);

Quoted 11 lines

(quoted diff) +0 -0

@@ -213,6 +274,13 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
 >  				__func__);
 >  	}

drivers/gpu/drm/tilcdc/tilcdc_crtc.c static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) · line 283

Quoted 8 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +7 -0

+	/*
+	 * LCDC will not retain the palette when reset. Make sure it gets
+	 * reloaded on tilcdc_crtc_enable().
+	 */
+	if (priv->rev == 1)
+		reinit_completion(&tilcdc_crtc->palette_loaded);
+

So when the crtc is disabled, you do this, so that on crtc enable the driver would load palette? When is the crtc enabled if it hasn't been disabled first? In other words, when will the palette loading in tilcdc_crtc_enable() _not_ trigger for v1?

This looks a bit messy to me.

Why not just load the palette every time on crtc enable? And reinit the completion in tilcdc_crtc_load_palette().

Quoted 16 lines · Tomi Valkeinen

Quoted 11 lines

(quoted diff) +0 -0

@@ -213,6 +274,13 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
 >  				__func__);
 >  	}

drivers/gpu/drm/tilcdc/tilcdc_crtc.c static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) · line 283

Quoted 8 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +7 -0

+	/*
+	 * LCDC will not retain the palette when reset. Make sure it gets
+	 * reloaded on tilcdc_crtc_enable().
+	 */
+	if (priv->rev == 1)
+		reinit_completion(&tilcdc_crtc->palette_loaded);
+

So when the crtc is disabled, you do this, so that on crtc enable the driver would load palette? When is the crtc enabled if it hasn't been disabled first? In other words, when will the palette loading in tilcdc_crtc_enable() _not_ trigger for v1?

Quoted 5 lines

This looks a bit messy to me.

Why not just load the palette every time on crtc enable? And reinit the completion in tilcdc_crtc_load_palette().

My final version loads it at the end of modeset_nofb(), to avoid this:

1. Load the fb address to dma registers with "ingenious" 64 bit write

2. Load the dma registers to a temporary storrage

3. Put palette address in dma registers and load the palette

4. Restore the dma registers (= fb address)

But, sure. I can load the palette every time the mode changes, but not every time the display is enabled.

What is the difference? If mode changes, you need to disable and enable the crtc, right? What other cases there are to enable the display? After blank? Then the display has been off, and I presume palette has to be loaded.

Quoted 4 lines · Tomi Valkeinen

What is the difference? If mode changes, you need to disable and enable the crtc, right? What other cases there are to enable the display? After blank? Then the display has been off, and I presume palette has to be loaded.

At the moment the palette or register values do not appear to vanish ever. But that is probably due to PM not doing much to optimize the LCDC power consumption.

Anyway, if simple enable is enough to turn on the display - all video timings, frame buffer dma addresses etc. are already in the registers - then I think it is safe to assume that the palette is still in there too.

Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.

Quoted 6 lines · Jyri Sarha

Quoted 4 lines · Tomi Valkeinen

What is the difference? If mode changes, you need to disable and enable the crtc, right? What other cases there are to enable the display? After blank? Then the display has been off, and I presume palette has to be loaded.

Quoted 3 lines

At the moment the palette or register values do not appear to vanish ever. But that is probably due to PM not doing much to optimize the LCDC power consumption.

If runtime PM for the device goes to suspend, you have to presume the IP has lost all context. That may not always happen, but that's what the driver has to presume, unless there's a way for the driver to verify whether the context has been lost or not.

Quoted 3 lines

Anyway, if simple enable is enough to turn on the display - all video timings, frame buffer dma addresses etc. are already in the registers - then I think it is safe to assume that the palette is still in there too.

As long as the driver makes sure the device doesn't go to suspend (by having called pm_runtime_get).

Quoted 6 lines

Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.

You can do things in resume callback, but I think quite often it's simplest to just do those things when enabling the display. The device never goes to suspend if the display is enabled. And if you disable the display, the device should go to suspend, so usually enable is called after the device has been in suspend.

So, I haven't looked at the tilcdc code in detail in this regard, but my guess is that runtime PM resume could be used to set hardcoded things to registers, stuff that you always know how they are set. Everything else can be just programmed at enable.

Looking at the registers to find out if they're intact is fine, but it's just an optimization.

Quoted 2 lines

As long as the driver makes sure the device doesn't go to suspend (by having called pm_runtime_get).

Runtime get has always been called when modeset_nofb() is called.

Quoted 6 lines

Quoted 5 lines

Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.

Quoted 6 lines

You can do things in resume callback, but I think quite often it's simplest to just do those things when enabling the display. The device never goes to suspend if the display is enabled. And if you disable the display, the device should go to suspend, so usually enable is called after the device has been in suspend.

Well, the two places are pretty much the same thing in tilcdc, because enable calls pm_runtime_get(). Also with atomic the suspend/resume implementation is really straight forward. Just get the current atomic state with drm_atomic_helper_suspend() and commit it back in with drm_atomic_helper_resume(), if needed. I don't think I should implement myself something that is so well provided by pm and drm infrastructure.

Quoted 4 lines · Jyri Sarha

Quoted 2 lines

As long as the driver makes sure the device doesn't go to suspend (by having called pm_runtime_get).

Runtime get has always been called when modeset_nofb() is called.

Yes, runtime get is needed to access the HW, but the question here was "has runtime_get been active ever since setting the registers previously".

If you do

runtime_get();
setup_things();
runtime_put();

runtime_get();
setup_more_things();
runtime_put();

the code is broken as the context is possibly lost between those two blocks. Unless runtime suspend/resume callbacks do a full context save and restore.

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.

Quoted 6 lines

You can do things in resume callback, but I think quite often it's simplest to just do those things when enabling the display. The device never goes to suspend if the display is enabled. And if you disable the display, the device should go to suspend, so usually enable is called after the device has been in suspend.

Quoted 6 lines

Well, the two places are pretty much the same thing in tilcdc, because enable calls pm_runtime_get(). Also with atomic the suspend/resume implementation is really straight forward. Just get the current atomic state with drm_atomic_helper_suspend() and commit it back in with drm_atomic_helper_resume(), if needed. I don't think I should implement myself something that is so well provided by pm and drm infrastructure.

The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.

The current system suspend/resume in tilcdc looks fine.

Tomi

Quoted 6 lines

The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.

The current system suspend/resume in tilcdc looks fine.

I don't undestand the same mechanism could not be used for runtime resume. Why should it matter if we configure the previous drm atomic state after system suspend or simple LCDC power down suspend?

Quoted 10 lines · Jyri Sarha

Quoted 6 lines

The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.

The current system suspend/resume in tilcdc looks fine.

I don't undestand the same mechanism could not be used for runtime resume. Why should it matter if we configure the previous drm atomic state after system suspend or simple LCDC power down suspend?

They are quite different things.

For example, you have display up and running. The user requests for system suspend. System suspend callback is called in tilcdc. That callback should turn off the displays etc.

Runtime PM suspend should not do anything like that, because it's called when the driver says the IP is not used, which means the driver has already turned off the displays etc.

So, for example, when the system suspend happens, tilcdc's system suspend callback disables the displays by calling some functions. These functions stop the HW, maybe do other cleanups, and then do pm_runtime_put(). This then causes runtime PM suspend callback to be called.

And, of course, runtime PM suspend is called anytime the driver is not using the HW, not only when system suspend happens.

So, system suspend is a high level thing, comes at any point of time from the userspace. Runtime PM is a driver internal thing, comes from the driver.

Quoted 28 lines · Tomi Valkeinen

Quoted 10 lines · Jyri Sarha

Quoted 6 lines

The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.

The current system suspend/resume in tilcdc looks fine.

I don't undestand the same mechanism could not be used for runtime resume. Why should it matter if we configure the previous drm atomic state after system suspend or simple LCDC power down suspend?

They are quite different things.

For example, you have display up and running. The user requests for system suspend. System suspend callback is called in tilcdc. That callback should turn off the displays etc.

Runtime PM suspend should not do anything like that, because it's called when the driver says the IP is not used, which means the driver has already turned off the displays etc.

So, for example, when the system suspend happens, tilcdc's system suspend callback disables the displays by calling some functions. These functions stop the HW, maybe do other cleanups, and then do pm_runtime_put(). This then causes runtime PM suspend callback to be called.

Quoted 7 lines

And, of course, runtime PM suspend is called anytime the driver is not using the HW, not only when system suspend happens.

So, system suspend is a high level thing, comes at any point of time from the userspace. Runtime PM is a driver internal thing, comes from the driver.

I am aware of the difference, but in theory I thought it should work, because the situation is pretty much the same. We need restore the state that was in LCDC when it was shut down in disable and restore the state right before we are turning it back on, all the runtime_get() and puts would serialize nicely.

But now after giving it a bit more thought, the drm locking prevents this from working. Who ever is enabling the display, is already holding some modeset locks and prevents drm_atomic_helper_resume() from taking the drm_modeset_lock_all().

Actually following your suggestion appears to be really straight forward. Simply get rid of mode_set_nofb() callback and call the same function in enable just before turning the raster on.

Failed tilcdc_crtc_create() error handling was broken, this patch should fix it.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 +++++++-----
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 11 ++++-------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  2 +-
 3 files changed, 12 insertions(+), 13 deletions(-)

The LCDC revision 2 documentation also mentions the mandatory palette for true color modes. Even if the rev 2 LCDC appears to work just fine without the palette being loaded loading it helps in testing the feature.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 85 +++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 44 deletions(-)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +33 -30

index 0f89422..3bdab77 100644
@@ -28,8 +28,8 @@
 #include "tilcdc_regs.h"
 
 #define TILCDC_VBLANK_SAFETY_THRESHOLD_US      1000
-#define TILCDC_REV1_PALETTE_SIZE               32
-#define TILCDC_REV1_PALETTE_FIRST_ENTRY                0x4000
+#define TILCDC_PALETTE_SIZE                    32
+#define TILCDC_PALETTE_FIRST_ENTRY             0x4000
 
 struct tilcdc_crtc {
        struct drm_crtc base;

@@ -62,7 +62,7 @@ struct tilcdc_crtc {
        struct work_struct recover_work;
 
        dma_addr_t palette_dma_handle;
-       void *palette_base;
+       u16 *palette_base;
        struct completion palette_loaded;
 };
 #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)

@@ -114,23 +114,17 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
 }
 
 /*
- * The driver currently only supports the RGB565 format for revision 1. For
- * 16 bits-per-pixel the palette block is bypassed, but the first 32 bytes of
- * the framebuffer are still considered palette. The first 16-bit entry must
- * be 0x4000 while all other entries must be zeroed.
+ * The driver currently only supports only true color formats. For
+ * true color the palette block is bypassed, but a 32 byte palette
+ * should still be loaded. The first 16-bit entry must be 0x4000 while
+ * all other entries must be zeroed.
  */
 static void tilcdc_crtc_load_palette(struct drm_crtc *crtc)
 {
        u32 dma_fb_base, dma_fb_ceiling, raster_ctl;
-       struct tilcdc_crtc *tilcdc_crtc;
-       struct drm_device *dev;
-       u16 *first_entry;
-
-       dev = crtc->dev;
-       tilcdc_crtc = to_tilcdc_crtc(crtc);
-       first_entry = tilcdc_crtc->palette_base;
-
-       *first_entry = TILCDC_REV1_PALETTE_FIRST_ENTRY;
+       struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+       struct drm_device *dev = crtc->dev;
+       struct tilcdc_drm_private *priv = dev->dev_private;
 
        dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG);
        dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG);

@@ -140,23 +134,34 @@ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc)
        tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG,
                     tilcdc_crtc->palette_dma_handle);
        tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG,
-                    (u32)tilcdc_crtc->palette_dma_handle
-                               + TILCDC_REV1_PALETTE_SIZE - 1);
+                    (u32) tilcdc_crtc->palette_dma_handle +
+                    TILCDC_PALETTE_SIZE - 1);
 
-       /* Load it. */
-       tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
-                    LCDC_PALETTE_LOAD_MODE(DATA_ONLY));
-       tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
-                  LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY));
+       /* Set dma load mode for palette loading only. */
+       tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG,
+                         LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY),
+                         LCDC_PALETTE_LOAD_MODE_MASK);
+
+       /* Enable DMA Palette Loaded Interrupt */
+       if (priv->rev == 1)
+               tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
+       else
+               tilcdc_write(dev, LCDC_INT_ENABLE_SET_REG, LCDC_V2_PL_INT_ENA);
 
-       /* Enable the LCDC and wait for palette to be loaded. */
-       tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
+       /* Enable LCDC DMA and wait for palette to be loaded. */
+       tilcdc_clear_irqstatus(dev, 0xffffffff);
        tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
 
        wait_for_completion(&tilcdc_crtc->palette_loaded);
 
-       /* Restore the registers. */
+       /* Disable LCDC DMA and DMA Palette Loaded Interrupt. */
        tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+       if (priv->rev == 1)
+               tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
+       else
+               tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG, LCDC_V2_PL_INT_ENA);
+
+       /* Restore the registers. */
        tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base);
        tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling);
        tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl);

@@ -217,7 +222,6 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 {
        struct drm_device *dev = crtc->dev;
        struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
-       struct tilcdc_drm_private *priv = dev->dev_private;
 
        WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
        mutex_lock(&tilcdc_crtc->enable_lock);

@@ -230,7 +234,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 
        reset(crtc);
 
-       if (priv->rev == 1 && !completion_done(&tilcdc_crtc->palette_loaded))
+       if (!completion_done(&tilcdc_crtc->palette_loaded))
                tilcdc_crtc_load_palette(crtc);
 
        tilcdc_crtc_enable_irqs(dev);

@@ -280,8 +284,7 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
         * LCDC will not retain the palette when reset. Make sure it gets
         * reloaded on tilcdc_crtc_enable().
         */
-       if (priv->rev == 1)
-               reinit_completion(&tilcdc_crtc->palette_loaded);
+       reinit_completion(&tilcdc_crtc->palette_loaded);
 
        drm_crtc_vblank_off(crtc);
 

@@ -916,13 +919,8 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
                dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underflow",
                                    __func__, stat);
 

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +6 -8

        if (stat & LCDC_SYNC_LOST) {
                dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",

@@ -968,15 +966,14 @@ int tilcdc_crtc_create(struct drm_device *dev)
                return -ENOMEM;
        }
 
-       if (priv->rev == 1) {
-               init_completion(&tilcdc_crtc->palette_loaded);
-               tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev,
-                                       TILCDC_REV1_PALETTE_SIZE,
+       init_completion(&tilcdc_crtc->palette_loaded);
+       tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev,
+                                       TILCDC_PALETTE_SIZE,
                                        &tilcdc_crtc->palette_dma_handle,
                                        GFP_KERNEL | __GFP_ZERO);
-               if (!tilcdc_crtc->palette_base)
-                       return -ENOMEM;
-       }
+       if (!tilcdc_crtc->palette_base)
+               return -ENOMEM;
+       *tilcdc_crtc->palette_base = TILCDC_PALETTE_FIRST_ENTRY;
 
        crtc = &tilcdc_crtc->base;
 
--

1.9.1

The LCDC revision 2 documentation also mentions the mandatory palette for true color modes. Even if the rev 2 LCDC appears to work just fine without the palette being loaded loading it helps in testing the feature.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 85 +++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 44 deletions(-)

Quoted 5 lines · Jyri Sarha

The LCDC revision 2 documentation also mentions the mandatory palette for true color modes. Even if the rev 2 LCDC appears to work just fine without the palette being loaded loading it helps in testing the feature.

Quoted 4 lines

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 85 +++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 44 deletions(-)

Squash with patch 2?

Tomi

Add timeout wait for palette load-ind to complete. We do not want to hang forever if palette loaded interrupt does not arrive for some reason.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

We should wait for the last frame to complete before shutting things down also on LCDC rev 1.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 34 +++++++++++++++++-----------------
 drivers/gpu/drm/tilcdc/tilcdc_regs.h |  1 +
 2 files changed, 18 insertions(+), 17 deletions(-)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +12 -12

index 1a1ff8d..f251546 100644
@@ -183,7 +183,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
 
        if (priv->rev == 1) {
                tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
-                       LCDC_V1_SYNC_LOST_ENA |
+                       LCDC_V1_SYNC_LOST_ENA | LCDC_V1_FRAME_DONE_ENA |
                        LCDC_V1_UNDERFLOW_INT_ENA);
                tilcdc_set(dev, LCDC_DMA_CTRL_REG,
                        LCDC_V1_END_OF_FRAME_INT_ENA);

@@ -201,7 +201,8 @@ static void tilcdc_crtc_disable_irqs(struct drm_device *dev)
 
        /* disable irqs that we might have enabled: */
        if (priv->rev == 1) {
-               tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_SYNC_LOST_ENA |
+               tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
+                       LCDC_V1_SYNC_LOST_ENA | LCDC_V1_FRAME_DONE_ENA |
                        LCDC_V1_UNDERFLOW_INT_ENA | LCDC_V1_PL_INT_ENA);
                tilcdc_clear(dev, LCDC_DMA_CTRL_REG,
                        LCDC_V1_END_OF_FRAME_INT_ENA);

@@ -261,6 +262,7 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
        struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
        struct drm_device *dev = crtc->dev;
        struct tilcdc_drm_private *priv = dev->dev_private;
+       int ret;
 
        mutex_lock(&tilcdc_crtc->enable_lock);
        if (shutdown)

@@ -273,17 +275,15 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
        tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
 
        /*
-        * if necessary wait for framedone irq which will still come
-        * before putting things to sleep..
+        * Wait for framedone irq which will still come before putting
+        * things to sleep..
         */
-       if (priv->rev == 2) {
-               int ret = wait_event_timeout(tilcdc_crtc->frame_done_wq,
-                                            tilcdc_crtc->frame_done,
-                                            msecs_to_jiffies(500));
-               if (ret == 0)
-                       dev_err(dev->dev, "%s: timeout waiting for framedone\n",
-                               __func__);
-       }
+       ret = wait_event_timeout(tilcdc_crtc->frame_done_wq,
+                                tilcdc_crtc->frame_done,
+                                msecs_to_jiffies(500));
+       if (ret == 0)
+               dev_err(dev->dev, "%s: timeout waiting for framedone\n",
+                       __func__);
 
        drm_crtc_vblank_off(crtc);
 

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +1 -5

+
        /* For revision 2 only */
        if (priv->rev == 2) {
-               if (stat & LCDC_FRAME_DONE) {
-                       tilcdc_crtc->frame_done = true;
-                       wake_up(&tilcdc_crtc->frame_done_wq);
-               }
-
                /* Indicate to LCDC that the interrupt service routine has
                 * completed, see 13.3.6.1.6 in AM335x TRM.
                 */

drivers/gpu/drm/tilcdc/tilcdc_regs.h +0 -0

index 56dbfbd..4e6975a 100644

drivers/gpu/drm/tilcdc/tilcdc_regs.h +0 -1

 #define LCDC_MONOCHROME_MODE                     BIT(1)
 #define LCDC_RASTER_ENABLE                       BIT(0)
 #define LCDC_TFT_ALT_ENABLE                      BIT(23)
--

1.9.1