Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

[Felipe Balbi]

Hi,

On Fri, Jul 19, 2013 at 05:18:47PM +0530, Sourav Poddar wrote:
> On Thursday 18 July 2013 04:54 PM, Felipe Balbi wrote:
> >On Thu, Jul 18, 2013 at 04:48:41PM +0530, Sourav Poddar wrote:
> >>>>+static void qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> >>>>+{
> >>>>+	const u8 *txbuf;
> >>>>+	int wlen, count;
> >>>>+
> >>>>+	count = t->len;
> >>>>+	txbuf = t->tx_buf;
> >>>>+	wlen = t->bits_per_word;
> >>>>+
> >>>>+	while (count--) {
> >>>>+		dev_dbg(qspi->dev, "tx cmd %08x dc %08x data %02x\n",
> >>>>+			qspi->cmd | QSPI_WR_SNGL, qspi->dc, *txbuf);
> >>>>+		ti_qspi_writel_data(qspi, *txbuf++, QSPI_SPI_DATA_REG, wlen);
> >>>you always increment by each byte. Here, if you used writel(), you wrote
> >>>4 bytes and should increment txbuf by 4.
> >>hmm..got this point. Yes, my mistake, here I agree if wlen is not 8 bits
> >>txbuf++ is not valid.
> >>>  Same goes for read_data(),
> >>>below. Another thing. Even though your wlen might be 8 bits, if you
> >>>write 4 bytes to write, you can save a few CPU cycles by using writel().
> >>>
> >>Do you mean 4 words of 8 bits?
> >yeah. Say you have wlen = 8 but the transfer length is 8 bytes (64
> >bits). If you use writeb(), you will do 8 writes, if you use writel()
> >you'll do 2 writes.
> >
> Just some more findings on this, after wlen bits are transferred we
> need an WC interrupt.
> So, if I try to pack 4 words of 8bits and use readl/writel, there
> will be an interrupt after every
> wlen bits transferred and things will get screwd up.

they will not if you throttle the IRQs or add some knowledge about the
FIFO sizes. I mean, there are ways to get this working.

> So, for 8 bits word we need to use readb, for 16 bits word readw.

not entirely true...

I mean, you have a 32-bit FIFO, why would underutilize the FIFO by
always writing 8-bits ? Write 32 bits, when you get the first word
completion, you know you have 8-bits of space in the FIFO, then you
can fill those 8 bits with new data. (all of this is assuming word
length is 8-bits, clearly if it is more, then you need to change the
assumptions).

Another thing which might help, is checking if the HW will even access
the other DATA registers even if word length is <= 32 bits. If it does,
then you have a total of 128 bits to shift data into, which can save you
a lot of time filling up FIFOs and waiting for them to empty.

-- 
balbi

Attachment: signature.asc
Description: Digital signature