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

[Herve Codina <herve.codina@bootlin.com>]

Hi Mike,

On Tue, 24 Jun 2025 13:45:15 +0200
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> 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>
> ---
> 
>  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
...

> @@ -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
> +		 * after the reset.

I think I have missed a verb in the last sentence when I previously wrote
the comment.
  "The interrupt will be re-enabled after the reset."
                         ^^^^^^^^^^
Can you fix it ?

Other than that,

Reviewed-by: Herve Codina <herve.codina@bootlin.com>

Thanks,
Hervé