Unverified Commit d5fcc710 authored by Mark Brown's avatar Mark Brown
Browse files

Merge series "spi: spi-geni-qcom: Fixes / perf improvements" from Douglas... 

Merge series "spi: spi-geni-qcom: Fixes / perf improvements" from Douglas Anderson <dianders@chromium.org>:

This patch series is a new version of the previous patch posted:
  [PATCH v2] spi: spi-geni-qcom: Speculative fix of "nobody cared" about interrupt
  https://lore.kernel.org/r/20200317133653.v2.1.I752ebdcfd5e8bf0de06d66e767b8974932b3620e@changeid

At this point I've done enough tracing to know that there was a real
race in the old code (not just weakly ordered memory problems) and
that should be fixed with the locking patches.

While looking at this driver, I also noticed we weren't properly
noting error interrupts and also weren't actually using our FIFO
effectively, so I fixed those.

The last patch in the series addresses review feedback about dislike
for the "cur_mcmd" state variable.  It also could possibly make
"abort" work ever-so-slightly more reliably.

Changes in v4:
- Drop 'controller' in comment.
- Use Stephen's diagram to explain the race better.

Changes in v3:
- ("spi: spi-geni-qcom: No need for irqsave variant...") new for v3
- Split out some lock cleanup to previous patch.
- Don't need to read IRQ status register inside spinlock.
- Don't check for state CMD_NONE; later patch is removing state var.
- Don't hold the lock for all of setup_fifo_xfer().
- Comment about why it's safe to Ack interrupts at the end.
- Subject/desc changed since race is definitely there.
- ("spi: spi-geni-qcom: Check for error IRQs") new in v3.
- ("spi: spi-geni-qcom: Actually use our FIFO") new in v3.
- ("spi: spi-geni-qcom: Don't keep a local state variable") new in v3.

Changes in v2:
- Detect true spurious interrupt.
- Still return IRQ_NONE for state machine mismatch, but print warn.

Douglas Anderson (5):
  spi: spi-geni-qcom: No need for irqsave variant of spinlock calls
  spi: spi-geni-qcom: Mo' betta locking
  spi: spi-geni-qcom: Check for error IRQs
  spi: spi-geni-qcom: Actually use our FIFO
  spi: spi-geni-qcom: Don't keep a local state variable

 drivers/spi/spi-geni-qcom.c | 120 ++++++++++++++++++++++++------------
 1 file changed, 81 insertions(+), 39 deletions(-)

--
2.27.0.290.gba653c62da-goog
parents 0ec544ce 7ba9bdcb

drivers/spi/spi-geni-qcom.c

+74 −31
Original line number Diff line number Diff line
@@ -63,13 +63,6 @@ 
#define TIMESTAMP_AFTER		BIT(3)

#define POST_CMD_DELAY		BIT(4)



enum spi_m_cmd_opcode {

	CMD_NONE,

	CMD_XFER,

	CMD_CS,

	CMD_CANCEL,

};



struct spi_geni_master {

	struct geni_se se;

	struct device *dev;
@@ -81,10 +74,11 @@ struct spi_geni_master { 
	unsigned int tx_rem_bytes;

	unsigned int rx_rem_bytes;

	const struct spi_transfer *cur_xfer;

	struct completion xfer_done;

	struct completion cs_done;

	struct completion cancel_done;

	struct completion abort_done;

	unsigned int oversampling;

	spinlock_t lock;

	enum spi_m_cmd_opcode cur_mcmd;

	int irq;

};
@@ -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;

	mas->tx_rem_bytes = mas->rx_rem_bytes = 0;

	geni_se_cancel_m_cmd(se);

	spin_unlock_irq(&mas->lock);

	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);



	time_left = wait_for_completion_timeout(&mas->cancel_done, HZ);

	if (time_left)

		return;



	spin_lock_irq(&mas->lock);

	reinit_completion(&mas->xfer_done);

	reinit_completion(&mas->abort_done);

	geni_se_abort_m_cmd(se);

	spin_unlock_irq(&mas->lock);

	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);



	time_left = wait_for_completion_timeout(&mas->abort_done, HZ);

	if (!time_left)

		dev_err(mas->dev, "Failed to cancel/abort m_cmd\n");

}
@@ -151,18 +148,19 @@ static void spi_geni_set_cs(struct spi_device *slv, bool set_flag) 
	struct geni_se *se = &mas->se;

	unsigned long time_left;



	reinit_completion(&mas->xfer_done);

	pm_runtime_get_sync(mas->dev);

	if (!(slv->mode & SPI_CS_HIGH))

		set_flag = !set_flag;



	mas->cur_mcmd = CMD_CS;

	spin_lock_irq(&mas->lock);

	reinit_completion(&mas->cs_done);

	if (set_flag)

		geni_se_setup_m_cmd(se, SPI_CS_ASSERT, 0);

	else

		geni_se_setup_m_cmd(se, SPI_CS_DEASSERT, 0);

	spin_unlock_irq(&mas->lock);



	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);

	time_left = wait_for_completion_timeout(&mas->cs_done, HZ);

	if (!time_left)

		handle_fifo_timeout(spi, NULL);
