Message-ID: <20251125103900.31750-1-francesco@dolcini.it>
Date: Tue, 25 Nov 2025 11:38:58 +0100
From: Francesco Dolcini <francesco@...cini.it>
To: Andrzej Hajda <andrzej.hajda@...el.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Robert Foss <rfoss@...nel.org>,
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
	Jonas Karlman <jonas@...boo.se>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>,
	Simona Vetter <simona@...ll.ch>
Cc: Francesco Dolcini <francesco.dolcini@...adex.com>,
	dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org,
	João Paulo Gonçalves <jpaulo.silvagoncalves@...il.com>,
	stable@...r.kernel.org,
	Herve Codina <herve.codina@...tlin.com>,
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	Luca Ceresoli <luca.ceresoli@...tlin.com>,
	Emanuele Ghidoli <emanuele.ghidoli@...adex.com>
Subject: [PATCH v1] Revert "drm: bridge: ti-sn65dsi83: Add error recovery mechanism"

From: Francesco Dolcini <francesco.dolcini@...adex.com>

This reverts commit ad5c6ecef27e ("drm: bridge: ti-sn65dsi83: Add error
recovery mechanism").

The reverted commit introduces a regression on Verdin AM62, and
potentially on more devices, not being able to generate a clock
that the TI SN65DSI83 PLL can lock to, with the display periodically
blinking.

Verdin AM62 SoM has a Toshiba TC358778 DPI to DSI bridge, that can be
connected to an LVDS display over a TI SN65DSI83 bridge. Before this
change despite the TI SN65DSI83 reporting with a debug print a PLL
locking error the display was working fine with no visible glitches.

The reasons for this issue was investigated without getting to a final
conclusion:

 - the DPI clock was measure and it is stable/accurate
 - the DSI clock was not possible to measure, but this setup is used
   with other display/bridges with no known issues
 - the DSI clock is configured in continuous mode
 - the actual DSI clock generated from the TC358778 is generate with a
   PLL from a 25MHz reference clock
 - it's not clear why some frequencies are working and some are not, for
   example 50000000, 68750000, 72750000, 75000000 frequencies are fine,
   while 69750000, 71100000, 72500000 are not

Given that the safest approach is to just revert the commit, till a
proper solution for error recovery that is not introducing regression
is figured out.

Reported-by: João Paulo Gonçalves <jpaulo.silvagoncalves@...il.com>
Closes: https://lore.kernel.org/all/bhkn6hley4xrol5o3ytn343h4unkwsr26p6s6ltcwexnrsjsdx@mgkdf6ztow42/
Fixes: ad5c6ecef27e ("drm: bridge: ti-sn65dsi83: Add error recovery mechanism")
Cc: stable@...r.kernel.org
Signed-off-by: Francesco Dolcini <francesco.dolcini@...adex.com>
---
Cc: Herve Codina <herve.codina@...tlin.com>
Cc: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Maxime Ripard <mripard@...nel.org>
Cc: Emanuele Ghidoli <emanuele.ghidoli@...adex.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 136 --------------------------
 1 file changed, 136 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 033c44326552..cf627aff569b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -35,12 +35,9 @@
 #include <linux/of_graph.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
-#include <linux/timer.h>
-#include <linux/workqueue.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
-#include <drm/drm_bridge_helper.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_of.h>
 #include <drm/drm_print.h>
@@ -161,9 +158,6 @@ struct sn65dsi83 {
 	bool				lvds_dual_link_even_odd_swap;
 	int				lvds_vod_swing_conf[2];
 	int				lvds_term_conf[2];
-	int				irq;
-	struct delayed_work		monitor_work;
-	struct work_struct		reset_work;
 };
 
 static const struct regmap_range sn65dsi83_readable_ranges[] = {
@@ -369,100 +363,6 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
 	return dsi_div - 1;
 }
 
-static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
-{
-	struct drm_modeset_acquire_ctx ctx;
-	int err;
-
-	/*
-	 * Reset active outputs of the related CRTC.
-	 *
-	 * This way, drm core will reconfigure each components in the CRTC
-	 * outputs path. In our case, this will force the previous component to
-	 * go back in LP11 mode and so allow the reconfiguration of SN65DSI83
-	 * bridge.
-	 *
-	 * Keep the lock during the whole operation to be atomic.
-	 */
-
-	drm_modeset_acquire_init(&ctx, 0);
-
-	dev_warn(sn65dsi83->dev, "reset the pipe\n");
-
-retry:
-	err = drm_bridge_helper_reset_crtc(&sn65dsi83->bridge, &ctx);
-	if (err == -EDEADLK) {
-		drm_modeset_backoff(&ctx);
-		goto retry;
-	}
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-
-	return 0;
-}
-
-static void sn65dsi83_reset_work(struct work_struct *ws)
-{
-	struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
-	int ret;
-
-	/* Reset the pipe */
-	ret = sn65dsi83_reset_pipe(ctx);
-	if (ret) {
-		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
-		return;
-	}
-	if (ctx->irq)
-		enable_irq(ctx->irq);
-}
-
-static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
-{
-	unsigned int irq_stat;
-	int ret;
-
-	/*
-	 * Schedule a reset in case of:
-	 *  - the bridge doesn't answer
-	 *  - the bridge signals an error
-	 */
-
-	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);
-
-		schedule_work(&ctx->reset_work);
-	}
-}
-
-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));
-}
-
-static void sn65dsi83_monitor_start(struct sn65dsi83 *ctx)
-{
-	schedule_delayed_work(&ctx->monitor_work, msecs_to_jiffies(1000));
-}
-
-static void sn65dsi83_monitor_stop(struct sn65dsi83 *ctx)
-{
-	cancel_delayed_work_sync(&ctx->monitor_work);
-}
-
 static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 					struct drm_atomic_state *state)
 {
@@ -650,15 +550,6 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
 	if (pval)
 		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
-
-	if (ctx->irq) {
-		/* Enable irq to detect errors */
-		regmap_write(ctx->regmap, REG_IRQ_GLOBAL, REG_IRQ_GLOBAL_IRQ_EN);
-		regmap_write(ctx->regmap, REG_IRQ_EN, 0xff);
-	} else {
-		/* Use the polling task */
-		sn65dsi83_monitor_start(ctx);
-	}
 }
 
 static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
