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

[Sourav Poddar <sourav.poddar@ti.com>]

Hi Felipe,
On Thursday 18 July 2013 03:54 PM, Felipe Balbi wrote:
> Hi,
>
> it might be just me, but ...
>
> On Thu, Jul 18, 2013 at 03:31:26PM +0530, Sourav Poddar wrote:
>> +static inline unsigned long ti_qspi_readl_data(struct ti_qspi *qspi,
>> +		unsigned long reg, int wlen)
>> +{
>> +	switch (wlen) {
>> +	case 8:
>> +		return readw(qspi->base + reg);
>> +		break;
>> +	case 16:
>> +		return readb(qspi->base + reg);
>> +		break;
>> +	case 32:
>> +		return readl(qspi->base + reg);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static inline void ti_qspi_writel_data(struct ti_qspi *qspi,
>> +		unsigned long val, unsigned long reg, int wlen)
>> +{
>> +	switch (wlen) {
>> +	case 8:
>> +		writew(val, qspi->base + reg);
>> +		break;
>> +	case 16:
>> +		writeb(val, qspi->base + reg);
>> +		break;
>> +	case 32:
>> +		writeb(val, qspi->base + reg);
>> +		break;
>> +	default:
>> +		dev_dbg(qspi->dev, "word lenght out of range");
>> +		break;
>> +	}
>> +}
> because of these two functions you have the hability to read/write
> *more* than one byte, and yet ...
>
>> +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?
> You only use writew() if you have exactly 2 bytes to write and writeb()
> if you have exactly 1 byte to write. 3 bytes we'll be left as an
> exercise.
hmm..yes.
>> +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
> 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.)
> Is your spansion
> memory accessed with 8 bits_per_word only ?
Yes, most of the places is like that and data is sapmled in 8 bits.
For some opcodes, we need to send 3 bytes addresses after instruction
  to the flash chip.
>   Is there anyway to use
> 32 bits_per_word with that device ? That would uncover quite a few
> mistakes in $subject.
>