spi: dw_spi: refine the IRQ mode working flow

PATCH In review Feng Tang 2 participants 5 messages Jan 19, 2010 – Jan 21, 2010 2 messages not in the archive SPI

lore

Share & tools

lore

Feng Tang

author

2010-01-19 03:20 UTC #0 lore raw

PATCH 3/3 spi: dw_spi: refine the IRQ mode working flow

Quoted 1 line

From e1a3ba6f5605e7d0ae568b3d747b5ff03f7e202c Mon Sep 17 00:00:00 2001

From: Feng Tang <[email protected]>
Date: Tue, 19 Jan 2010 10:22:45 +0800
Subject: [PATCH 3/3] spi: dw_spi: refine the IRQ mode working flow

Now dw_spi core fully supports 3 transfer modes: pure polling, DMA and IRQ mode. IRQ mode will use the FIFO half empty as the IRQ trigger, so each interface driver need set the fifo_len, so that core driver can handle it properly

Signed-off-by Feng Tang <[email protected]>

---
 drivers/spi/dw_spi.c       |   64 ++++++++++++++++++++++++++-----------------
 drivers/spi/dw_spi_pci.c   |    1 +
 include/linux/spi/dw_spi.h |    1 +
 3 files changed, 41 insertions(+), 25 deletions(-)

drivers/spi/dw_spi.c +39 ~~-25~~

index 521d680..1bb709b 100644
@@ -358,6 +358,8 @@ static void transfer_complete(struct dw_spi *dws)
 static irqreturn_t interrupt_transfer(struct dw_spi *dws)
 {
 	u16 irq_status, irq_mask = 0x3f;
+	u32 int_level = dws->fifo_len / 2;
+	u32 left;
 
 	irq_status = dw_readw(dws, isr) & irq_mask;
 	/* Error handling */

@@ -369,22 +371,23 @@ static irqreturn_t interrupt_transfer(struct dw_spi *dws)
 		return IRQ_HANDLED;
 	}
 
-	/* INT comes from tx */
-	if (dws->tx && (irq_status & SPI_INT_TXEI)) {
-		while (dws->tx < dws->tx_end)
+	if (irq_status & SPI_INT_TXEI) {
+		spi_mask_intr(dws, SPI_INT_TXEI);
+
+		left = (dws->tx_end - dws->tx) / dws->n_bytes;
+		left = (left > int_level) ? int_level : left;
+
+		while (left--)
 			dws->write(dws);
+		dws->read(dws);
 
-		if (dws->tx == dws->tx_end) {
-			spi_mask_intr(dws, SPI_INT_TXEI);
+		/* Re-enable the IRQ if there is still data left to tx */
+		if (dws->tx_end > dws->tx)
+			spi_umask_intr(dws, SPI_INT_TXEI);
+		else
 			transfer_complete(dws);
-		}
 	}
 
-	/* INT comes from rx */
-	if (dws->rx && (irq_status & SPI_INT_RXFI)) {
-		if (dws->read(dws))
-			transfer_complete(dws);
-	}
 	return IRQ_HANDLED;
 }

@@ -428,6 +431,7 @@ static void pump_transfers(unsigned long data)
 	u8 bits = 0;
 	u8 imask = 0;
 	u8 cs_change = 0;
+	u16 txint_level = 0;
 	u16 clk_div = 0;
 	u32 speed = 0;
 	u32 cr0 = 0;

@@ -438,6 +442,9 @@ static void pump_transfers(unsigned long data)
 	chip = dws->cur_chip;
 	spi = message->spi;
 