@@ -667,15 +558,6 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
 	int ret;
 
-	if (ctx->irq) {
-		/* Disable irq */
-		regmap_write(ctx->regmap, REG_IRQ_EN, 0x0);
-		regmap_write(ctx->regmap, REG_IRQ_GLOBAL, 0x0);
-	} else {
-		/* Stop the polling task */
-		sn65dsi83_monitor_stop(ctx);
-	}
-
 	/* Put the chip in reset, pull EN line low, and assure 10ms reset low timing. */
 	gpiod_set_value_cansleep(ctx->enable_gpio, 0);
 	usleep_range(10000, 11000);
@@ -925,14 +807,6 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
 	return 0;
 }
 
-static irqreturn_t sn65dsi83_irq(int irq, void *data)
-{
-	struct sn65dsi83 *ctx = data;
-
-	sn65dsi83_handle_errors(ctx);
-	return IRQ_HANDLED;
-}
-
 static int sn65dsi83_probe(struct i2c_client *client)
 {
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
@@ -946,8 +820,6 @@ static int sn65dsi83_probe(struct i2c_client *client)
 		return PTR_ERR(ctx);
 
 	ctx->dev = dev;
-	INIT_WORK(&ctx->reset_work, sn65dsi83_reset_work);
-	INIT_DELAYED_WORK(&ctx->monitor_work, sn65dsi83_monitor_work);
 
 	if (dev->of_node) {
 		model = (enum sn65dsi83_model)(uintptr_t)
@@ -972,14 +844,6 @@ static int sn65dsi83_probe(struct i2c_client *client)
 	if (IS_ERR(ctx->regmap))
 		return dev_err_probe(dev, PTR_ERR(ctx->regmap), "failed to get regmap\n");
 
-	if (client->irq) {
-		ctx->irq = client->irq;
-		ret = devm_request_threaded_irq(ctx->dev, ctx->irq, NULL, sn65dsi83_irq,
-						IRQF_ONESHOT, dev_name(ctx->dev), ctx);
-		if (ret)
-			return dev_err_probe(dev, ret, "failed to request irq\n");
-	}
-
 	dev_set_drvdata(dev, ctx);
 	i2c_set_clientdata(client, ctx);
 
-- 
2.47.3

Powered by blists - more mailing lists