diff mbox

[v3] spi: qup: Fix incorrect block transfers

Message ID 1412112088-25928-1-git-send-email-agross@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Andy Gross Sept. 30, 2014, 9:21 p.m. UTC 
This patch fixes a number of errors with the QUP block transfer mode.  Errors
manifested themselves as input underruns, output overruns, and timed out
transactions.

The block mode does not require the priming that occurs in FIFO mode.  At the
moment that the QUP is placed into the RUN state, the QUP will immediately raise
an interrupt if the request is a write.  Therefore, there is no need to prime
the pump.

In addition, the block transfers require that whole blocks of data are
read/written at a time.  The last block of data that completes a transaction may
contain less than a full blocks worth of data.

Each block of data results in an input/output service interrupt accompanied with
a input/output block flag set.  Additional block reads/writes require clearing
of the service flag.  It is ok to check for additional blocks of data in the
ISR, but you have to ack every block you transfer.  Imbalanced acks result in
early return from complete transactions with pending interrupts that still have
to be ack'd.  The next transaction can be affected by these interrupts.
Transactions are deemed complete when the MAX_INPUT or MAX_OUTPUT flag are set.

Changes from v2:
- Added in additional completion check so that transaction done is not
  prematurely signaled.
- Fixed various review comments.

Changes from v1:
- Split out read/write block function.
- Removed extraneous checks for transfer length

Signed-off-by: Andy Gross <agross@codeaurora.org>
---
 drivers/spi/spi-qup.c |  201 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 148 insertions(+), 53 deletions(-)

Comments

Ivan T. Ivanov Oct. 2, 2014, 1:44 p.m. UTC | #1 
Hi Andy,

I am trying to understand why we need extra functions for block 
read and write.

Essentially fifo and block read/write function are looking
the same for me. Except that block functions have one extra write
in QUP_OPERATIONAL register.

On Tue, 2014-09-30 at 16:21 -0500, Andy Gross wrote:
> This patch fixes a number of errors with the QUP block transfer mode.  Errors
> manifested themselves as input underruns, output overruns, and timed out
> transactions.
> 
> The block mode does not require the priming that occurs in FIFO mode.  At the
> moment that the QUP is placed into the RUN state, the QUP will immediately raise
> an interrupt if the request is a write.  Therefore, there is no need to prime
> the pump.

But does this hurt in some way? I mean fist FIFO fill happens when
controller is in PAUSED state. Once enabled it can start transfer 
immediately. 

> 
> In addition, the block transfers require that whole blocks of data are
> read/written at a time.  

Thats fine, but I can not see why this will not happen with existing
fill functions. Fifo's are drained until there is data and filled
until there is a space. And because we are not using pack/unpack mode, 
every SPI word occupy one cell in fifo (32 bits), this means that 
existing read/write functions are working in "block" mode.

> The last block of data that completes a transaction may
> contain less than a full blocks worth of data.
> 
> Each block of data results in an input/output service interrupt accompanied with
> a input/output block flag set.  Additional block reads/writes require clearing
> of the service flag.  It is ok to check for additional blocks of data in the
> ISR, but you have to ack every block you transfer.  Imbalanced acks result in
> early return from complete transactions with pending interrupts that still have
> to be ack'd.  The next transaction can be affected by these interrupts.
> Transactions are deemed complete when the MAX_INPUT or MAX_OUTPUT flag are set.

And this is the thing that can cause errors that you see, I suppose.
We are getting extra interrupts, which are not cleared, even if we have
drained fifo completely. 

Regards,
Ivan

P.S. They are still several coding style issues :-). The same as those that 
I have already pointed to you.


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gross Oct. 2, 2014, 3:28 p.m. UTC | #2 
On Thu, Oct 02, 2014 at 04:44:32PM +0300, Ivan T. Ivanov wrote:
> 
> Hi Andy,
> 
> I am trying to understand why we need extra functions for block 
> read and write.

