Hi, On Thu, Jun 18, 2020 at 11:05 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > Quoting Douglas Anderson (2020-06-18 08:06:26) > > @@ -126,20 +120,23 @@ static void handle_fifo_timeout(struct spi_master *spi, > > struct geni_se *se = &mas->se; > > > > spin_lock_irq(&mas->lock); > > - reinit_completion(&mas->xfer_done); > > - mas->cur_mcmd = CMD_CANCEL; > > - geni_se_cancel_m_cmd(se); > > + reinit_completion(&mas->cancel_done); > > writel(0, se->base + SE_GENI_TX_WATERMARK_REG); > > + mas->cur_xfer = NULL; > > BTW, is this necessary? It's subtlely placed here without a comment why. I believe so. Now that we don't have the "cur_mcmd" we rely on cur_xfer being NULL to tell the difference between a "done" for chip select vs. a "done" for transfer. * When we start a transfer we set "cur_xfer" to a non-NULL pointer. When the transfer finishes we set it to NULL again. * When we start a chip select transfer we _don't_ explicitly set it to NULL because it should already be NULL. * When we are aborting a transfer we need to NULL so we can handle the chip select that will come next. I suppose it's possible that we could get by without without NULLing it because I believe when the "abort" IRQ finally fires then it will include a "DONE" and that would presumably NULL it out. ...but I guess if both the cancel and abort timed out and no IRQ ever fired then nothing would have NULLed it and the next chip select would be confused. Prior to getting rid of "cur_mcmd" this all wasn't needed because "cur_xfer" was only ever looked at if "cur_mcmd" was set to "CMD_XFER". One part of my change that is technically not related to the removal of "cur_mcmd" is the part where I do "mas->tx_rem_bytes = mas->rx_rem_bytes = 0;". I can split that as a separate change if you want but it seemed fine to just clean up this extra bit of state here. -Doug
