Re: [PATCH] omap_hsmmc: improve interrupt synchronisation
Andrew Morton wrote: On Wed, 12 May 2010 10:50:45 +0300 Adrian Hunter wrote: >From ad2e1cd024ccf9144b6620cfe808893719db738f Mon Sep 17 00:00:00 2001 From: Adrian Hunter Date: Wed, 14 Apr 2010 16:26:45 +0300 Subject: [PATCH] omap_hsmmc: improve interrupt synchronisation The following changes were needed: - do not use in_interrupt() because it will not work with threaded interrupts In addition, the following improvements were made: - ensure DMA is unmapped only after the final DMA interrupt - ensure a request is completed only after the final DMA interrupt - disable controller interrupts when a request is not in progress - remove the spin-lock protecting the start of a new request from an unexpected interrupt because the locking was complicated and a 'req_in_progress' flag suffices (since the spin-lock only defers the unexpected interrupts anyway) - instead use the spin-lock to protect the MMC interrupt handler from the DMA interrupt handler - remove the semaphore preventing DMA from being started while the previous DMA is still in progress - the other changes make that impossible, so it is now a BUG_ON condition - ensure the controller interrupt status is clear before exiting the interrrupt handler In general, these changes make the code safer but do not fix any specific bugs so backporting is not necessary. ... +static void omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_request *mrq) +{ + int dma_ch; + + spin_lock(&host->irq_lock); + host->req_in_progress = 0; + dma_ch = host->dma_ch; + spin_unlock(&host->irq_lock); + + omap_hsmmc_disable_irq(host); + /* Do not complete the request if DMA is still in progress */ + if (mrq->data && host->use_dma && dma_ch != -1) + return; + host->mrq = NULL; + mmc_request_done(host->mmc, mrq); +} Are we sure that irq_lock doesn't need to be taken in an irq-safe fashion? It is OK at the moment. It is only used in interrupt handlers which currently do run in interrupt context with IRQF_DISABLED. If both DMA and MMC interrupt handlers were changed to be threaded interrupts, then the spin-lock would still be OK. But if only one of them was changed, then the spin-lock would have to be changed to be irq-safe. send_init_stream() doesn't report an error if its busywait times out. It is not a problem. It may mean the card does not initialize but if that happens there will be lots more errors. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] omap_hsmmc: improve interrupt synchronisation
On Wed, 12 May 2010 10:50:45 +0300 Adrian Hunter wrote: > >From ad2e1cd024ccf9144b6620cfe808893719db738f Mon Sep 17 00:00:00 2001 > From: Adrian Hunter > Date: Wed, 14 Apr 2010 16:26:45 +0300 > Subject: [PATCH] omap_hsmmc: improve interrupt synchronisation > > The following changes were needed: > - do not use in_interrupt() because it will not work > with threaded interrupts > > In addition, the following improvements were made: > - ensure DMA is unmapped only after the final DMA interrupt > - ensure a request is completed only after the final DMA interrupt > - disable controller interrupts when a request is not in progress > - remove the spin-lock protecting the start of a new request from > an unexpected interrupt because the locking was complicated and > a 'req_in_progress' flag suffices (since the spin-lock only defers > the unexpected interrupts anyway) > - instead use the spin-lock to protect the MMC interrupt handler > from the DMA interrupt handler > - remove the semaphore preventing DMA from being started while > the previous DMA is still in progress - the other changes make that > impossible, so it is now a BUG_ON condition > - ensure the controller interrupt status is clear before exiting > the interrrupt handler > > In general, these changes make the code safer but do not fix any specific > bugs so backporting is not necessary. > > > ... > > +static void omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct > mmc_request *mrq) > +{ > + int dma_ch; > + > + spin_lock(&host->irq_lock); > + host->req_in_progress = 0; > + dma_ch = host->dma_ch; > + spin_unlock(&host->irq_lock); > + > + omap_hsmmc_disable_irq(host); > + /* Do not complete the request if DMA is still in progress */ > + if (mrq->data && host->use_dma && dma_ch != -1) > + return; > + host->mrq = NULL; > + mmc_request_done(host->mmc, mrq); > +} Are we sure that irq_lock doesn't need to be taken in an irq-safe fashion? send_init_stream() doesn't report an error if its busywait times out. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] omap_hsmmc: improve interrupt synchronisation
From ad2e1cd024ccf9144b6620cfe808893719db738f Mon Sep 17 00:00:00 2001 From: Adrian Hunter Date: Wed, 14 Apr 2010 16:26:45 +0300 Subject: [PATCH] omap_hsmmc: improve interrupt synchronisation The following changes were needed: - do not use in_interrupt() because it will not work with threaded interrupts In addition, the following improvements were made: - ensure DMA is unmapped only after the final DMA interrupt - ensure a request is completed only after the final DMA interrupt - disable controller interrupts when a request is not in progress - remove the spin-lock protecting the start of a new request from an unexpected interrupt because the locking was complicated and a 'req_in_progress' flag suffices (since the spin-lock only defers the unexpected interrupts anyway) - instead use the spin-lock to protect the MMC interrupt handler from the DMA interrupt handler - remove the semaphore preventing DMA from being started while the previous DMA is still in progress - the other changes make that impossible, so it is now a BUG_ON condition - ensure the controller interrupt status is clear before exiting the interrrupt handler In general, these changes make the code safer but do not fix any specific bugs so backporting is not necessary. Signed-off-by: Adrian Hunter Tested-by: Venkatraman S Acked-by: Madhusudhan Chikkature Acked-by: Tony Lindgren --- drivers/mmc/host/omap_hsmmc.c | 262 + 1 files changed, 134 insertions(+), 128 deletions(-) diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index c0b5021..cc0272d 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -157,12 +157,10 @@ struct omap_hsmmc_host { */ struct regulator *vcc; struct regulator *vcc_aux; - struct semaphore sem; struct work_struct mmc_carddetect_work; void__iomem *base; resource_size_t mapbase; spinlock_t irq_lock; /* Prevent races with irq handler */ - unsigned long flags; unsigned intid; unsigned intdma_len; unsigned intdma_sg_idx; @@ -183,6 +181,7 @@ struct omap_hsmmc_host { int protect_card; int reqs_blocked; int use_reg; + int req_in_progress; struct omap_mmc_platform_data *pdata; }; @@ -480,6 +479,27 @@ static void omap_hsmmc_stop_clock(struct omap_hsmmc_host *host) dev_dbg(mmc_dev(host->mmc), "MMC Clock is not stoped\n"); } +static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host) +{ + unsigned int irq_mask; + + if (host->use_dma) + irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE); + else + irq_mask = INT_EN_MASK; + + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); + OMAP_HSMMC_WRITE(host->base, ISE, irq_mask); + OMAP_HSMMC_WRITE(host->base, IE, irq_mask); +} + +static void omap_hsmmc_disable_irq(struct omap_hsmmc_host *host) +{ + OMAP_HSMMC_WRITE(host->base, ISE, 0); + OMAP_HSMMC_WRITE(host->base, IE, 0); + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); +} + #ifdef CONFIG_PM /* @@ -548,9 +568,7 @@ static int omap_hsmmc_context_restore(struct omap_hsmmc_host *host) && time_before(jiffies, timeout)) ; - OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); - OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK); - OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); + omap_hsmmc_disable_irq(host); /* Do not initialize card-specific things if the power is off */ if (host->power_mode == MMC_POWER_OFF) @@ -653,6 +671,8 @@ static void send_init_stream(struct omap_hsmmc_host *host) return; disable_irq(host->irq); + + OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); OMAP_HSMMC_WRITE(host->base, CON, OMAP_HSMMC_READ(host->base, CON) | INIT_STREAM); OMAP_HSMMC_WRITE(host->base, CMD, INIT_STREAM_CMD); @@ -718,17 +738,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd, mmc_hostname(host->mmc), cmd->opcode, cmd->arg); host->cmd = cmd; - /* -* Clear status bits and enable interrupts -*/ - OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); - OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK); - - if (host->use_dma) - OMAP_HSMMC_WRITE(host->base, IE, -INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE)); - else - OMAP_
Re: [PATCH] omap_hsmmc: improve interrupt synchronisation
Venkatraman S wrote: Adrian Hunter wrote: From: Adrian Hunter Date: Wed, 14 Apr 2010 16:26:45 +0300 Subject: [PATCH] omap_hsmmc: improve interrupt synchronisation The following changes were needed: - do not use in_interrupt() because it will not work with threaded interrupts In addition, the following improvements were made: - ensure DMA is unmapped only after the final DMA interrupt - ensure a request is completed only after the final DMA interrupt - disable controller interrupts when a request is not in progress - remove the spin-lock protecting the start of a new request from an unexpected interrupt because the locking was complicated and a 'req_in_progress' flag suffices (since the spin-lock only defers the unexpected interrupts anyway) - remove the semaphore preventing DMA from being started while the previous DMA is still in progress - the other changes make that impossible, so it is now a BUG_ON condition - ensure the controller interrupt status is clear before exiting the interrupt handler On OMAP4, the MMC and DMA interrupt lines could be routed to different CPUs and the handlers could be executed simultaneously. The req_in_progress variable would need spin lock protection in this case. Ditto host->dma_ch In general, these changes make the code safer but do not fix any specific bugs so backporting is not necessary. Signed-off-by: Adrian Hunter --- drivers/mmc/host/omap_hsmmc.c | 211 ++--- 1 files changed, 94 insertions(+), 117 deletions(-) diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index c0b5021..e58ca99 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -157,11 +157,9 @@ struct omap_hsmmc_host { */ struct regulator *vcc; struct regulator *vcc_aux; - struct semaphore sem; struct work_struct mmc_carddetect_work; void__iomem *base; resource_size_t mapbase; - spinlock_t irq_lock; /* Prevent races with irq handler */ unsigned long flags; unsigned intid; unsigned intdma_len; @@ -183,6 +181,7 @@ struct omap_hsmmc_host { int protect_card; int reqs_blocked; int use_reg; + int req_in_progress; struct omap_mmc_platform_data *pdata; }; @@ -480,6 +479,27 @@ static void omap_hsmmc_stop_clock(struct omap_hsmmc_host *host) dev_dbg(mmc_dev(host->mmc), "MMC Clock is not stoped\n"); } +static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host) +{ + unsigned int irq_mask; + + if (host->use_dma) + irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE); + else + irq_mask = INT_EN_MASK; + + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); + OMAP_HSMMC_WRITE(host->base, ISE, irq_mask); + OMAP_HSMMC_WRITE(host->base, IE, irq_mask); +} + +static void omap_hsmmc_disable_irq(struct omap_hsmmc_host *host) +{ + OMAP_HSMMC_WRITE(host->base, ISE, 0); + OMAP_HSMMC_WRITE(host->base, IE, 0); + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); +} + #ifdef CONFIG_PM /* @@ -548,9 +568,7 @@ static int omap_hsmmc_context_restore(struct omap_hsmmc_host *host) && time_before(jiffies, timeout)) ; - OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); - OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK); - OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); + omap_hsmmc_disable_irq(host); /* Do not initialize card-specific things if the power is off */ if (host->power_mode == MMC_POWER_OFF) @@ -653,6 +671,8 @@ static void send_init_stream(struct omap_hsmmc_host *host) return; disable_irq(host->irq); + + OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); OMAP_HSMMC_WRITE(host->base, CON, OMAP_HSMMC_READ(host->base, CON) | INIT_STREAM); OMAP_HSMMC_WRITE(host->base, CMD, INIT_STREAM_CMD); @@ -718,17 +738,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd, mmc_hostname(host->mmc), cmd->opcode, cmd->arg); host->cmd = cmd; - /* -* Clear status bits and enable interrupts -*/ - OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); - OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK); - - if (host->use_dma) - OMAP_HSMMC_WRITE(host->base, IE, -INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE)); - else - OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); + omap_hsmmc_enable_irq(host);
Re: [PATCH] omap_hsmmc: improve interrupt synchronisation
Adrian Hunter wrote: > From: Adrian Hunter > Date: Wed, 14 Apr 2010 16:26:45 +0300 > Subject: [PATCH] omap_hsmmc: improve interrupt synchronisation > > The following changes were needed: > - do not use in_interrupt() because it will not work > with threaded interrupts > > In addition, the following improvements were made: > - ensure DMA is unmapped only after the final DMA interrupt > - ensure a request is completed only after the final DMA interrupt > - disable controller interrupts when a request is not in progress > - remove the spin-lock protecting the start of a new request from > an unexpected interrupt because the locking was complicated and > a 'req_in_progress' flag suffices (since the spin-lock only defers > the unexpected interrupts anyway) > - remove the semaphore preventing DMA from being started while > the previous DMA is still in progress - the other changes make that > impossible, so it is now a BUG_ON condition > - ensure the controller interrupt status is clear before exiting > the interrupt handler > On OMAP4, the MMC and DMA interrupt lines could be routed to different CPUs and the handlers could be executed simultaneously. The req_in_progress variable would need spin lock protection in this case. > In general, these changes make the code safer but do not fix any specific > bugs so backporting is not necessary. > > Signed-off-by: Adrian Hunter > --- > drivers/mmc/host/omap_hsmmc.c | 211 > ++--- > 1 files changed, 94 insertions(+), 117 deletions(-) > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index c0b5021..e58ca99 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -157,11 +157,9 @@ struct omap_hsmmc_host { > */ > struct regulator *vcc; > struct regulator *vcc_aux; > - struct semaphore sem; > struct work_struct mmc_carddetect_work; > void __iomem *base; > resource_size_t mapbase; > - spinlock_t irq_lock; /* Prevent races with irq handler > */ > unsigned long flags; > unsigned int id; > unsigned int dma_len; > @@ -183,6 +181,7 @@ struct omap_hsmmc_host { > int protect_card; > int reqs_blocked; > int use_reg; > + int req_in_progress; > > struct omap_mmc_platform_data *pdata; > }; > @@ -480,6 +479,27 @@ static void omap_hsmmc_stop_clock(struct > omap_hsmmc_host *host) > dev_dbg(mmc_dev(host->mmc), "MMC Clock is not stoped\n"); > } > > +static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host) > +{ > + unsigned int irq_mask; > + > + if (host->use_dma) > + irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE); > + else > + irq_mask = INT_EN_MASK; > + > + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); > + OMAP_HSMMC_WRITE(host->base, ISE, irq_mask); > + OMAP_HSMMC_WRITE(host->base, IE, irq_mask); > +} > + > +static void omap_hsmmc_disable_irq(struct omap_hsmmc_host *host) > +{ > + OMAP_HSMMC_WRITE(host->base, ISE, 0); > + OMAP_HSMMC_WRITE(host->base, IE, 0); > + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); > +} > + > #ifdef CONFIG_PM > > /* > @@ -548,9 +568,7 @@ static int omap_hsmmc_context_restore(struct > omap_hsmmc_host *host) > && time_before(jiffies, timeout)) > ; > > - OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); > - OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK); > - OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); > + omap_hsmmc_disable_irq(host); > > /* Do not initialize card-specific things if the power is off */ > if (host->power_mode == MMC_POWER_OFF) > @@ -653,6 +671,8 @@ static void send_init_stream(struct omap_hsmmc_host > *host) > return; > > disable_irq(host->irq); > + > + OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); > OMAP_HSMMC_WRITE(host->base, CON, > OMAP_HSMMC_READ(host->base, CON) | INIT_STREAM); > OMAP_HSMMC_WRITE(host->base, CMD, INIT_STREAM_CMD); > @@ -718,17 +738,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, > struct mmc_command *cmd, > mmc_hostname(host->mmc), cmd->opcode, cmd->arg); > host-&
[PATCH] omap_hsmmc: improve interrupt synchronisation
From: Adrian Hunter Date: Wed, 14 Apr 2010 16:26:45 +0300 Subject: [PATCH] omap_hsmmc: improve interrupt synchronisation The following changes were needed: - do not use in_interrupt() because it will not work with threaded interrupts In addition, the following improvements were made: - ensure DMA is unmapped only after the final DMA interrupt - ensure a request is completed only after the final DMA interrupt - disable controller interrupts when a request is not in progress - remove the spin-lock protecting the start of a new request from an unexpected interrupt because the locking was complicated and a 'req_in_progress' flag suffices (since the spin-lock only defers the unexpected interrupts anyway) - remove the semaphore preventing DMA from being started while the previous DMA is still in progress - the other changes make that impossible, so it is now a BUG_ON condition - ensure the controller interrupt status is clear before exiting the interrupt handler In general, these changes make the code safer but do not fix any specific bugs so backporting is not necessary. Signed-off-by: Adrian Hunter --- drivers/mmc/host/omap_hsmmc.c | 211 ++--- 1 files changed, 94 insertions(+), 117 deletions(-) diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index c0b5021..e58ca99 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -157,11 +157,9 @@ struct omap_hsmmc_host { */ struct regulator *vcc; struct regulator *vcc_aux; - struct semaphore sem; struct work_struct mmc_carddetect_work; void__iomem *base; resource_size_t mapbase; - spinlock_t irq_lock; /* Prevent races with irq handler */ unsigned long flags; unsigned intid; unsigned intdma_len; @@ -183,6 +181,7 @@ struct omap_hsmmc_host { int protect_card; int reqs_blocked; int use_reg; + int req_in_progress; struct omap_mmc_platform_data *pdata; }; @@ -480,6 +479,27 @@ static void omap_hsmmc_stop_clock(struct omap_hsmmc_host *host) dev_dbg(mmc_dev(host->mmc), "MMC Clock is not stoped\n"); } +static void omap_hsmmc_enable_irq(struct omap_hsmmc_host *host) +{ + unsigned int irq_mask; + + if (host->use_dma) + irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE); + else + irq_mask = INT_EN_MASK; + + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); + OMAP_HSMMC_WRITE(host->base, ISE, irq_mask); + OMAP_HSMMC_WRITE(host->base, IE, irq_mask); +} + +static void omap_hsmmc_disable_irq(struct omap_hsmmc_host *host) +{ + OMAP_HSMMC_WRITE(host->base, ISE, 0); + OMAP_HSMMC_WRITE(host->base, IE, 0); + OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); +} + #ifdef CONFIG_PM /* @@ -548,9 +568,7 @@ static int omap_hsmmc_context_restore(struct omap_hsmmc_host *host) && time_before(jiffies, timeout)) ; - OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); - OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK); - OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); + omap_hsmmc_disable_irq(host); /* Do not initialize card-specific things if the power is off */ if (host->power_mode == MMC_POWER_OFF) @@ -653,6 +671,8 @@ static void send_init_stream(struct omap_hsmmc_host *host) return; disable_irq(host->irq); + + OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); OMAP_HSMMC_WRITE(host->base, CON, OMAP_HSMMC_READ(host->base, CON) | INIT_STREAM); OMAP_HSMMC_WRITE(host->base, CMD, INIT_STREAM_CMD); @@ -718,17 +738,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd, mmc_hostname(host->mmc), cmd->opcode, cmd->arg); host->cmd = cmd; - /* -* Clear status bits and enable interrupts -*/ - OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR); - OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK); - - if (host->use_dma) - OMAP_HSMMC_WRITE(host->base, IE, -INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE)); - else - OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK); + omap_hsmmc_enable_irq(host); host->response_busy = 0; if (cmd->flags & MMC_RSP_PRESENT) { @@ -762,13 +772,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd, if (host->use_dma) c