I separated them out because the constraints for doing the allowable
writes/reads was making the whole thing messy.  I opted for a cleaner
implementation.

> 
> Essentially fifo and block read/write function are looking
> the same for me. Except that block functions have one extra write
> in QUP_OPERATIONAL register.

It's not just that, the difference between the fifo and block is that you just
read/write the fifo until you hit the full or empty.  You can't do that with
block.  you have to read in block size transfers (4x32B read/writes).

> 
> On Tue, 2014-09-30 at 16:21 -0500, Andy Gross wrote:
> > This patch fixes a number of errors with the QUP block transfer mode.  Errors
> > manifested themselves as input underruns, output overruns, and timed out
> > transactions.
> > 
> > The block mode does not require the priming that occurs in FIFO mode.  At the
> > moment that the QUP is placed into the RUN state, the QUP will immediately raise
> > an interrupt if the request is a write.  Therefore, there is no need to prime
> > the pump.
> 
> But does this hurt in some way? I mean fist FIFO fill happens when
> controller is in PAUSED state. Once enabled it can start transfer 
> immediately. 

Unfortunately that creates a race in the block mode.  Because we hit run, we are
telling the controller to do something.  In the block case, it fires off an IRQ
to request output blocks.  This IRQ is going to hit sometime before, during, or
after the fifo_write function.  If we are unlucky and we are in the fifo_write
and somewhere in that loop, the irq is handled and we enter the fifo_write
function again.  We already did some number of writes/reads that have not been
recorded.  The IRQ fifo_write rips through and fills the fifo.  Then we return
from the IRQ and continue to read/write and this causes the overrun/underrun.

Letting the IRQ handle the block transfer start removes this specific issue.
The cost is one irq overhead and the read/write of 16 words.

> 
> > 
> > In addition, the block transfers require that whole blocks of data are
> > read/written at a time.  
> 
> Thats fine, but I can not see why this will not happen with existing
> fill functions. Fifo's are drained until there is data and filled
> until there is a space. And because we are not using pack/unpack mode, 
> every SPI word occupy one cell in fifo (32 bits), this means that 
> existing read/write functions are working in "block" mode.

Not true.  The block mode is different because the IRQ generation is tied to the
block size.  You won't get a service interrupt until either the transaction is
completely done, or a block of data is read/written.  And you get an IRQ for
each block that has to be ACKd.  That is completely different than the behavior
than the FIFO mode.  If you are not reading/writing full blocks of data, you are
doing it wrong.

In FIFO mode, you get a service interrupt when at least 1 word of data is ready
to be read, or there is space to place at least one word of data for write.

> 
> > The last block of data that completes a transaction may
> > contain less than a full blocks worth of data.
> > 
> > Each block of data results in an input/output service interrupt accompanied with
> > a input/output block flag set.  Additional block reads/writes require clearing
> > of the service flag.  It is ok to check for additional blocks of data in the
> > ISR, but you have to ack every block you transfer.  Imbalanced acks result in
> > early return from complete transactions with pending interrupts that still have
> > to be ack'd.  The next transaction can be affected by these interrupts.
> > Transactions are deemed complete when the MAX_INPUT or MAX_OUTPUT flag are set.
> 
> And this is the thing that can cause errors that you see, I suppose.
> We are getting extra interrupts, which are not cleared, even if we have
> drained fifo completely. 

We get errors because
1) We don't adhere to the programming model for block mode
2) We return early from transactions that have not been fully ack'd.

> 
> Regards,
> Ivan
> 
> P.S. They are still several coding style issues :-). The same as those that 
> I have already pointed to you.

Sigh, I thought i fixed those.  I'll have another look.
diff mbox

Patch

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 9f83d29..889a33a 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -80,6 +80,8 @@ 
 #define QUP_IO_M_MODE_BAM		3
 
 /* QUP_OPERATIONAL fields */
