Re: [PATCH v2 3/4] spi: spi-geni-qcom: Don't try to set CS if an xfer is pending

[Stephen Boyd]

Quoting Douglas Anderson (2020-12-16 14:41:51)
> If we get a timeout sending then this happens:
> * spi_transfer_wait() will get a timeout.
> * We'll set the chip select
> * We'll call handle_err() => handle_fifo_timeout().
> 
> Unfortunately that won't work so well on geni.  If we got a timeout
> transferring then it's likely that our interrupt handler is blocked,
> but we need that same interrupt handler to adjust the chip select.
> Trying to set the chip select doesn't crash us but ends up confusing
> our state machine and leads to messages like:
>   Premature done. rx_rem = 32 bpw8
> 
> Let's just drop the chip select request in this case.  Sure, we might
> leave the chip select in the wrong state but it's likely it was going
> to fail anyway and this avoids getting the driver even more confused
> about what it's doing.
> 
> The SPI core in general assumes that setting chip select is a simple
> operation that doesn't fail.  Yet another reason to just reconfigure
> the chip select line as GPIOs.

Indeed.

> 
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> 
> Changes in v2:
> - ("spi: spi-geni-qcom: Don't try to set CS if an xfer is pending") new for v2.
> 
>  drivers/spi/spi-geni-qcom.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index d988463e606f..0e4fa52ac017 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -204,9 +204,14 @@ static void spi_geni_set_cs(struct spi_device *slv, bool set_flag)
>                 goto exit;
>         }
>  
> -       mas->cs_flag = set_flag;
> -
>         spin_lock_irq(&mas->lock);
> +       if (mas->cur_xfer) {

How is it possible that cs change happens when cur_xfer is non-NULL?

> +               dev_err(mas->dev, "Can't set CS when prev xfter running\n");

xfer? or xfter?

> +               spin_unlock_irq(&mas->lock);
> +               goto exit;
> +       }
> +
> +       mas->cs_flag = set_flag;
>         reinit_completion(&mas->cs_done);