Re: [PATCH] spi: qup: Fix incorrect block transfers

[Andy Gross <agross-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On Tue, Sep 23, 2014 at 12:24:27PM +0300, Ivan T. Ivanov wrote:
> 
> 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?

We've tried 25MHz and 50MHz.  Both fail in the same way.  Keep in mind this is
definitely a timing / race issue and it probably also dependent on the latency
of the attached device.  I cannot reproduce this at all on my IPQ8064 based
board, but others can.

This problem manifested itself while using spidev and a usermode flash
programming application (flashrom).

> 
> > 
> > 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?

No, a better way of putting this is that immediately on setting RUN state,
you'll get a service interrupt to fill the FIFO in block mode.  So there is no
need to prime the FIFO from the non-isr context due to this behavior.

<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.

Good point.  I need to be consistent.

> >  {
> >  	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.

Ah I missed the __spi_validate where this is done.  I'll remove this.  Good
catch.

<snip>

> >  	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.

Agreed.

<snip>

> > -		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.

Well I had it collapsed and the functions are identical except for the
read/write specific pieces, which amount to 2 lines.  I can resplit it out.  It
makes it symmetric.

> 
> > +	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.

Agreed.

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

-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html