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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130718112458.GJ11251@arwen.pp.htv.fi>
Date:	Thu, 18 Jul 2013 14:24:58 +0300
From:	Felipe Balbi <balbi@...com>
To:	Sourav Poddar <sourav.poddar@...com>
CC:	<balbi@...com>, <broonie@...nel.org>,
	<spi-devel-general@...ts.sourceforge.net>,
	<grant.likely@...aro.org>, <rnayak@...com>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

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.

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

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)