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

[Sourav Poddar]

Hi Felipe,
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.

So, for 8 bits word we need to use readb, for 16 bits word readw.
> > > > +static int ti_qspi_start_transfer_one(struct spi_master *master,
> > > > +		struct spi_message *m)
> > > > +{
> > > > +	struct ti_qspi *qspi = spi_master_get_devdata(master);
> > > > +	struct spi_device *spi = m->spi;
> > > > +	struct spi_transfer *t;
> > > > +	int status = 0, ret;
> > > > +	int frame_length;
> > > > +
> > > > +	/* setup device control reg */
> > > > +	qspi->dc = 0;
> > > > +
> > > > +	if (spi->mode&   SPI_CPHA)
> > > > +		qspi->dc |= QSPI_CKPHA(spi->chip_select);
> > > > +	if (spi->mode&   SPI_CPOL)
> > > > +		qspi->dc |= QSPI_CKPOL(spi->chip_select);
> > > > +	if (spi->mode&   SPI_CS_HIGH)
> > > > +		qspi->dc |= QSPI_CSPOL(spi->chip_select);
> > > > +
> > > > +	frame_length = DIV_ROUND_UP(m->frame_length * spi->bits_per_word,
> > > > +				spi->bits_per_word);
> > > this calculation doesn't look correct.
> > >
> > > 	(m->frame_length * spi->bits_per_word) /
> > > 		spi->bits_per_word = m->frame_length
> > >
> > > What are you trying to achieve here ? frame_length should be counted in
> > > words right ? And we get that value in bytes. So what's the best
> > > calculation to convert bytes into words ? If you have 8 bits_per_word
> > > you don't need any calculation, but if you have 32 bits_per_word, you
> > > _do_ need something.
> > Yes, just derive this formulae with 8 bits per word in mind.
> > Will change.
> > It should be (m->frame_length * 8) / spi->bits_per_word
> right on. To make sure this will execute a little faster (you never know
> what several different versions of GCC will do), instead of multiplying
> by 8, left shift by 3.
>
> > > How will you achieve the number you want ? (hint: 1 byte == 8 bits)
> > >
> > > And btw, all of these mistakes pretty much tell me that this driver
> > > hasn't been tested. How have you tested this driver ?
> > After bootup, I checked for deive detting enumerated as /proc/mtd.
> > After which I am using mtdutils(erase, dump and write utilied to
> > check for the communication with the flash device.)
> alright, make that clear in your commit log.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/