Re: [PATCHv7 1/2] drivers: spi: Add qspi flash controller

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 4653 bytes --]

Hi,

On Wed, Jul 31, 2013 at 02:40:51PM +0530, Sourav Poddar wrote:
> >>+#define	QSPI_FRAME_MAX			0xfff
> >Frame max is 4096, 0x1000, right ?
> Yes,
> this macro was used initially to fill the register bits, where 4095 =
> 4096 words.
> Will change it to now.

you can make this something like:

#define QSPI_FRAME(n)		(((n) - 1) & 0xfff)

> >>+static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> >>+{
> >>+	const u8 *txbuf;
> >>+	int wlen, count, ret;
> >>+
> >>+	count = t->len;
> >>+	txbuf = t->tx_buf;
> >>+	wlen = t->bits_per_word;
> >>+
> >>+	while (count--) {
> >you're decrementing count by one, but in some cases you write 4 bytes or
> >2 bytes... This will blow up very soon. I can already see overflows
> >happening...
> we write 2 bytes and 4 bytes for 16 bits_per_word and 32 bits_per_word case.
> count is t->len, which is the total number of words to transfer. This

t->len is total number of bytes as you can see from the documentation in
the header:

* @len: size of rx and tx buffers (in bytes)

As I said before, please read the documentation.

> words can be of any length (1, 2 or 4) bytes. So, I think it should be
> decremented by 1 only.

this is wrong.

> >>+static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> >>+{
> >>+	int ret;
> >>+
> >>+	if (t->tx_buf) {
> >>+		ret = qspi_write_msg(qspi, t);
> >>+		if (ret) {
> >>+			dev_dbg(qspi->dev, "Error while writing\n");
> >>+			return -EINVAL;
> >why do you change the return value from qspi_write_msg() ?
> >
> I  was not sure about this, I thought I had signals an ETIMEOUT during
> timeout, So signal a invalid transfer here.
> Do you suggest keeping ETIMEOUT here also?

yeah, so we tell whoever called us that the transfer timed out. If you
return -EINVAL you're telling your clients they gave you an invalid
spi_transfer, which is not the case.

> >>+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 = (m->frame_length<<  3) / spi->bits_per_word;
> >>+
> >>+	frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX);
> >>+
> >>+	/* setup command reg */
> >>+	qspi->cmd = 0;
> >>+	qspi->cmd |= QSPI_EN_CS(spi->chip_select);
> >>+	qspi->cmd |= QSPI_FLEN(frame_length);
> >>+	qspi->cmd |= QSPI_WC_CMD_INT_EN;
> >>+
> >>+	ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
> >>+
> >>+	list_for_each_entry(t,&m->transfers, transfer_list) {
> >no locking around list traversal ?
> >
> hmm..can put a spin_lock around "qspi_transfer_msg" ?

no dude, you need to protect the access to the list. So it needs to be
around list_for_each_entry().

> >>+static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
> >>+{
> >>+	struct ti_qspi *qspi = dev_id;
> >>+	u16 stat;
> >>+
> >>+	irqreturn_t ret = IRQ_HANDLED;
> >>+
> >>+	spin_lock(&qspi->lock);
> >>+
> >>+	stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR);
> >>+
> >>+	if (!stat) {
> >>+		dev_dbg(qspi->dev, "No IRQ triggered\n");
> >>+		return IRQ_NONE;
> >leaving lock held.
> >
> Will add a unlock before returning.

there's a very nice C statement, goto, which you can use here.

> >>+static irqreturn_t ti_qspi_threaded_isr(int this_irq, void *dev_id)
> >>+{
> >>+	struct ti_qspi *qspi = dev_id;
> >>+	unsigned long flags;
> >>+
> >>+	spin_lock_irqsave(&qspi->lock, flags);
> >>+
> >>+	complete(&qspi->transfer_complete);
> >you need to check if your word completion is actually set. Don't assume
> >it's set because we might want to change the code later.
> >
> hmm..something like this.?
>   if (ti_qspi_read(qspi, QSPI_SPI_STATUS_REG) & WC)
>         complete(&qspi->transfer_complete);

I rather:

stat = ti_qspi_read(qspi, QSPI_SPI_STATUS_REG);

if (stat & WC)
	complete()

then, if we want to add frame interrupt handling later, we don't need to
read status register again. In fact, to avoid reading the status
register here, you could even cache the returned value you read in your
hardirq handler inside your qspi struct.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]