+#define QUP_OP_IN_BLOCK_READ_REQ	BIT(13)
+#define QUP_OP_OUT_BLOCK_WRITE_REQ	BIT(12)
 #define QUP_OP_MAX_INPUT_DONE_FLAG	BIT(11)
 #define QUP_OP_MAX_OUTPUT_DONE_FLAG	BIT(10)
 #define QUP_OP_IN_SERVICE_FLAG		BIT(9)
@@ -143,6 +145,7 @@  struct spi_qup {
 	int			tx_bytes;
 	int			rx_bytes;
 	int			qup_v1;
+	int			mode;
 };
 
 
@@ -198,30 +201,14 @@  static int spi_qup_set_state(struct spi_qup *controller, u32 state)
 	return 0;
 }
 
-
-static void spi_qup_fifo_read(struct spi_qup *controller,
-			    struct spi_transfer *xfer)
+static void spi_qup_fill_read_buffer(struct spi_qup *controller,
+	struct spi_transfer *xfer, u32 data)
 {
 	u8 *rx_buf = xfer->rx_buf;
-	u32 word, state;
-	int idx, shift, w_size;
-
-	w_size = controller->w_size;
-
-	while (controller->rx_bytes < xfer->len) {
-
-		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
-		if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
-			break;
-
-		word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
-
-		if (!rx_buf) {
-			controller->rx_bytes += w_size;
-			continue;
-		}
+	int idx, shift;
 
-		for (idx = 0; idx < w_size; idx++, controller->rx_bytes++) {
+	if (rx_buf)
+		for (idx = 0; idx < controller->w_size; idx++) {
 			/*
 			 * The data format depends on bytes per SPI word:
 			 *  4 bytes: 0x12345678
@@ -229,41 +216,139 @@  static void spi_qup_fifo_read(struct spi_qup *controller,
 			 *  1 byte : 0x00000012
 			 */
 			shift = BITS_PER_BYTE;
-			shift *= (w_size - idx - 1);
-			rx_buf[controller->rx_bytes] = word >> shift;
+			shift *= (controller->w_size - idx - 1);
+			rx_buf[controller->rx_bytes + idx] = data >> shift;
+		}
+
+	controller->rx_bytes += controller->w_size;
+}
+
+static void spi_qup_prepare_write_data(struct spi_qup *controller,
+	struct spi_transfer *xfer, u32 *data)
+{
+	const u8 *tx_buf = xfer->tx_buf;
+	u32 val;
+	int idx;
+
+	*data = 0;
+
+	if (tx_buf)
+		for (idx = 0; idx < controller->w_size; idx++) {
+			val = tx_buf[controller->tx_bytes + idx];
+			*data |= val << (BITS_PER_BYTE * (3 - idx));
 		}
+
+	controller->tx_bytes += controller->w_size;
+}
+
+static void spi_qup_fifo_read(struct spi_qup *controller,
+			    struct spi_transfer *xfer)
+{
+	u32 data;
+
+	/* clear service request */
+	writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
+			controller->base + QUP_OPERATIONAL);
+
+	while (controller->rx_bytes < xfer->len) {
+		if (!(readl_relaxed(controller->base + QUP_OPERATIONAL) &
+		    QUP_OP_IN_FIFO_NOT_EMPTY))
+			break;
+
+		data = readl_relaxed(controller->base + QUP_INPUT_FIFO);
+
+		spi_qup_fill_read_buffer(controller, xfer, data);
 	}
 }
 
 static void spi_qup_fifo_write(struct spi_qup *controller,
-			    struct spi_transfer *xfer)
+	struct spi_transfer *xfer)
 {
-	const u8 *tx_buf = xfer->tx_buf;
-	u32 word, state, data;
-	int idx, w_size;
+	u32 data;
 
-	w_size = controller->w_size;
+	/* clear service request */
+	writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
+		controller->base + QUP_OPERATIONAL);
 
 	while (controller->tx_bytes < xfer->len) {
 
-		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
-		if (state & QUP_OP_OUT_FIFO_FULL)
+		if (readl_relaxed(controller->base + QUP_OPERATIONAL) &
+				QUP_OP_OUT_FIFO_FULL)
 			break;
 
-		word = 0;
-		for (idx = 0; idx < w_size; idx++, controller->tx_bytes++) {
+		spi_qup_prepare_write_data(controller, xfer, &data);
+		writel_relaxed(data, controller->base + QUP_OUTPUT_FIFO);
 
-			if (!tx_buf) {
-				controller->tx_bytes += w_size;
-				break;
-			}
+	}
+}
 
