Hi all,
Currently the sdhci driver does everything in the atomic context.
And what is worse, PIO transfers are made from the IRQ handler.
This causes huge latencies (up to 120 ms). On some P2020 SOCs,
DMA and card detection is broken, which means that kernel polls
for the card via PIO transfers every second. Needless to say
that this is quite bad.
So, this patch set reworks sdhci code to avoid atomic context,
almost completely. We only do two device memory operations
in the atomic context, and all the rest is threaded.
I noticed no throughput drop neither with PIO transfers nor
with DMA (tested on MPC8569E CPU), while latencies should be
greatly improved.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
There is no need for the timeout handler to run in the atomic
context, so this patch turns timeout timeout into the delayed
work.
Note that the timeout handler still grabs an irqsave spinlock,
we'll deal with it in a separate patch.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/mmc/host/sdhci.c | 14 ++++++++------
drivers/mmc/host/sdhci.h | 3 ++-
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..dc6328c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -13,6 +13,8 @@
* - JMicron (hardware and technical support)
*/
+#include <linux/kernel.h>
+#include <linux/workqueue.h>
#include <linux/delay.h>
#include <linux/highmem.h>
#include <linux/io.h>
@@ -906,7 +908,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
mdelay(1);
}
- mod_timer(&host->timer, jiffies + 10 * HZ);
+ schedule_delayed_work(&host->timeout_work, 10 * HZ);
host->cmd = cmd;
@@ -1280,7 +1282,7 @@ static void sdhci_tasklet_finish(unsigned long param)
spin_lock_irqsave(&host->lock, flags);
- del_timer(&host->timer);
+ __cancel_delayed_work(&host->timeout_work);
mrq = host->mrq;
@@ -1324,12 +1326,12 @@ static void sdhci_tasklet_finish(unsigned long param)
mmc_request_done(host->mmc, mrq);
}
-static void sdhci_timeout_timer(unsigned long data)
+static void sdhci_timeout_work(struct work_struct *wk)
{
struct sdhci_host *host;
unsigned long flags;
- host = (struct sdhci_host*)data;
+ host = container_of(wk, struct sdhci_host, timeout_work.work);
spin_lock_irqsave(&host->lock, flags);
@@ -1877,7 +1879,7 @@ int sdhci_add_host(struct sdhci_host *host)
tasklet_init(&host->finish_tasklet,
sdhci_tasklet_finish, (unsigned long)host);
- setup_timer(&host->timer, sdhci_timeout_timer, (unsigned long)host);
+ INIT_DELAYED_WORK(&host->timeout_work, sdhci_timeout_work);
ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
mmc_hostname(mmc), host);
@@ -1963,7 +1965,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
free_irq(host->irq, host);
- del_timer_sync(&host->timer);
+ flush_delayed_work(&host->timeout_work);
tasklet_kill(&host->card_tasklet);
tasklet_kill(&host->finish_tasklet);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..55c114d 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -12,6 +12,7 @@
#define __SDHCI_H
#include <linux/scatterlist.h>
+#include <linux/workqueue.h>
#include <linux/compiler.h>
#include <linux/types.h>
#include <linux/io.h>
@@ -290,7 +291,7 @@ struct sdhci_host {
struct tasklet_struct card_tasklet; /* Tasklet structures */
struct tasklet_struct finish_tasklet;
- struct timer_list timer; /* Timer for timeouts */
+ struct delayed_work timeout_work; /* Work for timeouts */
unsigned long private[0] ____cacheline_aligned;
};
--
1.7.0.5
The driver can happily live without an atomic context and tasklets,
so turn the tasklets into the work structs.
Tasklets handlers still grab irqsave spinlocks, but we'll deal
with it in a separate patch.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/mmc/host/sdhci.c | 48 ++++++++++++++++++++-------------------------
drivers/mmc/host/sdhci.h | 4 +-
2 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index dc6328c..748a2e3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -872,7 +872,7 @@ static void sdhci_finish_data(struct sdhci_host *host)
sdhci_send_command(host, data->stop);
} else
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
}
static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
@@ -901,7 +901,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
"inhibit bit(s).\n", mmc_hostname(host->mmc));
sdhci_dumpregs(host);
cmd->error = -EIO;
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
return;
}
timeout--;
@@ -922,7 +922,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
printk(KERN_ERR "%s: Unsupported response type!\n",
mmc_hostname(host->mmc));
cmd->error = -EINVAL;
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
return;
}
@@ -973,7 +973,7 @@ static void sdhci_finish_command(struct sdhci_host *host)
sdhci_finish_data(host);
if (!host->cmd->data)
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
host->cmd = NULL;
}
@@ -1122,7 +1122,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
if (!present || host->flags & SDHCI_DEVICE_DEAD) {
host->mrq->cmd->error = -ENOMEDIUM;
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
} else
sdhci_send_command(host, mrq->cmd);
@@ -1239,16 +1239,16 @@ static const struct mmc_host_ops sdhci_ops = {
/*****************************************************************************\
* *
- * Tasklets *
+ * Work handlers *
* *
\*****************************************************************************/
-static void sdhci_tasklet_card(unsigned long param)
+static void sdhci_card_detect_work(struct work_struct *wk)
{
struct sdhci_host *host;
unsigned long flags;
- host = (struct sdhci_host*)param;
+ host = container_of(wk, struct sdhci_host, card_detect_work);
spin_lock_irqsave(&host->lock, flags);
@@ -1263,7 +1263,7 @@ static void sdhci_tasklet_card(unsigned long param)
sdhci_reset(host, SDHCI_RESET_DATA);
host->mrq->cmd->error = -ENOMEDIUM;
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
}
}
@@ -1272,13 +1272,13 @@ static void sdhci_tasklet_card(unsigned long param)
mmc_detect_change(host->mmc, msecs_to_jiffies(200));
}
-static void sdhci_tasklet_finish(unsigned long param)
+static void sdhci_finish_work(struct work_struct *wk)
{
struct sdhci_host *host;
unsigned long flags;
struct mmc_request *mrq;
- host = (struct sdhci_host*)param;
+ host = container_of(wk, struct sdhci_host, finish_work);
spin_lock_irqsave(&host->lock, flags);
@@ -1349,7 +1349,7 @@ static void sdhci_timeout_work(struct work_struct *wk)
else
host->mrq->cmd->error = -ETIMEDOUT;
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
}
}
@@ -1382,7 +1382,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
host->cmd->error = -EILSEQ;
if (host->cmd->error) {
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
return;
}
@@ -1528,7 +1528,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
sdhci_writel(host, intmask & (SDHCI_INT_CARD_INSERT |
SDHCI_INT_CARD_REMOVE), SDHCI_INT_STATUS);
- tasklet_schedule(&host->card_tasklet);
+ schedule_work(&host->card_detect_work);
}
intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
@@ -1872,19 +1872,17 @@ int sdhci_add_host(struct sdhci_host *host)
mmc->max_blk_count = (host->quirks & SDHCI_QUIRK_NO_MULTIBLOCK) ? 1 : 65535;
/*
- * Init tasklets.
+ * Init work structs.
*/
- tasklet_init(&host->card_tasklet,
- sdhci_tasklet_card, (unsigned long)host);
- tasklet_init(&host->finish_tasklet,
- sdhci_tasklet_finish, (unsigned long)host);
+ INIT_WORK(&host->card_detect_work, sdhci_card_detect_work);
+ INIT_WORK(&host->finish_work, sdhci_finish_work);
INIT_DELAYED_WORK(&host->timeout_work, sdhci_timeout_work);
ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
mmc_hostname(mmc), host);
if (ret)
- goto untasklet;
+ return ret;
sdhci_init(host, 0);
@@ -1923,10 +1921,6 @@ reset:
sdhci_reset(host, SDHCI_RESET_ALL);
free_irq(host->irq, host);
#endif
-untasklet:
- tasklet_kill(&host->card_tasklet);
- tasklet_kill(&host->finish_tasklet);
-
return ret;
}
@@ -1946,7 +1940,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
" transfer!\n", mmc_hostname(host->mmc));
host->mrq->cmd->error = -ENOMEDIUM;
- tasklet_schedule(&host->finish_tasklet);
+ schedule_work(&host->finish_work);
}
spin_unlock_irqrestore(&host->lock, flags);
@@ -1967,8 +1961,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
flush_delayed_work(&host->timeout_work);
- tasklet_kill(&host->card_tasklet);
- tasklet_kill(&host->finish_tasklet);
+ flush_work(&host->card_detect_work);
+ flush_work(&host->finish_work);
kfree(host->adma_desc);
kfree(host->align_buffer);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 55c114d..d96e4dd 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -288,8 +288,8 @@ struct sdhci_host {
dma_addr_t adma_addr; /* Mapped ADMA descr. table */
dma_addr_t align_addr; /* Mapped bounce buffer */
- struct tasklet_struct card_tasklet; /* Tasklet structures */
- struct tasklet_struct finish_tasklet;
+ struct work_struct card_detect_work;
+ struct work_struct finish_work;
struct delayed_work timeout_work; /* Work for timeouts */
--
1.7.0.5
Finally, we can get rid of the host->lock spinlock, and turn it
into a mutex.
This patch does just this.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/mmc/host/sdhci.c | 53 +++++++++++++++++++--------------------------
drivers/mmc/host/sdhci.h | 3 +-
2 files changed, 24 insertions(+), 32 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0358b35..5eddbdb 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -20,6 +20,7 @@
#include <linux/io.h>
#include <linux/dma-mapping.h>
#include <linux/slab.h>
+#include <linux/mutex.h>
#include <linux/scatterlist.h>
#include <linux/leds.h>
@@ -228,16 +229,15 @@ static void sdhci_led_control(struct led_classdev *led,
enum led_brightness brightness)
{
struct sdhci_host *host = container_of(led, struct sdhci_host, led);
- unsigned long flags;
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
if (brightness == LED_OFF)
sdhci_deactivate_led(host);
else
sdhci_activate_led(host);
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
}
#endif
@@ -1099,11 +1099,10 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
{
struct sdhci_host *host;
bool present;
- unsigned long flags;
host = mmc_priv(mmc);
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
WARN_ON(host->mrq != NULL);
@@ -1127,18 +1126,17 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
sdhci_send_command(host, mrq->cmd);
mmiowb();
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
}
static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
{
struct sdhci_host *host;
- unsigned long flags;
u8 ctrl;
host = mmc_priv(mmc);
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
if (host->flags & SDHCI_DEVICE_DEAD)
goto out;
@@ -1183,25 +1181,24 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
out:
mmiowb();
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
}
static int sdhci_get_ro(struct mmc_host *mmc)
{
struct sdhci_host *host;
- unsigned long flags;
int present;
host = mmc_priv(mmc);
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
if (host->flags & SDHCI_DEVICE_DEAD)
present = 0;
else
present = sdhci_readl(host, SDHCI_PRESENT_STATE);
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
if (host->quirks & SDHCI_QUIRK_INVERTED_WRITE_PROTECT)
return !!(present & SDHCI_WRITE_PROTECT);
@@ -1211,11 +1208,10 @@ static int sdhci_get_ro(struct mmc_host *mmc)
static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
{
struct sdhci_host *host;
- unsigned long flags;
host = mmc_priv(mmc);
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
if (host->flags & SDHCI_DEVICE_DEAD)
goto out;
@@ -1227,7 +1223,7 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
out:
mmiowb();
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
}
static const struct mmc_host_ops sdhci_ops = {
@@ -1246,11 +1242,10 @@ static const struct mmc_host_ops sdhci_ops = {
static void sdhci_card_detect_work(struct work_struct *wk)
{
struct sdhci_host *host;
- unsigned long flags;
host = container_of(wk, struct sdhci_host, card_detect_work);
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
if (host->mrq) {
@@ -1267,7 +1262,7 @@ static void sdhci_card_detect_work(struct work_struct *wk)
}
}
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
mmc_detect_change(host->mmc, msecs_to_jiffies(200));
}
@@ -1275,12 +1270,11 @@ static void sdhci_card_detect_work(struct work_struct *wk)
static void sdhci_finish_work(struct work_struct *wk)
{
struct sdhci_host *host;
- unsigned long flags;
struct mmc_request *mrq;
host = container_of(wk, struct sdhci_host, finish_work);
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
__cancel_delayed_work(&host->timeout_work);
@@ -1321,7 +1315,7 @@ static void sdhci_finish_work(struct work_struct *wk)
#endif
mmiowb();
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
mmc_request_done(host->mmc, mrq);
}
@@ -1329,11 +1323,10 @@ static void sdhci_finish_work(struct work_struct *wk)
static void sdhci_timeout_work(struct work_struct *wk)
{
struct sdhci_host *host;
- unsigned long flags;
host = container_of(wk, struct sdhci_host, timeout_work.work);
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
if (host->mrq) {
printk(KERN_ERR "%s: Timeout waiting for hardware "
@@ -1354,7 +1347,7 @@ static void sdhci_timeout_work(struct work_struct *wk)
}
mmiowb();
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
}
/*****************************************************************************\
@@ -1512,7 +1505,7 @@ static irqreturn_t sdhci_irq_thread(int irq, void *dev_id)
u32 intmask;
int cardint = 0;
- spin_lock(&host->lock);
+ mutex_lock(&host->lock);
intmask = sdhci_readl(host, SDHCI_INT_STATUS);
sdhci_writel(host, intmask, SDHCI_INT_STATUS);
@@ -1551,7 +1544,7 @@ static irqreturn_t sdhci_irq_thread(int irq, void *dev_id)
mmiowb();
- spin_unlock(&host->lock);
+ mutex_unlock(&host->lock);
/*
* We have to delay this as it calls back into the driver.
@@ -1816,7 +1809,7 @@ int sdhci_add_host(struct sdhci_host *host)
return -ENODEV;
}
- spin_lock_init(&host->lock);
+ mutex_init(&host->lock);
/*
* Maximum number of segments. Depends on if the hardware
@@ -1926,10 +1919,8 @@ EXPORT_SYMBOL_GPL(sdhci_add_host);
void sdhci_remove_host(struct sdhci_host *host, int dead)
{
- unsigned long flags;
-
if (dead) {
- spin_lock_irqsave(&host->lock, flags);
+ mutex_lock(&host->lock);
host->flags |= SDHCI_DEVICE_DEAD;
@@ -1941,7 +1932,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
schedule_work(&host->finish_work);
}
- spin_unlock_irqrestore(&host->lock, flags);
+ mutex_unlock(&host->lock);
}
sdhci_disable_card_detection(host);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d96e4dd..364d4e8 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -15,6 +15,7 @@
#include <linux/workqueue.h>
#include <linux/compiler.h>
#include <linux/types.h>
+#include <linux/mutex.h>
#include <linux/io.h>
/*
@@ -256,7 +257,7 @@ struct sdhci_host {
char led_name[32];
#endif
- spinlock_t lock; /* Mutex */
+ struct mutex lock; /* Mutex */
int flags; /* Host attributes */
#define SDHCI_USE_SDMA (1<<0) /* Host is SDMA capable */
--
1.7.0.5
Nowadays we can just call the card detection handler directly,
no need for the separate work struct.
We still need host->finish_work as sdhci_finish_work() calls
mmc_request_done(), which tries to re-enter the driver.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/mmc/host/sdhci.c | 14 ++------------
drivers/mmc/host/sdhci.h | 1 -
2 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 5eddbdb..56d5c56 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1239,14 +1239,8 @@ static const struct mmc_host_ops sdhci_ops = {
* *
\*****************************************************************************/
-static void sdhci_card_detect_work(struct work_struct *wk)
+static void sdhci_card_detect(struct sdhci_host *host)
{
- struct sdhci_host *host;
-
- host = container_of(wk, struct sdhci_host, card_detect_work);
-
- mutex_lock(&host->lock);
-
if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
if (host->mrq) {
printk(KERN_ERR "%s: Card removed during transfer!\n",
@@ -1262,8 +1256,6 @@ static void sdhci_card_detect_work(struct work_struct *wk)
}
}
- mutex_unlock(&host->lock);
-
mmc_detect_change(host->mmc, msecs_to_jiffies(200));
}
@@ -1511,7 +1503,7 @@ static irqreturn_t sdhci_irq_thread(int irq, void *dev_id)
sdhci_writel(host, intmask, SDHCI_INT_STATUS);
if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE))
- schedule_work(&host->card_detect_work);
+ sdhci_card_detect(host);
intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
@@ -1865,7 +1857,6 @@ int sdhci_add_host(struct sdhci_host *host)
/*
* Init work structs.
*/
- INIT_WORK(&host->card_detect_work, sdhci_card_detect_work);
INIT_WORK(&host->finish_work, sdhci_finish_work);
INIT_DELAYED_WORK(&host->timeout_work, sdhci_timeout_work);
@@ -1950,7 +1941,6 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
flush_delayed_work(&host->timeout_work);
- flush_work(&host->card_detect_work);
flush_work(&host->finish_work);
kfree(host->adma_desc);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 364d4e8..5f7d649 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -289,7 +289,6 @@ struct sdhci_host {
dma_addr_t adma_addr; /* Mapped ADMA descr. table */
dma_addr_t align_addr; /* Mapped bounce buffer */
- struct work_struct card_detect_work;
struct work_struct finish_work;
struct delayed_work timeout_work; /* Work for timeouts */
--
1.7.0.5
Just a cosmetic change, should not affect functionality.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/mmc/host/sdhci.c | 11 +++++------
1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e6adda8..c754df1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -22,6 +22,7 @@
#include <linux/slab.h>
#include <linux/mutex.h>
#include <linux/scatterlist.h>
+#include <linux/jiffies.h>
#include <linux/leds.h>
@@ -160,17 +161,16 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
host->clock = 0;
/* Wait max 100 ms */
- timeout = 100;
+ timeout = jiffies + msecs_to_jiffies(100);
/* hw clears the bit when it's done */
while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
- if (timeout == 0) {
+ if (time_after(jiffies, timeout)) {
printk(KERN_ERR "%s: Reset 0x%x never completed.\n",
mmc_hostname(host->mmc), (int)mask);
sdhci_dumpregs(host);
return;
}
- timeout--;
msleep(1);
}
@@ -884,7 +884,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
WARN_ON(host->cmd);
/* Wait max 10 ms */
- timeout = 10;
+ timeout = jiffies + msecs_to_jiffies(10);
mask = SDHCI_CMD_INHIBIT;
if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))
@@ -896,7 +896,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
mask &= ~SDHCI_DATA_INHIBIT;
while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
- if (timeout == 0) {
+ if (time_after(jiffies, timeout)) {
printk(KERN_ERR "%s: Controller never released "
"inhibit bit(s).\n", mmc_hostname(host->mmc));
sdhci_dumpregs(host);
@@ -904,7 +904,6 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
schedule_work(&host->finish_work);
return;
}
- timeout--;
mdelay(1);
}
--
1.7.0.5
Since we don't run in the atomic context any longer, we can
turn mdelay()s into msleep()s.
The only place where the driver is still using mdelay() is
sdhci_send_command(). There it is possible to use sleepable
delays too, but we don't actually want to force rescheduling
in a hot path.
Sure, we might end up calling msleep() there too, just not
via this safe patch.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/mmc/host/sdhci-of-esdhc.c | 2 +-
drivers/mmc/host/sdhci.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index c8623de..e9f99fe 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -94,7 +94,7 @@ static void esdhc_set_clock(struct sdhci_host *host, unsigned int clock)
setbits32(host->ioaddr + ESDHC_SYSTEM_CONTROL, ESDHC_CLOCK_IPGEN |
ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN |
div << ESDHC_DIVIDER_SHIFT | pre_div << ESDHC_PREDIV_SHIFT);
- mdelay(100);
+ msleep(100);
out:
host->clock = clock;
}
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 56d5c56..e6adda8 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -171,7 +171,7 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
return;
}
timeout--;
- mdelay(1);
+ msleep(1);
}
if (host->quirks & SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET)
@@ -1019,7 +1019,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
return;
}
timeout--;
- mdelay(1);
+ msleep(1);
}
clk |= SDHCI_CLOCK_CARD_EN;
@@ -1086,7 +1086,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
* can apply clock after applying power
*/
if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
- mdelay(10);
+ msleep(10);
}
/*****************************************************************************\
--
1.7.0.5
There's no need to clear the interrupt status register bit-by-bit,
we can just clear it once. This simplifies irq handler.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/mmc/host/sdhci.c | 23 ++++++-----------------
1 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 748a2e3..9ec245c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1522,38 +1522,29 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
goto out;
}
+ sdhci_writel(host, intmask, SDHCI_INT_STATUS);
+
DBG("*** %s got interrupt: 0x%08x\n",
mmc_hostname(host->mmc), intmask);
- if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
- sdhci_writel(host, intmask & (SDHCI_INT_CARD_INSERT |
- SDHCI_INT_CARD_REMOVE), SDHCI_INT_STATUS);
+ if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE))
schedule_work(&host->card_detect_work);
- }
intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
- if (intmask & SDHCI_INT_CMD_MASK) {
- sdhci_writel(host, intmask & SDHCI_INT_CMD_MASK,
- SDHCI_INT_STATUS);
+ if (intmask & SDHCI_INT_CMD_MASK)
sdhci_cmd_irq(host, intmask & SDHCI_INT_CMD_MASK);
- }
- if (intmask & SDHCI_INT_DATA_MASK) {
- sdhci_writel(host, intmask & SDHCI_INT_DATA_MASK,
- SDHCI_INT_STATUS);
+ if (intmask & SDHCI_INT_DATA_MASK)
sdhci_data_irq(host, intmask & SDHCI_INT_DATA_MASK);
- }
intmask &= ~(SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK);
intmask &= ~SDHCI_INT_ERROR;
- if (intmask & SDHCI_INT_BUS_POWER) {
+ if (intmask & SDHCI_INT_BUS_POWER)
printk(KERN_ERR "%s: Card is consuming too much power!\n",
mmc_hostname(host->mmc));
- sdhci_writel(host, SDHCI_INT_BUS_POWER, SDHCI_INT_STATUS);
- }
intmask &= ~SDHCI_INT_BUS_POWER;
@@ -1566,8 +1557,6 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
printk(KERN_ERR "%s: Unexpected interrupt 0x%08x.\n",
mmc_hostname(host->mmc), intmask);
sdhci_dumpregs(host);
-
- sdhci_writel(host, intmask, SDHCI_INT_STATUS);
}
result = IRQ_HANDLED;
--
1.7.0.5
We only need atomic context to disable SDHCI interrupts, after that
we can run in a kernel thread.
Note that irq handler still grabs an irqsave spinlock, we'll deal
with it in a subsequent patch.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/mmc/host/sdhci.c | 47 +++++++++++++++++++++++++++------------------
1 files changed, 28 insertions(+), 19 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9ec245c..0358b35 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1506,9 +1506,8 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
}
}
-static irqreturn_t sdhci_irq(int irq, void *dev_id)
+static irqreturn_t sdhci_irq_thread(int irq, void *dev_id)
{
- irqreturn_t result;
struct sdhci_host* host = dev_id;
u32 intmask;
int cardint = 0;
@@ -1516,17 +1515,8 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
spin_lock(&host->lock);
intmask = sdhci_readl(host, SDHCI_INT_STATUS);
-
- if (!intmask || intmask == 0xffffffff) {
- result = IRQ_NONE;
- goto out;
- }
-
sdhci_writel(host, intmask, SDHCI_INT_STATUS);
- DBG("*** %s got interrupt: 0x%08x\n",
- mmc_hostname(host->mmc), intmask);
-
if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE))
schedule_work(&host->card_detect_work);
@@ -1559,10 +1549,8 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
sdhci_dumpregs(host);
}
- result = IRQ_HANDLED;
-
mmiowb();
-out:
+
spin_unlock(&host->lock);
/*
@@ -1571,7 +1559,28 @@ out:
if (cardint)
mmc_signal_sdio_irq(host->mmc);
- return result;
+ /* Restore interrupts */
+ intmask = sdhci_readl(host, SDHCI_INT_ENABLE);
+ sdhci_writel(host, intmask, SDHCI_SIGNAL_ENABLE);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t sdhci_irq(int irq, void *dev_id)
+{
+ struct sdhci_host *host = dev_id;
+ u32 intmask = sdhci_readl(host, SDHCI_INT_STATUS);
+
+ if (!intmask || intmask == 0xffffffff)
+ return IRQ_NONE;
+
+ /* Disable interrupts */
+ sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
+
+ DBG("*** %s got interrupt: 0x%08x\n",
+ mmc_hostname(host->mmc), intmask);
+
+ return IRQ_WAKE_THREAD;
}
/*****************************************************************************\
@@ -1608,8 +1617,8 @@ int sdhci_resume_host(struct sdhci_host *host)
host->ops->enable_dma(host);
}
- ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
- mmc_hostname(host->mmc), host);
+ ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_irq_thread,
+ IRQF_SHARED, mmc_hostname(host->mmc), host);
if (ret)
return ret;
@@ -1868,8 +1877,8 @@ int sdhci_add_host(struct sdhci_host *host)
INIT_DELAYED_WORK(&host->timeout_work, sdhci_timeout_work);
- ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
- mmc_hostname(mmc), host);
+ ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_irq_thread,
+ IRQF_SHARED, mmc_hostname(host->mmc), host);
if (ret)
return ret;
--
1.7.0.5
On Wed, 14 Jul 2010 17:07:28 +0400, Anton Vorontsov <[email protected]> wrote:
> Hi all,
>
> Currently the sdhci driver does everything in the atomic context.
> And what is worse, PIO transfers are made from the IRQ handler.
>
> This causes huge latencies (up to 120 ms). On some P2020 SOCs,
> DMA and card detection is broken, which means that kernel polls
> for the card via PIO transfers every second. Needless to say
> that this is quite bad.
>
> So, this patch set reworks sdhci code to avoid atomic context,
> almost completely. We only do two device memory operations
> in the atomic context, and all the rest is threaded.
>
> I noticed no throughput drop neither with PIO transfers nor
> with DMA (tested on MPC8569E CPU), while latencies should be
> greatly improved.
I haven't had time to read these patches in detail yet but they all seem
to be sensible changes. A very nice series!
On Wed, 14 Jul 2010 17:07:28 +0400
Anton Vorontsov <[email protected]> wrote:
> Hi all,
>
> Currently the sdhci driver does everything in the atomic context.
> And what is worse, PIO transfers are made from the IRQ handler.
>
> This causes huge latencies (up to 120 ms). On some P2020 SOCs,
> DMA and card detection is broken, which means that kernel polls
> for the card via PIO transfers every second. Needless to say
> that this is quite bad.
>
> So, this patch set reworks sdhci code to avoid atomic context,
> almost completely. We only do two device memory operations
> in the atomic context, and all the rest is threaded.
>
> I noticed no throughput drop neither with PIO transfers nor
> with DMA (tested on MPC8569E CPU), while latencies should be
> greatly improved.
>
The patchset looks good to me, but it'd be nice to hear from the other
people who work on this code, please?