+	if (unlikely(!chip->clk_div))
+		chip->clk_div = dws->max_freq / chip->speed_hz;
+
 	if (message->state == ERROR_STATE) {
 		message->status = -EIO;
 		goto early_exit;

@@ -492,7 +499,7 @@ static void pump_transfers(unsigned long data)
 
 			/* clk_div doesn't support odd number */
 			clk_div = dws->max_freq / speed;
-			clk_div = (clk_div >> 1) << 1;
+			clk_div = (clk_div + 1) & 0xfffe;
 
 			chip->speed_hz = speed;
 			chip->clk_div = clk_div;

@@ -535,11 +542,16 @@ static void pump_transfers(unsigned long data)
 	/* Check if current transfer is a DMA transaction */
 	dws->dma_mapped = map_dma_buffers(dws);
 
+	/*
+	 * Interrupt mode
+	 * we only need set the TXEI IRQ, as TX/RX always happen syncronizely
+	 */
 	if (!dws->dma_mapped && !chip->poll_mode) {
-		if (dws->rx)
-			imask |= SPI_INT_RXFI;
-		if (dws->tx)
-			imask |= SPI_INT_TXEI;
+		int templen = dws->len / dws->n_bytes;
+		txint_level = dws->fifo_len / 2;
+		txint_level = (templen > txint_level) ? txint_level : templen;
+
+		imask |= SPI_INT_TXEI;
 		dws->transfer_handler = interrupt_transfer;
 	}

@@ -549,21 +561,23 @@ static void pump_transfers(unsigned long data)
 	 *	2. clk_div is changed
 	 *	3. control value changes
 	 */
-	if (dw_readw(dws, ctrl0) != cr0 || cs_change || clk_div) {
+	if (dw_readw(dws, ctrl0) != cr0 || cs_change || clk_div || imask) {
 		spi_enable_chip(dws, 0);
 
 		if (dw_readw(dws, ctrl0) != cr0)
 			dw_writew(dws, ctrl0, cr0);
 
+		spi_set_clk(dws, clk_div ? clk_div : chip->clk_div);
+		spi_chip_sel(dws, spi->chip_select);
+
 		/* Set the interrupt mask, for poll mode just diable all int */
 		spi_mask_intr(dws, 0xff);
-		if (!chip->poll_mode)
+		if (imask)
 			spi_umask_intr(dws, imask);
+		if (txint_level)
+			dw_writew(dws, txfltr, txint_level);
 
-		spi_set_clk(dws, clk_div ? clk_div : chip->clk_div);
-		spi_chip_sel(dws, spi->chip_select);
 		spi_enable_chip(dws, 1);
-
 		if (cs_change)
 			dws->prev_chip = chip;
 	}

@@ -712,11 +726,11 @@ static int dw_spi_setup(struct spi_device *spi)
 	}
 	chip->bits_per_word = spi->bits_per_word;
 
+	if (!spi->max_speed_hz) {
+		dev_err(&spi->dev, "No max speed HZ parameter\n");
+		return -EINVAL;
+	}
 	chip->speed_hz = spi->max_speed_hz;
-	if (chip->speed_hz)
-		chip->clk_div = 25000000 / chip->speed_hz;
-	else
-		chip->clk_div = 8;	/* default value */
 
 	chip->tmode = 0; /* Tx & Rx */
 	/* Default SPI mode is SCPOL = 0, SCPH = 0 */

drivers/spi/dw_spi_pci.c +1 -0

index 7980f14..1f0735f 100644
@@ -73,6 +73,7 @@ static int __devinit spi_pci_probe(struct pci_dev *pdev,
 	dws->num_cs = 4;
 	dws->max_freq = 25000000;	/* for Moorestwon */
 	dws->irq = pdev->irq;
+	dws->fifo_len = 40;		/* FIFO has 40 words buffer */
 
 	ret = dw_spi_add_host(dws);
 	if (ret)

include/linux/spi/dw_spi.h +1 -0

index 51b3e77..1a127a3 100644
@@ -90,6 +90,7 @@ struct dw_spi {
 	unsigned long		paddr;
 	u32			iolen;
 	int			irq;
+	u32			fifo_len;	/* depth of the FIFO buffer */
 	u32			max_freq;	/* max bus freq supported */
 
 	u16			bus_num;

Signature · 8 lines

1.6.0.4

------------------------------------------------------------------------------
Throughout its 18-year history, RSA Conference consistently attracts the
world's best and brightest in the field, creating opportunities for Conference
attendees to learn about information security's most important issues through
interactions with peers, luminaries and emerging and established companies.
http://p.sf.net/sfu/rsaconf-dev2dev

Jean-Hugues Deschenes

2010-01-19 14:20 UTC #1 lore raw

Hi Feng,

Review

Quoted 8 lines · Feng Tang

(quoted diff) +1 -0

 	dws->num_cs = 4;
 	dws->max_freq = 25000000;	/* for Moorestwon */
 	dws->irq = pdev->irq;
+	dws->fifo_len = 40;		/* FIFO has 40 words buffer */
 
 	ret = dw_spi_add_host(dws);

Since we'll be sharing some of our init code, as well as some of our init values, would it be a good idea to move:
"dws->num_cs = 4;" and "dws->fifo_len = 40;"

into dw_spi_add_host(), if no values were provided by the caller?

example:
--------------------------- [PATCH] ------------------

drivers/spi/dw_spi.c +5 -0

index 31620fa..01ad79a 100644
@@ -832,6 +832,11 @@ int __devinit dw_spi_add_host(struct dw_spi *dws)
                goto exit;
        }
 
+       if (!dws->num_cs)
+               dws->num_cs = 4;
+       if (!dws->fifo_len)
+               dws->fifo_len = 40;
+      
        dws->master = master;
        dws->type = SSI_MOTO_SPI;
        dws->prev_chip = NULL;

------------------------------------------------------------------------------
Throughout its 18-year history, RSA Conference consistently attracts the world's best and brightest in the field, creating opportunities for Conference attendees to learn about information security's most important issues through interactions with peers, luminaries and emerging and established companies.
http://p.sf.net/sfu/rsaconf-dev2dev

A message that is not in the archive (sent off-list or not yet received): [email protected]

Feng Tang

author

2010-01-20 03:18 UTC #3 lore raw

Quoted 9 lines · On Tue, 19 Jan 2010 22:20:53 +0800 Jean-Hugues Deschenes…

Hi Feng,

Quoted 6 lines · Feng Tang

(quoted diff) +1 -0

 	dws->num_cs = 4;
 	dws->max_freq = 25000000;	/* for Moorestwon */
 	dws->irq = pdev->irq;
+	dws->fifo_len = 40;		/* FIFO has 40 words

buffer */

Quoted 8 lines

Quoted 3 lines
ret = dw_spi_add_host(dws);
Since we'll be sharing some of our init code, as well as some of our init values, would it be a good idea to move:
"dws->num_cs = 4;" and "dws->fifo_len = 40;"
into dw_spi_add_host(), if no values were provided by the caller?

For FIFO depth, it depends on each specific implementation based on DW core, and interface driver would better set it. If fifo_len is not set in IRQ mode, core driver will set 0 as the TX interrupt threshold, which will only trigger the TXE IRQ when the TX FIFO is fully empty. This is my design thought.

For num_cs, I don't think it's a big deal to put its initialization in the interface driver :)

Thanks,
Feng

Jean-Hugues Deschenes

2010-01-20 12:44 UTC #4 lore raw

Quoted 22 lines · Feng Tang

Quoted 18 lines · On Tue, 19 Jan 2010 22:20:53 +0800 Jean-Hugues Deschenes…
Hi Feng,
Quoted 10 lines · Feng Tang
(quoted diff) +1 -0
 	dws->num_cs = 4;
 	dws->max_freq = 25000000;	/* for Moorestwon */
 	dws->irq = pdev->irq;
+	dws->fifo_len = 40;		/* FIFO has 40 words
buffer */
ret = dw_spi_add_host(dws);
Since we'll be sharing some of our init code, as well as some of our init values, would it be a good idea to move:
"dws->num_cs = 4;" and "dws->fifo_len = 40;"

Quoted 8 lines

Quoted 2 lines
into dw_spi_add_host(), if no values were provided by the caller?
For FIFO depth, it depends on each specific implementation based on DW core, and interface driver would better set it. If fifo_len is not set in IRQ mode, core driver will set 0 as the TX interrupt threshold, which will only trigger the TXE IRQ when the TX FIFO is fully empty. This is my design thought.

I'm sorry; At first, I was under the impression that it referred to a software FIFO.
...So could this be auto-detected from the [RT]XFTLR registers, then?

A message that is not in the archive (sent off-list or not yet received): [email protected]

Feng Tang

author

2010-01-21 02:47 UTC #6 lore raw

Quoted 12 lines · On Wed, 20 Jan 2010 20:44:03 +0800 Jean-Hugues Deschenes…

Quoted 8 lines
Quoted 2 lines
For FIFO depth, it depends on each specific implementation based on DW core, and interface driver would better set it. If fifo_len is not set in IRQ mode, core driver will set 0 as the TX interrupt threshold, which will only trigger the TXE IRQ when the TX FIFO is fully empty. This is my design thought.
I'm sorry; At first, I was under the impression that it referred to a software FIFO.
...So could this be auto-detected from the [RT]XFTLR registers, then?

Yep, really a good idea, from the HW spec, the depth could be detected from [RT}XTLR, will submit a patch for this.

Thanks,
Feng