@@ -283,7 +281,7 @@ static int spi_geni_init(struct spi_geni_master *mas) 
	 * Hardware programming guide suggests to configure

	 * RX FIFO RFR level to fifo_depth-2.

	 */

	geni_se_init(se, 0x0, mas->tx_fifo_depth - 2);

	geni_se_init(se, mas->tx_fifo_depth / 2, mas->tx_fifo_depth - 2);

	/* Transmit an entire FIFO worth of data per IRQ */

	mas->tx_wm = 1;

	ver = geni_se_get_qup_hw_version(se);
@@ -307,6 +305,21 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, 
	u32 spi_tx_cfg, len;

	struct geni_se *se = &mas->se;



	/*

	 * Ensure that our interrupt handler isn't still running from some

	 * prior command before we start messing with the hardware behind

	 * its back.  We don't need to _keep_ the lock here since we're only

	 * worried about racing with out interrupt handler.  The SPI core

	 * already handles making sure that we're not trying to do two

	 * transfers at once or setting a chip select and doing a transfer

	 * concurrently.

	 *

	 * NOTE: we actually _can't_ hold the lock here because possibly we

	 * might call clk_set_rate() which needs to be able to sleep.

	 */

	spin_lock_irq(&mas->lock);

	spin_unlock_irq(&mas->lock);



	spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG);

	if (xfer->bits_per_word != mas->cur_bits_per_word) {

		spi_setup_word_len(mas, mode, xfer->bits_per_word);
@@ -366,7 +379,12 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, 
		mas->rx_rem_bytes = xfer->len;

	}

	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);

	mas->cur_mcmd = CMD_XFER;



	/*

	 * Lock around right before we start the transfer since our

	 * interrupt could come in at any time now.

	 */

	spin_lock_irq(&mas->lock);

	geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);



	/*
@@ -376,6 +394,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, 
	 */

	if (m_cmd & SPI_TX_ONLY)

		writel(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);

	spin_unlock_irq(&mas->lock);

}



static int spi_geni_transfer_one(struct spi_master *spi,
@@ -478,11 +497,16 @@ static irqreturn_t geni_spi_isr(int irq, void *data) 
	struct geni_se *se = &mas->se;

	u32 m_irq;



	if (mas->cur_mcmd == CMD_NONE)

	m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);

	if (!m_irq)

		return IRQ_NONE;



	if (m_irq & (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |

		     M_RX_FIFO_RD_ERR_EN | M_RX_FIFO_WR_ERR_EN |

		     M_TX_FIFO_RD_ERR_EN | M_TX_FIFO_WR_ERR_EN))

		dev_warn(mas->dev, "Unexpected IRQ err status %#010x\n", m_irq);



	spin_lock(&mas->lock);

	m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);



	if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))

		geni_spi_handle_rx(mas);
@@ -491,11 +515,13 @@ static irqreturn_t geni_spi_isr(int irq, void *data) 
		geni_spi_handle_tx(mas);



	if (m_irq & M_CMD_DONE_EN) {

		if (mas->cur_mcmd == CMD_XFER)

		if (mas->cur_xfer) {

			spi_finalize_current_transfer(spi);

		else if (mas->cur_mcmd == CMD_CS)

			complete(&mas->xfer_done);

		mas->cur_mcmd = CMD_NONE;

			mas->cur_xfer = NULL;

		} else {

			complete(&mas->cs_done);

		}



		/*

		 * If this happens, then a CMD_DONE came before all the Tx

		 * buffer bytes were sent out. This is unusual, log this
@@ -517,13 +543,28 @@ static irqreturn_t geni_spi_isr(int irq, void *data) 
				mas->rx_rem_bytes, mas->cur_bits_per_word);

	}



	if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN)) {

		mas->cur_mcmd = CMD_NONE;

		complete(&mas->xfer_done);

	}

	if (m_irq & M_CMD_CANCEL_EN)

		complete(&mas->cancel_done);

	if (m_irq & M_CMD_ABORT_EN)

		complete(&mas->abort_done);



	/*

	 * It's safe or a good idea to Ack all of our our interrupts at the

	 * end of the function. Specifically:

	 * - M_CMD_DONE_EN / M_RX_FIFO_LAST_EN: Edge triggered interrupts and

	 *   clearing Acks. Clearing at the end relies on nobody else having

	 *   started a new transfer yet or else we could be clearing _their_

	 *   done bit, but everyone grabs the spinlock before starting a new

	 *   transfer.

	 * - M_RX_FIFO_WATERMARK_EN / M_TX_FIFO_WATERMARK_EN: These appear

	 *   to be "latched level" interrupts so it's important to clear them

	 *   _after_ you've handled the condition and always safe to do so

	 *   since they'll re-assert if they're still happening.

	 */

	writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);



	spin_unlock(&mas->lock);



	return IRQ_HANDLED;

}
@@ -573,7 +614,9 @@ static int spi_geni_probe(struct platform_device *pdev) 
	spi->handle_err = handle_fifo_timeout;

	spi->set_cs = spi_geni_set_cs;



	init_completion(&mas->xfer_done);

	init_completion(&mas->cs_done);

	init_completion(&mas->cancel_done);

	init_completion(&mas->abort_done);

	spin_lock_init(&mas->lock);

	pm_runtime_enable(dev);

CRA Git | Maintained and supported by SUSTech CRA and CCSE