-			data = tx_buf[controller->tx_bytes];
-			word |= data << (BITS_PER_BYTE * (3 - idx));
+static void spi_qup_block_read(struct spi_qup *controller,
+	struct spi_transfer *xfer)
+{
+	u32 data;
+	u32 reads_per_blk = controller->in_blk_sz >> 2;
+	u32 num_words = (xfer->len - controller->rx_bytes) / controller->w_size;
+	int i;
+
+	do {
+		/* ACK by clearing service flag */
+		writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
+			controller->base + QUP_OPERATIONAL);
+
+		/* transfer up to a block size of data in a single pass */
+		for (i = 0; num_words && i < reads_per_blk; i++, num_words--) {
+
+			/* read data and fill up rx buffer */
+			data = readl_relaxed(controller->base + QUP_INPUT_FIFO);
+			spi_qup_fill_read_buffer(controller, xfer, data);
 		}
 
-		writel_relaxed(word, controller->base + QUP_OUTPUT_FIFO);
-	}
+		/* check to see if next block is ready */
+		if (!(readl_relaxed(controller->base + QUP_OPERATIONAL) &
+			QUP_OP_IN_BLOCK_READ_REQ))
+			break;
+
+	} while (num_words);
+
+	/*
+	 * Due to extra stickiness of the QUP_OP_IN_SERVICE_FLAG during block
+	 * reads, it has to be cleared again at the very end
+	 */
+	if (readl_relaxed(controller->base + QUP_OPERATIONAL) &
+		QUP_OP_MAX_INPUT_DONE_FLAG)
+		writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
+			controller->base + QUP_OPERATIONAL);
+
+}
+
+static void spi_qup_block_write(struct spi_qup *controller,
+	struct spi_transfer *xfer)
+{
+	u32 data;
+	u32 writes_per_blk = controller->out_blk_sz >> 2;
+	u32 num_words = (xfer->len - controller->tx_bytes) / controller->w_size;
+	int i;
+
+	do {
+		/* ACK by clearing service flag */
+		writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
+			controller->base + QUP_OPERATIONAL);
+
+		/* transfer up to a block size of data in a single pass */
+		for (i = 0; num_words && i < writes_per_blk; i++, num_words--) {
+
+			/* swizzle the bytes for output and write out */
+			spi_qup_prepare_write_data(controller, xfer, &data);
+			writel_relaxed(data,
+				controller->base + QUP_OUTPUT_FIFO);
+		}
+
+		/* check to see if next block is ready */
+		if (!(readl_relaxed(controller->base + QUP_OPERATIONAL) &
+			QUP_OP_OUT_BLOCK_WRITE_REQ))
+			break;
+
+	} while (num_words);
 }
 
 static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
@@ -285,9 +370,9 @@  static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
 
 	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
 	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
-	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
 
 	if (!xfer) {
+		writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
 		dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x %08x\n",
 				    qup_err, spi_err, opflags);
 		return IRQ_HANDLED;
@@ -315,29 +400,37 @@  static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
 		error = -EIO;
 	}
 
-	if (opflags & QUP_OP_IN_SERVICE_FLAG)
-		spi_qup_fifo_read(controller, xfer);
+	if (opflags & QUP_OP_IN_SERVICE_FLAG) {
+		if (opflags & QUP_OP_IN_BLOCK_READ_REQ)
+			spi_qup_block_read(controller, xfer);
+		else
+			spi_qup_fifo_read(controller, xfer);
+	}
 
