[RFC 1/2] mmc: davinci: Eliminate spurious interrupts

[RFC 1/2] mmc: davinci: Eliminate spurious interrupts

[Ido Yariv]

The davinci mmc interrupt handler fills the fifo, as long as the DXRDY
or DRRDY bits are set in the status register.

If interrupts fire during this loop, they will be handled by the
handler, but the interrupt controller will still buffer these. As a
result, the handler will be called again to serve these needlessly. In
order to avoid these spurious interrupts, keep interrupts masked while
filling the fifo.

Signed-off-by: Ido Yariv <ido@wizery.com>
---
 drivers/mmc/host/davinci_mmc.c |   33 +++++++++++++++++++++++++++------
 1 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index 64a8325..9cea66f 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -1009,12 +1009,33 @@ static irqreturn_t mmc_davinci_irq(int irq, void *dev_id)
 	 * by read. So, it is not unbouned loop even in the case of
 	 * non-dma.
 	 */
-	while (host->bytes_left && (status & (MMCST0_DXRDY | MMCST0_DRRDY))) {
-		davinci_fifo_data_trans(host, rw_threshold);
-		status = readl(host->base + DAVINCI_MMCST0);
-		if (!status)
-			break;
-		qstatus |= status;
+	if (host->bytes_left && (status & (MMCST0_DXRDY | MMCST0_DRRDY))) {
+		unsigned long im_val;
+
+		/*
+		 * If interrupts fire during the following loop, they will be
+		 * handled by the handler, but the PIC will still buffer these.
+		 * As a result, the handler will be called again to serve these
+		 * needlessly. In order to avoid these spurious interrupts,
+		 * keep interrupts masked during the loop.
+		 */
+		im_val = readl(host->base + DAVINCI_MMCIM);
+		writel(0, host->base + DAVINCI_MMCIM);
+
+		do {
+			davinci_fifo_data_trans(host, rw_threshold);
+			status = readl(host->base + DAVINCI_MMCST0);
+			qstatus |= status;
+		} while (host->bytes_left &&
+			 (status & (MMCST0_DXRDY | MMCST0_DRRDY)));
+
+		/*
+		 * If an interrupt is pending, it is assumed it will fire when
+		 * it is unmasked. This assumption is also taken when the MMCIM
+		 * is first set. Otherwise, writing to MMCIM after reading the
+		 * status is race-prone.
+		 */
+		writel(im_val, host->base + DAVINCI_MMCIM);
 	}
 
 	if (qstatus & MMCST0_DATDNE) {
-- 
1.7.7.5

[RFC 2/2] mmc: davinci: Poll status for small size transfers

[Ido Yariv]

For small size non-dma sdio transactions, it is sometimes better to poll
the mmc host and avoid interrupts altogether. Polling lowers the number
of interrupts and context switches. Tests have shown that for small
transactions, only a few polling iterations are needed, so this is worth
while.

Signed-off-by: Ido Yariv <ido@wizery.com>
---
 drivers/mmc/host/davinci_mmc.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index 9cea66f..fb1368a 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -160,6 +160,16 @@ module_param(rw_threshold, uint, S_IRUGO);
 MODULE_PARM_DESC(rw_threshold,
 		"Read/Write threshold. Default = 32");
 
+static unsigned poll_threshold = 128;
+module_param(poll_threshold, uint, S_IRUGO);
+MODULE_PARM_DESC(poll_threshold,
+		 "Polling transaction size threshold. Default = 128");
+
+static unsigned poll_loopcount = 32;
+module_param(poll_loopcount, uint, S_IRUGO);
+MODULE_PARM_DESC(poll_loopcount,
+		 "Maximum polling loop count. Default = 32");
+
 static unsigned __initdata use_dma = 1;
 module_param(use_dma, uint, 0);
 MODULE_PARM_DESC(use_dma, "Whether to use DMA or not. Default = 1");
@@ -193,6 +203,7 @@ struct mmc_davinci_host {
 	bool use_dma;
 	bool do_dma;
 	bool sdio_int;
+	bool active_request;
 
 	/* Scatterlist DMA uses one or more parameter RAM entries:
 	 * the main one (associated with rxdma or txdma) plus zero or
@@ -219,6 +230,7 @@ struct mmc_davinci_host {
 #endif
 };
 
+static irqreturn_t mmc_davinci_irq(int irq, void *dev_id);
 
 /* PIO only */
 static void mmc_davinci_sg_to_buf(struct mmc_davinci_host *host)
@@ -376,7 +388,20 @@ static void mmc_davinci_start_command(struct mmc_davinci_host *host,
 
 	writel(cmd->arg, host->base + DAVINCI_MMCARGHL);
 	writel(cmd_reg,  host->base + DAVINCI_MMCCMD);
-	writel(im_val, host->base + DAVINCI_MMCIM);
+
+	host->active_request = true;
+
+	if (!host->do_dma && host->bytes_left <= poll_threshold) {
+		u32 count = poll_loopcount;
+
+		while (host->active_request && count--) {
+			mmc_davinci_irq(0, host);
+			cpu_relax();
+		}
+	}
+
+	if (host->active_request)
+		writel(im_val, host->base + DAVINCI_MMCIM);
 }
 
 /*----------------------------------------------------------------------*/
@@ -915,6 +940,7 @@ mmc_davinci_xfer_done(struct mmc_davinci_host *host, struct mmc_data *data)
 	if (!data->stop || (host->cmd && host->cmd->error)) {
 		mmc_request_done(host->mmc, data->mrq);
 		writel(0, host->base + DAVINCI_MMCIM);
+		host->active_request = false;
 	} else
 		mmc_davinci_start_command(host, data->stop);
 }
@@ -942,6 +968,7 @@ static void mmc_davinci_cmd_done(struct mmc_davinci_host *host,
 			cmd->mrq->cmd->retries = 0;
 		mmc_request_done(host->mmc, cmd->mrq);
 		writel(0, host->base + DAVINCI_MMCIM);
+		host->active_request = false;
 	}
 }
 
-- 
1.7.7.5

RE: [RFC 1/2] mmc: davinci: Eliminate spurious interrupts

[Rajashekhara, Sudhakar]

Hello Ido,

On Tue, Jan 24, 2012 at 16:46:05, Ido Yariv wrote:
> The davinci mmc interrupt handler fills the fifo, as long as the DXRDY
> or DRRDY bits are set in the status register.
> 
> If interrupts fire during this loop, they will be handled by the
> handler, but the interrupt controller will still buffer these. As a
> result, the handler will be called again to serve these needlessly. In
> order to avoid these spurious interrupts, keep interrupts masked while
> filling the fifo.
> 

I tested both these patches and they work fine on the OMAP-L138 EVM. I
observed that with these patches the number of interrupts during a transfer
are less compared to earlier. For a 100 MB transfer, I could see around
700-800 interrupts less. Did you see any performance improvement with these
patches?

Thanks,
Sudhakar

Re: [RFC 1/2] mmc: davinci: Eliminate spurious interrupts

[Ido Yariv]

Hello Sudhakar,

On Fri, Jan 27, 2012 at 08:11:55AM +0000, Rajashekhara, Sudhakar wrote:
> I tested both these patches and they work fine on the OMAP-L138 EVM. I
> observed that with these patches the number of interrupts during a transfer
> are less compared to earlier. For a 100 MB transfer, I could see around
> 700-800 interrupts less. Did you see any performance improvement with these
> patches?

Thanks for testing these.

The difference in the number of interrupts, or rather the ratio, depends
on the scenario you're working on.
The spurious patch has an effect on non-dma transfers, while the polling
optimization affects small (i.e. up to 128 bytes) transactions.
I expect these to have a greater impact on SDIO scenarios than on SD scenarios.

Regarding the performance improvement, these patches were tested using a
WLAN SDIO adapter, which requires quite a few small transactions.
The polling optimization had an impact of around 10-15% in throughput.
The spurious patch did not have any noticeable effect on throughput, but did
lower the number of interrupts by up to 25% in some cases.

Thanks,
Ido.

RE: [RFC 1/2] mmc: davinci: Eliminate spurious interrupts

[Rajashekhara, Sudhakar]

Hi,

On Tue, Jan 24, 2012 at 16:46:05, Ido Yariv wrote:
> The davinci mmc interrupt handler fills the fifo, as long as the DXRDY
> or DRRDY bits are set in the status register.
> 
> If interrupts fire during this loop, they will be handled by the
> handler, but the interrupt controller will still buffer these. As a
> result, the handler will be called again to serve these needlessly. In
> order to avoid these spurious interrupts, keep interrupts masked while
> filling the fifo.
> 
> Signed-off-by: Ido Yariv <ido-Ix1uc/W3ht7QT0dZR+AlfA@public.gmane.org>
> ---

Tested this series on OMAP-L138 EVM using SD card. Observed that number of
interrupts during IO transaction were less compared to earlier and there
was no drop in performance numbers.

Tested-by: Rajashekhara, Sudhakar <sudhakar.raj-l0cyMroinI0@public.gmane.org>

Regards,
Sudhakar

[PATCH REPOST 1/2] mmc: davinci: Eliminate spurious interrupts

[Ido Yariv]

The davinci mmc interrupt handler fills the fifo, as long as the DXRDY
or DRRDY bits are set in the status register.

If interrupts fire during this loop, they will be handled by the
handler, but the interrupt controller will still buffer these. As a
result, the handler will be called again to serve these needlessly. In
order to avoid these spurious interrupts, keep interrupts masked while
filling the fifo.

Signed-off-by: Ido Yariv <ido@wizery.com>
Tested-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>
---
 drivers/mmc/host/davinci_mmc.c |   33 +++++++++++++++++++++++++++------
 1 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index 64a8325..9cea66f 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -1009,12 +1009,33 @@ static irqreturn_t mmc_davinci_irq(int irq, void *dev_id)
 	 * by read. So, it is not unbouned loop even in the case of
 	 * non-dma.
 	 */
-	while (host->bytes_left && (status & (MMCST0_DXRDY | MMCST0_DRRDY))) {
-		davinci_fifo_data_trans(host, rw_threshold);
-		status = readl(host->base + DAVINCI_MMCST0);
-		if (!status)
-			break;
-		qstatus |= status;
+	if (host->bytes_left && (status & (MMCST0_DXRDY | MMCST0_DRRDY))) {
+		unsigned long im_val;
+
+		/*
+		 * If interrupts fire during the following loop, they will be
+		 * handled by the handler, but the PIC will still buffer these.
+		 * As a result, the handler will be called again to serve these
+		 * needlessly. In order to avoid these spurious interrupts,
+		 * keep interrupts masked during the loop.
+		 */
+		im_val = readl(host->base + DAVINCI_MMCIM);
+		writel(0, host->base + DAVINCI_MMCIM);
+
+		do {
+			davinci_fifo_data_trans(host, rw_threshold);
+			status = readl(host->base + DAVINCI_MMCST0);
+			qstatus |= status;
+		} while (host->bytes_left &&
+			 (status & (MMCST0_DXRDY | MMCST0_DRRDY)));
+
+		/*
+		 * If an interrupt is pending, it is assumed it will fire when
+		 * it is unmasked. This assumption is also taken when the MMCIM
+		 * is first set. Otherwise, writing to MMCIM after reading the
+		 * status is race-prone.
+		 */
+		writel(im_val, host->base + DAVINCI_MMCIM);
 	}
 
 	if (qstatus & MMCST0_DATDNE) {
-- 
1.7.7.6

[PATCH REPOST 2/2] mmc: davinci: Poll status for small size transfers

[Ido Yariv]

For small size non-dma sdio transactions, it is sometimes better to poll
the mmc host and avoid interrupts altogether. Polling lowers the number
of interrupts and context switches. Tests have shown that for small
transactions, only a few polling iterations are needed, so this is worth
while.

Signed-off-by: Ido Yariv <ido@wizery.com>
Tested-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>
---
 drivers/mmc/host/davinci_mmc.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index 9cea66f..fb1368a 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -160,6 +160,16 @@ module_param(rw_threshold, uint, S_IRUGO);
 MODULE_PARM_DESC(rw_threshold,
 		"Read/Write threshold. Default = 32");
 
+static unsigned poll_threshold = 128;
+module_param(poll_threshold, uint, S_IRUGO);
+MODULE_PARM_DESC(poll_threshold,
+		 "Polling transaction size threshold. Default = 128");
+
+static unsigned poll_loopcount = 32;
+module_param(poll_loopcount, uint, S_IRUGO);
+MODULE_PARM_DESC(poll_loopcount,
+		 "Maximum polling loop count. Default = 32");
+
 static unsigned __initdata use_dma = 1;
 module_param(use_dma, uint, 0);
 MODULE_PARM_DESC(use_dma, "Whether to use DMA or not. Default = 1");
@@ -193,6 +203,7 @@ struct mmc_davinci_host {
 	bool use_dma;
 	bool do_dma;
 	bool sdio_int;
+	bool active_request;
 
 	/* Scatterlist DMA uses one or more parameter RAM entries:
 	 * the main one (associated with rxdma or txdma) plus zero or
@@ -219,6 +230,7 @@ struct mmc_davinci_host {
 #endif
 };
 
+static irqreturn_t mmc_davinci_irq(int irq, void *dev_id);
 
 /* PIO only */
 static void mmc_davinci_sg_to_buf(struct mmc_davinci_host *host)
@@ -376,7 +388,20 @@ static void mmc_davinci_start_command(struct mmc_davinci_host *host,
 
 	writel(cmd->arg, host->base + DAVINCI_MMCARGHL);
 	writel(cmd_reg,  host->base + DAVINCI_MMCCMD);
-	writel(im_val, host->base + DAVINCI_MMCIM);
+
+	host->active_request = true;
+
+	if (!host->do_dma && host->bytes_left <= poll_threshold) {
+		u32 count = poll_loopcount;
+
+		while (host->active_request && count--) {
+			mmc_davinci_irq(0, host);
+			cpu_relax();
+		}
+	}
+
+	if (host->active_request)
+		writel(im_val, host->base + DAVINCI_MMCIM);
 }
 
 /*----------------------------------------------------------------------*/
@@ -915,6 +940,7 @@ mmc_davinci_xfer_done(struct mmc_davinci_host *host, struct mmc_data *data)
 	if (!data->stop || (host->cmd && host->cmd->error)) {
 		mmc_request_done(host->mmc, data->mrq);
 		writel(0, host->base + DAVINCI_MMCIM);
+		host->active_request = false;
 	} else
 		mmc_davinci_start_command(host, data->stop);
 }
@@ -942,6 +968,7 @@ static void mmc_davinci_cmd_done(struct mmc_davinci_host *host,
 			cmd->mrq->cmd->retries = 0;
 		mmc_request_done(host->mmc, cmd->mrq);
 		writel(0, host->base + DAVINCI_MMCIM);
+		host->active_request = false;
 	}
 }
 
-- 
1.7.7.6

Re: [PATCH REPOST 1/2] mmc: davinci: Eliminate spurious interrupts

[Chris Ball]

Hi,

On Sun, Mar 11 2012, Ido Yariv wrote:
> The davinci mmc interrupt handler fills the fifo, as long as the DXRDY
> or DRRDY bits are set in the status register.
>
> If interrupts fire during this loop, they will be handled by the
> handler, but the interrupt controller will still buffer these. As a
> result, the handler will be called again to serve these needlessly. In
> order to avoid these spurious interrupts, keep interrupts masked while
> filling the fifo.
>
> Signed-off-by: Ido Yariv <ido@wizery.com>
> Tested-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>

Thanks, I've pushed both of these two patches to mmc-next for 3.4 now.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child