Re: [PATCH v2 3/4] spi: spi-geni-qcom: Don't try to set CS if an xfer is pending
From: Doug Anderson
Date: Thu Dec 17 2020 - 16:22:26 EST
Hi,
On Wed, Dec 16, 2020 at 8:25 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> 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?
I'll add this to the commit message:
spi_transfer_one_message()
->transfer_one() AKA spi_geni_transfer_one()
setup_fifo_xfer()
mas->cur_xfer = non-NULL
spi_transfer_wait() => TIMES OUT
msg->status != -EINPROGRESS => goto out
if (ret != 0 ...)
spi_set_cs()
->set_cs AKA spi_geni_set_cs()
# mas->cur_xfer is non-NULL
Specifically the place where cur_xfer is usually made NULL is in the
interrupt handler. If that doesn't run then it will be non-NULL.
> > + dev_err(mas->dev, "Can't set CS when prev xfter running\n");
>
> xfer? or xfter?
Will fix.
-Doug