| Message ID | 20250108101907.410456-4-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for errors recovery in the TI SN65DSI83 bridge driver | expand |
Hi Herve, Am Mittwoch, 8. Januar 2025, 11:19:02 CET schrieb Herve Codina: > In some cases observed during ESD tests, the TI SN65DSI83 cannot recover > from errors by itself. A full restart of the bridge is needed in those > cases to have the bridge output LVDS signals again. > > Also, during tests, cases were observed where reading the status of the > bridge was not even possible. Indeed, in those cases, the bridge stops > to acknowledge I2C transactions. Only a full reset of the bridge (power > off/on) brings back the bridge to a functional state. > > The TI SN65DSI83 has some error detection capabilities. Introduce an > error recovery mechanism based on this detection. > > The errors detected are signaled through an interrupt. On system where > this interrupt is not available, the driver uses a polling monitoring > fallback to check for errors. When an error is present or when reading > the bridge status leads to an I2C failure, the recovery process is > launched. > > Restarting the bridge needs to redo the initialization sequence. This > initialization sequence has to be done with the DSI data lanes driven in > LP11 state. In order to do that, the recovery process resets the whole > output path (i.e the path from the encoder to the connector) where the > bridge is located. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 ++++++++++++++++++++++++++ > 1 file changed, 147 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index e6264514bb3f..74bc05647436 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -35,9 +35,12 @@ > #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_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */ Shouldn't this include be added to include/drm/drm_modeset_lock.h instead? > #include <drm/drm_mipi_dsi.h> > #include <drm/drm_of.h> > #include <drm/drm_panel.h> > @@ -147,6 +150,9 @@ struct sn65dsi83 { > struct regulator *vcc; > bool lvds_dual_link; > bool lvds_dual_link_even_odd_swap; > + bool use_irq; > + struct delayed_work monitor_work; > + struct work_struct reset_work; Can you please rebase? You are missing commit d2b8c6d549570 ("drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties") > }; > > static const struct regmap_range sn65dsi83_readable_ranges[] = { > @@ -328,6 +334,111 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx) > return dsi_div - 1; > } > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83) > +{ > + struct drm_atomic_state *state = ERR_PTR(-EINVAL); > + struct drm_device *dev = sn65dsi83->bridge.dev; > + struct drm_connector_state *connector_state; > + struct drm_modeset_acquire_ctx ctx; > + struct drm_connector *connector; > + 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 SN64DSI83 > + * bridge. > + * > + * Keep the lock during the whole operation to be atomic. > + */ > + > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err); > + > + state = drm_atomic_helper_duplicate_state(dev, &ctx); > + if (IS_ERR(state)) { > + err = PTR_ERR(state); > + goto unlock; > + } > + > + state->acquire_ctx = &ctx; > + > + connector = drm_atomic_get_old_connector_for_encoder(state, > + sn65dsi83->bridge.encoder); > + if (!connector) { > + err = -EINVAL; > + goto unlock; > + } > + > + connector_state = drm_atomic_get_connector_state(state, connector); > + if (IS_ERR(connector_state)) { > + err = PTR_ERR(connector_state); > + goto unlock; > + } > + > + err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx); > + if (err < 0) > + goto unlock; > + > +unlock: > + DRM_MODESET_LOCK_ALL_END(dev, ctx, err); > + if (!IS_ERR(state)) > + drm_atomic_state_put(state); > + return err; > +} > + > +static void sn65dsi83_reset_work(struct work_struct *ws) > +{ > + struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work); > + int ret; > + > + dev_warn(ctx->dev, "reset the pipe\n"); > + > + /* Reset the pipe */ > + ret = sn65dsi83_reset_pipe(ctx); > + if (ret) { > + dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret)); > + return; > + } > +} > + > +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) > + schedule_work(&ctx->reset_work); Shouldn't you clear the error bits as well? Best regards, Alexander > +} > + > +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_bridge_state *old_bridge_state) > { > @@ -516,6 +627,15 @@ 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->use_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, > @@ -524,6 +644,15 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge); > int ret; > > + if (ctx->use_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); > @@ -681,6 +810,14 @@ 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); > @@ -698,6 +835,8 @@ static int sn65dsi83_probe(struct i2c_client *client) > return ret; > > 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) > @@ -722,6 +861,14 @@ 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) { > + ret = devm_request_threaded_irq(ctx->dev, client->irq, NULL, sn65dsi83_irq, > + IRQF_ONESHOT, dev_name(ctx->dev), ctx); > + if (ret) > + return dev_err_probe(dev, ret, "failed to request irq\n"); > + ctx->use_irq = true; > + } > + > dev_set_drvdata(dev, ctx); > i2c_set_clientdata(client, ctx); > >
Hi Alexander, On Wed, 08 Jan 2025 11:54:49 +0100 Alexander Stein <alexander.stein@ew.tq-group.com> wrote: [...] > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_bridge.h> > > +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */ > > Shouldn't this include be added to include/drm/drm_modeset_lock.h instead? Yes indeed. I will change that in the next iteration. > > > #include <drm/drm_mipi_dsi.h> > > #include <drm/drm_of.h> > > #include <drm/drm_panel.h> > > @@ -147,6 +150,9 @@ struct sn65dsi83 { > > struct regulator *vcc; > > bool lvds_dual_link; > > bool lvds_dual_link_even_odd_swap; > > + bool use_irq; > > + struct delayed_work monitor_work; > > + struct work_struct reset_work; > > Can you please rebase? You are missing commit d2b8c6d549570 > ("drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties") Sure, I will rebase. [...] > > +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) > > + schedule_work(&ctx->reset_work); > > Shouldn't you clear the error bits as well? Thanks for pointing that. I can clear the error bit but further more, I probably need to simply disable the interrupt. In some cases, we observed i2c access failure. In that cases clearing error bits is simply not possible. To avoid some possible interrupt storms (the chip detect a failure, set its interrupt line but could be not accessible anymore), the best thing to do is to disable the interrupt line here, let the reset work to do its job performing a full reset of the device and re-enabling the interrupt line when needed, probably in sn65dsi83_atomic_enable(). What do you think about that? Best regards, Hervé
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index e6264514bb3f..74bc05647436 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -35,9 +35,12 @@ #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_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */ #include <drm/drm_mipi_dsi.h> #include <drm/drm_of.h> #include <drm/drm_panel.h> @@ -147,6 +150,9 @@ struct sn65dsi83 { struct regulator *vcc; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap; + bool use_irq; + struct delayed_work monitor_work; + struct work_struct reset_work; }; static const struct regmap_range sn65dsi83_readable_ranges[] = { @@ -328,6 +334,111 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx) return dsi_div - 1; } +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83) +{ + struct drm_atomic_state *state = ERR_PTR(-EINVAL); + struct drm_device *dev = sn65dsi83->bridge.dev; + struct drm_connector_state *connector_state; + struct drm_modeset_acquire_ctx ctx; + struct drm_connector *connector; + 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 SN64DSI83 + * bridge. + * + * Keep the lock during the whole operation to be atomic. + */ + + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err); + + state = drm_atomic_helper_duplicate_state(dev, &ctx); + if (IS_ERR(state)) { + err = PTR_ERR(state); + goto unlock; + } + + state->acquire_ctx = &ctx; + + connector = drm_atomic_get_old_connector_for_encoder(state, + sn65dsi83->bridge.encoder); + if (!connector) { + err = -EINVAL; + goto unlock; + } + + connector_state = drm_atomic_get_connector_state(state, connector); + if (IS_ERR(connector_state)) { + err = PTR_ERR(connector_state); + goto unlock; + } + + err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx); + if (err < 0) + goto unlock; + +unlock: + DRM_MODESET_LOCK_ALL_END(dev, ctx, err); + if (!IS_ERR(state)) + drm_atomic_state_put(state); + return err; +} + +static void sn65dsi83_reset_work(struct work_struct *ws) +{ + struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work); + int ret; + + dev_warn(ctx->dev, "reset the pipe\n"); + + /* Reset the pipe */ + ret = sn65dsi83_reset_pipe(ctx); + if (ret) { + dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret)); + return; + } +} + +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) + 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_bridge_state *old_bridge_state) { @@ -516,6 +627,15 @@ 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->use_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, @@ -524,6 +644,15 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge); int ret; + if (ctx->use_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); @@ -681,6 +810,14 @@ 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); @@ -698,6 +835,8 @@ static int sn65dsi83_probe(struct i2c_client *client) return ret; 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) @@ -722,6 +861,14 @@ 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) { + ret = devm_request_threaded_irq(ctx->dev, client->irq, NULL, sn65dsi83_irq, + IRQF_ONESHOT, dev_name(ctx->dev), ctx); + if (ret) + return dev_err_probe(dev, ret, "failed to request irq\n"); + ctx->use_irq = true; + } + dev_set_drvdata(dev, ctx); i2c_set_clientdata(client, ctx);
In some cases observed during ESD tests, the TI SN65DSI83 cannot recover from errors by itself. A full restart of the bridge is needed in those cases to have the bridge output LVDS signals again. Also, during tests, cases were observed where reading the status of the bridge was not even possible. Indeed, in those cases, the bridge stops to acknowledge I2C transactions. Only a full reset of the bridge (power off/on) brings back the bridge to a functional state. The TI SN65DSI83 has some error detection capabilities. Introduce an error recovery mechanism based on this detection. The errors detected are signaled through an interrupt. On system where this interrupt is not available, the driver uses a polling monitoring fallback to check for errors. When an error is present or when reading the bridge status leads to an I2C failure, the recovery process is launched. Restarting the bridge needs to redo the initialization sequence. This initialization sequence has to be done with the DSI data lanes driven in LP11 state. In order to do that, the recovery process resets the whole output path (i.e the path from the encoder to the connector) where the bridge is located. Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 ++++++++++++++++++++++++++ 1 file changed, 147 insertions(+)