[PATCH v2] drm/bridge: ti-sn65dsi83: Improve error reporting and handling

[PATCH v2] drm/bridge: ti-sn65dsi83: Improve error reporting and handling

[Mike Looijmans]

The datasheet advises to wait 5ms after starting the video stream before
resetting the error registers. The driver only waits 1ms. Change the
sequence to match the datasheet:
- Turn on the DSI
- Wait 5ms
- Write 0xFF to CSR 0xE5 to clear the error registers

Don't read the error register (which may fail), just write 0xff as the
datasheet suggests.

The driver creates a timer or IRQ handler that reads the error register,
which implements the "wait some time and read the register" part.

When using a timer to poll the status register, the timer did not stop
when the error handler triggers a reset. This has been observed to cause
a series of multiple resets. Let handle_errors return a bool indicating
whether all is fine, and only extend the time when it returns true. That
also allows the IRQ disable call to move to the interrupt routine.

When the error handler does trigger, log a message that explains the
reset cause.

Fixes: ad5c6ecef27e ("drm: bridge: ti-sn65dsi83: Add error recovery mechanism")
Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
Reviewed-by: Herve Codina <herve.codina@bootlin.com>

---

Changes in v2:
Added missing verb "re-enabled" in IRQ comment (as suggested by HC)

 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 50 +++++++++++++++------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 033c44326552..8b21d8d689e4 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -417,7 +417,7 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
 		enable_irq(ctx->irq);
 }
 
-static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
+static bool sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
 {
 	unsigned int irq_stat;
 	int ret;
@@ -430,17 +430,20 @@ static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
 
 	ret = regmap_read(ctx->regmap, REG_IRQ_STAT, &irq_stat);
 	if (ret || irq_stat) {
-		/*
-		 * IRQ acknowledged is not always possible (the bridge can be in
-		 * a state where it doesn't answer anymore). To prevent an
-		 * interrupt storm, disable interrupt. The interrupt will be
-		 * after the reset.
-		 */
-		if (ctx->irq)
-			disable_irq_nosync(ctx->irq);
+		if (ret) {
+			dev_err(ctx->dev, "Communication failure\n");
+		} else {
+			dev_err(ctx->dev, "Error status: 0x%02x\n", irq_stat);
+			/* Clear errors if the chip was still responding */
+			regmap_write(ctx->regmap, REG_IRQ_STAT, irq_stat);
+		}
 
 		schedule_work(&ctx->reset_work);
+
+		return false;
 	}
+
+	return true;
 }
 
 static void sn65dsi83_monitor_work(struct work_struct *work)
@@ -448,9 +451,8 @@ static void sn65dsi83_monitor_work(struct work_struct *work)
 	struct sn65dsi83 *ctx = container_of(to_delayed_work(work),
 					     struct sn65dsi83, monitor_work);
 
-	sn65dsi83_handle_errors(ctx);
-
-	schedule_delayed_work(&ctx->monitor_work, msecs_to_jiffies(1000));
+	if (sn65dsi83_handle_errors(ctx))
+		schedule_delayed_work(&ctx->monitor_work, msecs_to_jiffies(1000));
 }
 
 static void sn65dsi83_monitor_start(struct sn65dsi83 *ctx)
@@ -639,18 +641,13 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 				    struct drm_atomic_state *state)
 {
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
-	unsigned int pval;
 
+	/* Wait 5 ms after starting DSI stream */
+	usleep_range(5000, 5500);
 	/* Clear all errors that got asserted during initialization. */
-	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
-	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
-
-	/* Wait for 1ms and check for errors in status register */
-	usleep_range(1000, 1100);
-	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
-	if (pval)
-		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
+	regmap_write(ctx->regmap, REG_IRQ_STAT, 0xff);
 
+	/* Start checking for errors in status register */
 	if (ctx->irq) {
 		/* Enable irq to detect errors */
 		regmap_write(ctx->regmap, REG_IRQ_GLOBAL, REG_IRQ_GLOBAL_IRQ_EN);
@@ -929,7 +926,16 @@ static irqreturn_t sn65dsi83_irq(int irq, void *data)
 {
 	struct sn65dsi83 *ctx = data;
 
-	sn65dsi83_handle_errors(ctx);
+	if (!sn65dsi83_handle_errors(ctx)) {
+		/*
+		 * IRQ acknowledged is not always possible (the bridge can be in
+		 * a state where it doesn't answer anymore). To prevent an
+		 * interrupt storm, disable interrupt. The interrupt will be
+		 * re-enabled after the reset.
+		 */
+		disable_irq_nosync(ctx->irq);
+	}
+
 	return IRQ_HANDLED;
 }
 
-- 
2.43.0

base-commit: 78f4e737a53e1163ded2687a922fce138aee73f5
branch: linux-master-sn65dsi83-errorhandling

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail