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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux