[3/3] spi: s3c64xx: support interrupt based pio mode

Commit Message

Jaewon Kim April 4, 2023, 6 a.m. UTC

This patch adds IRQ based PIO mode instead of cpu polling.
By using the FIFO trigger level, interrupts are received.
CPU consumption is reduced in PIO mode because registers are not
constantly checked.

Signed-off-by: Jaewon Kim <[e-mail]>
---
 drivers/spi/spi-s3c64xx.c | 54 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 6 deletions(-)

Comments

Mark Brown April 4, 2023, 12:58 p.m. UTC | #1

On Tue, Apr 04, 2023 at 03:00:11PM +0900, Jaewon Kim wrote:

> This patch adds IRQ based PIO mode instead of cpu polling.
> By using the FIFO trigger level, interrupts are received.
> CPU consumption is reduced in PIO mode because registers are not
> constantly checked.

Is there some lower limit where it's still worth using polling, for
example for just one or two bytes like a register address?  Taking an
interrupt isn't free...

Jaewon Kim April 4, 2023, 1:15 p.m. UTC | #2

Hello Mark,


On 23. 4. 4. 21:58, Mark Brown wrote:
> On Tue, Apr 04, 2023 at 03:00:11PM +0900, Jaewon Kim wrote:
>
>> This patch adds IRQ based PIO mode instead of cpu polling.
>> By using the FIFO trigger level, interrupts are received.
>> CPU consumption is reduced in PIO mode because registers are not
>> constantly checked.
> Is there some lower limit where it's still worth using polling, for
> example for just one or two bytes like a register address?  Taking an
> interrupt isn't free...


I did not considers lower limit.
According to your review, interrupt seems to be called too often.
However, It can't prevent the CPU utilization going to 100% during spi 
transmission.
We will give more consideration and deliver a better solution to the 
next patch version.


Thanks

Jaewon Kim

Mark Brown April 4, 2023, 1:33 p.m. UTC | #3

On Tue, Apr 04, 2023 at 10:15:05PM +0900, Jaewon Kim wrote:
> On 23. 4. 4. 21:58, Mark Brown wrote:

> > Is there some lower limit where it's still worth using polling, for
> > example for just one or two bytes like a register address?  Taking an
> > interrupt isn't free...

> I did not considers lower limit.
> According to your review, interrupt seems to be called too often.
> However, It can't prevent the CPU utilization going to 100% during spi 
> transmission.

It's not so much that the interrupt could be called too often as that
the time taken to take the interrupt (including all the overhead the CPU
has) might be large compared to what busy waiting would take if the
transfer is very small.  If the FIFO is deep enough and the transfer is
long enough to use that then you start to see a win from interrupts.

> We will give more consideration and deliver a better solution to the 
> next patch version.

Great.

Patch

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index bf1f3dcca202..f8986b05309e 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -59,6 +59,8 @@ 
 #define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD	(1<<17)
 #define S3C64XX_SPI_MODE_BUS_TSZ_WORD		(2<<17)
 #define S3C64XX_SPI_MODE_BUS_TSZ_MASK		(3<<17)
+#define S3C64XX_SPI_MODE_RX_RDY_LVL		GENMASK(16, 11)
+#define S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT	11
 #define S3C64XX_SPI_MODE_SELF_LOOPBACK		(1<<3)
 #define S3C64XX_SPI_MODE_RXDMA_ON		(1<<2)
 #define S3C64XX_SPI_MODE_TXDMA_ON		(1<<1)
@@ -567,6 +569,7 @@  static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 {
 	void __iomem *regs = sdd->regs;
 	unsigned long val;
+	unsigned long time;
 	u32 status;
 	int loops;
 	u32 cpy_len;
@@ -577,6 +580,11 @@  static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 	ms = xfer->len * 8 * 1000 / sdd->cur_speed;
 	ms += 10; /* some tolerance */
 
+	val = msecs_to_jiffies(ms);
+	time = wait_for_completion_timeout(&sdd->xfer_completion, val);
+	if (!time)
+		return -EIO;
+
 	val = msecs_to_loops(ms);
 	do {
 		status = readl(regs + S3C64XX_SPI_STATUS);
@@ -743,6 +751,8 @@  static int s3c64xx_spi_transfer_one(struct spi_master *master,
 	u32 speed;
 	u8 bpw;
 	unsigned long flags;
+	u32 rdy_lv;
+	u32 val;
 
 	reinit_completion(&sdd->xfer_completion);
 
@@ -763,17 +773,41 @@  static int s3c64xx_spi_transfer_one(struct spi_master *master,
 	    sdd->rx_dma.ch && sdd->tx_dma.ch) {
 		use_dma = 1;
 
-	} else if (xfer->len > fifo_len) {
+	} else if (xfer->len >= fifo_len) {
 		tx_buf = xfer->tx_buf;
 		rx_buf = xfer->rx_buf;
 		origin_len = xfer->len;
-
 		target_len = xfer->len;
-		if (xfer->len > fifo_len)
-			xfer->len = fifo_len;
+		xfer->len = fifo_len - 1;
 	}
 
 	do {
+		if (!use_dma) {
+			reinit_completion(&sdd->xfer_completion);
+
+			rdy_lv = xfer->len;
+			/* Setup RDY_FIFO trigger Level
+			 * RDY_LVL =
+			 * fifo_lvl upto 64 byte -> N bytes
+			 *              128 byte -> RDY_LVL * 2 bytes
+			 *              256 byte -> RDY_LVL * 4 bytes
+			 */
+			if (fifo_len == 128)
+				rdy_lv /= 2;
+			else if (fifo_len == 256)
+				rdy_lv /= 4;
+
+			val = readl(sdd->regs + S3C64XX_SPI_MODE_CFG);
+			val &= ~S3C64XX_SPI_MODE_RX_RDY_LVL;
+			val |= (rdy_lv << S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT);
+			writel(val, sdd->regs + S3C64XX_SPI_MODE_CFG);
+
+			/* Enable FIFO_RDY_EN IRQ */
+			val = readl(sdd->regs + S3C64XX_SPI_INT_EN);
+			writel((val | S3C64XX_SPI_INT_RX_FIFORDY_EN),
+					sdd->regs + S3C64XX_SPI_INT_EN);
+
+		}
 		spin_lock_irqsave(&sdd->lock, flags);
 
 		/* Pending only which is to be done */
@@ -834,8 +868,8 @@  static int s3c64xx_spi_transfer_one(struct spi_master *master,
 			if (xfer->rx_buf)
 				xfer->rx_buf += xfer->len;
 
-			if (target_len > fifo_len)
-				xfer->len = fifo_len;
+			if (target_len >= fifo_len)
+				xfer->len = fifo_len - 1;
 			else
 				xfer->len = target_len;
 		}
@@ -1005,6 +1039,14 @@  static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
 		dev_err(&spi->dev, "TX underrun\n");
 	}
 
+	if (val & S3C64XX_SPI_ST_RX_FIFORDY) {
+		complete(&sdd->xfer_completion);
+		/* No pending clear irq, turn-off INT_EN_RX_FIFO_RDY */
+		val = readl(sdd->regs + S3C64XX_SPI_INT_EN);
+		writel((val & ~S3C64XX_SPI_INT_RX_FIFORDY_EN),
+				sdd->regs + S3C64XX_SPI_INT_EN);
+	}
+
 	/* Clear the pending irq by setting and then clearing it */
 	writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
 	writel(0, sdd->regs + S3C64XX_SPI_PENDING_CLR);