Hi Andy, On Sun, 2014-09-21 at 23:27 -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.
At what speeds are you seeing those errors? > > 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 may 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. Does this mean that block transfer will start only if the required bytes from block is written into buffer? > > 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. > > Signed-off-by: Andy Gross <[email protected]> > --- > drivers/spi/spi-qup.c | 194 > +++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 141 insertions(+), 53 deletions(-) > <snip> > +static void qup_fill_read_buffer(struct spi_qup *controller, > + struct spi_transfer *xfer, u32 data) Please, could prefix this whit spi_ to be consistent with the rest of the code. > { > 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; > + int idx, shift; > + int read_len = min_t(int, xfer->len - controller->rx_bytes, > + controller->w_size); You should not need this check here. xfer->len is multiple of controller->w_size and you always read one word at time. > > - word = readl_relaxed(controller->base + QUP_INPUT_FIFO); > - > - if (!rx_buf) { > - controller->rx_bytes += w_size; > - continue; > - } > - > - for (idx = 0; idx < w_size; idx++, controller->rx_bytes++) { > + if (rx_buf) > + for (idx = 0; idx < read_len; idx++) { > /* > * The data format depends on bytes per SPI word: > * 4 bytes: 0x12345678 > @@ -229,40 +218,129 @@ 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 += read_len; > } > > -static void spi_qup_fifo_write(struct spi_qup *controller, > - struct spi_transfer *xfer) > +static void qup_prepare_write_data(struct spi_qup *controller, > + struct spi_transfer *xfer, u32 *data) > { > const u8 *tx_buf = xfer->tx_buf; > - u32 word, state, data; > - int idx, w_size; > + u32 val; > + int idx; > + int write_len = min_t(int, xfer->len - controller->tx_bytes, > + controller->w_size); > Same here. > - w_size = controller->w_size; > + *data = 0; > > - while (controller->tx_bytes < xfer->len) { > + if (tx_buf) > + for (idx = 0; idx < write_len; idx++) { > + val = tx_buf[controller->tx_bytes + idx]; > + *data |= val << (BITS_PER_BYTE * (3 - idx)); > + } > > - state = readl_relaxed(controller->base + QUP_OPERATIONAL); > - if (state & QUP_OP_OUT_FIFO_FULL) > - break; > + controller->tx_bytes += write_len; > +} > > - word = 0; > - for (idx = 0; idx < w_size; idx++, controller->tx_bytes++) { > +static void spi_qup_service_block(struct spi_qup *controller, > + struct spi_transfer *xfer, bool is_read) > +{ Please, could you split this function to read and write, so we can use: spi_qup_fifo_read() and spi_qup_fifo_write() in FIFO modes and spi_qup_block_read() and spi_qup_block_write() for BLOCK mode. > + u32 data, words_per_blk, num_words, ack_flag, op_flag; > + int i; > + > + if (is_read) { > + op_flag = QUP_OP_IN_BLOCK_READ_REQ; > + ack_flag = QUP_OP_IN_SERVICE_FLAG; > + num_words = DIV_ROUND_UP(xfer->len - controller->rx_bytes, > + controller->w_size); Same here and below. > + words_per_blk = controller->in_blk_sz >> 2; > + } else { > + op_flag = QUP_OP_OUT_BLOCK_WRITE_REQ; > + ack_flag = QUP_OP_OUT_SERVICE_FLAG; > + num_words = DIV_ROUND_UP(xfer->len - controller->tx_bytes, > + controller->w_size); > + words_per_blk = controller->out_blk_sz >> 2; > + } Thank you. Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