-	if (opflags & QUP_OP_OUT_SERVICE_FLAG)
-		spi_qup_fifo_write(controller, xfer);
+	if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
+		if (opflags & QUP_OP_OUT_BLOCK_WRITE_REQ)
+			spi_qup_block_write(controller, xfer);
+		else
+			spi_qup_fifo_write(controller, xfer);
+	}
 
 	spin_lock_irqsave(&controller->lock, flags);
 	controller->error = error;
 	controller->xfer = xfer;
 	spin_unlock_irqrestore(&controller->lock, flags);
 
-	if (controller->rx_bytes == xfer->len || error)
+	if ((controller->rx_bytes == xfer->len &&
+		(opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error)
 		complete(&controller->done);
 
 	return IRQ_HANDLED;
 }
 
-
 /* set clock freq ... bits per word */
 static int spi_qup_io_config(struct spi_device *spi, struct spi_transfer *xfer)
 {
 	struct spi_qup *controller = spi_master_get_devdata(spi->master);
-	u32 config, iomode, mode;
+	u32 config, iomode;
 	int ret, n_words, w_size;
 
 	if (spi->mode & SPI_LOOP && xfer->len > controller->in_fifo_sz) {
@@ -368,14 +461,14 @@  static int spi_qup_io_config(struct spi_device *spi, struct spi_transfer *xfer)
 	controller->w_size = w_size;
 
 	if (n_words <= (controller->in_fifo_sz / sizeof(u32))) {
-		mode = QUP_IO_M_MODE_FIFO;
+		controller->mode = QUP_IO_M_MODE_FIFO;
 		writel_relaxed(n_words, controller->base + QUP_MX_READ_CNT);
 		writel_relaxed(n_words, controller->base + QUP_MX_WRITE_CNT);
 		/* must be zero for FIFO */
 		writel_relaxed(0, controller->base + QUP_MX_INPUT_CNT);
 		writel_relaxed(0, controller->base + QUP_MX_OUTPUT_CNT);
 	} else {
-		mode = QUP_IO_M_MODE_BLOCK;
+		controller->mode = QUP_IO_M_MODE_BLOCK;
 		writel_relaxed(n_words, controller->base + QUP_MX_INPUT_CNT);
 		writel_relaxed(n_words, controller->base + QUP_MX_OUTPUT_CNT);
 		/* must be zero for BLOCK and BAM */
@@ -387,8 +480,8 @@  static int spi_qup_io_config(struct spi_device *spi, struct spi_transfer *xfer)
 	/* Set input and output transfer mode */
 	iomode &= ~(QUP_IO_M_INPUT_MODE_MASK | QUP_IO_M_OUTPUT_MODE_MASK);
 	iomode &= ~(QUP_IO_M_PACK_EN | QUP_IO_M_UNPACK_EN);
-	iomode |= (mode << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT);
-	iomode |= (mode << QUP_IO_M_INPUT_MODE_MASK_SHIFT);
+	iomode |= (controller->mode << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT);
+	iomode |= (controller->mode << QUP_IO_M_INPUT_MODE_MASK_SHIFT);
 
 	writel_relaxed(iomode, controller->base + QUP_IO_M_MODES);
 
@@ -462,7 +555,8 @@  static int spi_qup_transfer_one(struct spi_master *master,
 		goto exit;
 	}
 
-	spi_qup_fifo_write(controller, xfer);
+	if (controller->mode == QUP_IO_M_MODE_FIFO)
+		spi_qup_fifo_write(controller, xfer);
 
 	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
 		dev_warn(controller->dev, "cannot set EXECUTE state\n");
@@ -478,6 +572,7 @@  exit:
 	if (!ret)
 		ret = controller->error;
 	spin_unlock_irqrestore(&controller->lock, flags);
+
 	return ret;
 }