| Message ID | 20170217182241.11489-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17 February 2017 at 19:22, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > The current code assumes that DMA is finished before SD access end is > flagged. Thus, it schedules the 'dma_complete' tasklet in the SD card > interrupt routine when DATAEND is set. The assumption is not safe, > though. Even by mounting an SD card, it can be seen that sometimes DMA > complete is first, sometimes DATAEND. It seems they are usually close > enough timewise to not cause problems. However, a customer reported that > with CMD53 sometimes things really break apart. As a result, the BSP has > a patch which introduces flags for both events and makes sure both flags > are set before scheduling the tasklet. The customer accepted the patch, > yet it doesn't seem a proper upstream solution to me. > > This patch refactors the code to replace the tasklet with already > existing and more lightweight mechanisms. First of all, we set the > callback in a DMA descriptor to automatically get notified when DMA is > done. In the callback, we then use a completion to make sure the SD > access has already ended. Then, we proceed as before. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks, applied for next! Kind regards Uffe > --- > > Changes since V1/RFC: > > * removed #ifdef DEBUG > * rebased to latest mmc/next > * did some more performance testing. Couldn't spot any difference > > drivers/mmc/host/tmio_mmc.h | 2 +- > drivers/mmc/host/tmio_mmc_dma.c | 58 ++++++++++++++++++++++++----------------- > drivers/mmc/host/tmio_mmc_pio.c | 4 +-- > 3 files changed, 37 insertions(+), 27 deletions(-) > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index 2b349d48fb9a8a..891d400d2a7cf2 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -137,7 +137,7 @@ struct tmio_mmc_host { > bool force_pio; > struct dma_chan *chan_rx; > struct dma_chan *chan_tx; > - struct tasklet_struct dma_complete; > + struct completion dma_dataend; > struct tasklet_struct dma_issue; > struct scatterlist bounce_sg; > u8 *bounce_buf; > diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c > index fa8a936a3d9ba1..c7684fa91f1f9c 100644 > --- a/drivers/mmc/host/tmio_mmc_dma.c > +++ b/drivers/mmc/host/tmio_mmc_dma.c > @@ -43,6 +43,31 @@ void tmio_mmc_abort_dma(struct tmio_mmc_host *host) > tmio_mmc_enable_dma(host, true); > } > > +static void tmio_mmc_dma_callback(void *arg) > +{ > + struct tmio_mmc_host *host = arg; > + > + wait_for_completion(&host->dma_dataend); > + > + spin_lock_irq(&host->lock); > + > + if (!host->data) > + goto out; > + > + if (host->data->flags & MMC_DATA_READ) > + dma_unmap_sg(host->chan_rx->device->dev, > + host->sg_ptr, host->sg_len, > + DMA_FROM_DEVICE); > + else > + dma_unmap_sg(host->chan_tx->device->dev, > + host->sg_ptr, host->sg_len, > + DMA_TO_DEVICE); > + > + tmio_mmc_do_data_irq(host); > +out: > + spin_unlock_irq(&host->lock); > +} > + > static void tmio_mmc_start_dma_rx(struct tmio_mmc_host *host) > { > struct scatterlist *sg = host->sg_ptr, *sg_tmp; > @@ -88,6 +113,10 @@ static void tmio_mmc_start_dma_rx(struct tmio_mmc_host *host) > DMA_DEV_TO_MEM, DMA_CTRL_ACK); > > if (desc) { > + reinit_completion(&host->dma_dataend); > + desc->callback = tmio_mmc_dma_callback; > + desc->callback_param = host; > + > cookie = dmaengine_submit(desc); > if (cookie < 0) { > desc = NULL; > @@ -162,6 +191,10 @@ static void tmio_mmc_start_dma_tx(struct tmio_mmc_host *host) > DMA_MEM_TO_DEV, DMA_CTRL_ACK); > > if (desc) { > + reinit_completion(&host->dma_dataend); > + desc->callback = tmio_mmc_dma_callback; > + desc->callback_param = host; > + > cookie = dmaengine_submit(desc); > if (cookie < 0) { > desc = NULL; > @@ -221,29 +254,6 @@ static void tmio_mmc_issue_tasklet_fn(unsigned long priv) > dma_async_issue_pending(chan); > } > > -static void tmio_mmc_tasklet_fn(unsigned long arg) > -{ > - struct tmio_mmc_host *host = (struct tmio_mmc_host *)arg; > - > - spin_lock_irq(&host->lock); > - > - if (!host->data) > - goto out; > - > - if (host->data->flags & MMC_DATA_READ) > - dma_unmap_sg(host->chan_rx->device->dev, > - host->sg_ptr, host->sg_len, > - DMA_FROM_DEVICE); > - else > - dma_unmap_sg(host->chan_tx->device->dev, > - host->sg_ptr, host->sg_len, > - DMA_TO_DEVICE); > - > - tmio_mmc_do_data_irq(host); > -out: > - spin_unlock_irq(&host->lock); > -} > - > void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdata) > { > /* We can only either use DMA for both Tx and Rx or not use it at all */ > @@ -306,7 +316,7 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat > if (!host->bounce_buf) > goto ebouncebuf; > > - tasklet_init(&host->dma_complete, tmio_mmc_tasklet_fn, (unsigned long)host); > + init_completion(&host->dma_dataend); > tasklet_init(&host->dma_issue, tmio_mmc_issue_tasklet_fn, (unsigned long)host); > } > > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > index 6b789a739d4dfe..c41f2252945eb7 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -596,11 +596,11 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host *host, unsigned int stat) > > if (done) { > tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND); > - tasklet_schedule(&host->dma_complete); > + complete(&host->dma_dataend); > } > } else if (host->chan_rx && (data->flags & MMC_DATA_READ) && !host->force_pio) { > tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND); > - tasklet_schedule(&host->dma_complete); > + complete(&host->dma_dataend); > } else { > tmio_mmc_do_data_irq(host); > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_READOP | TMIO_MASK_WRITEOP); > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 2b349d48fb9a8a..891d400d2a7cf2 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -137,7 +137,7 @@ struct tmio_mmc_host { bool force_pio; struct dma_chan *chan_rx; struct dma_chan *chan_tx; - struct tasklet_struct dma_complete; + struct completion dma_dataend; struct tasklet_struct dma_issue; struct scatterlist bounce_sg; u8 *bounce_buf; diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c index fa8a936a3d9ba1..c7684fa91f1f9c 100644 --- a/drivers/mmc/host/tmio_mmc_dma.c +++ b/drivers/mmc/host/tmio_mmc_dma.c @@ -43,6 +43,31 @@ void tmio_mmc_abort_dma(struct tmio_mmc_host *host) tmio_mmc_enable_dma(host, true); } +static void tmio_mmc_dma_callback(void *arg) +{ + struct tmio_mmc_host *host = arg; + + wait_for_completion(&host->dma_dataend); + + spin_lock_irq(&host->lock); + + if (!host->data) + goto out; + + if (host->data->flags & MMC_DATA_READ) + dma_unmap_sg(host->chan_rx->device->dev, + host->sg_ptr, host->sg_len, + DMA_FROM_DEVICE); + else + dma_unmap_sg(host->chan_tx->device->dev, + host->sg_ptr, host->sg_len, + DMA_TO_DEVICE); + + tmio_mmc_do_data_irq(host); +out: + spin_unlock_irq(&host->lock); +} + static void tmio_mmc_start_dma_rx(struct tmio_mmc_host *host) { struct scatterlist *sg = host->sg_ptr, *sg_tmp; @@ -88,6 +113,10 @@ static void tmio_mmc_start_dma_rx(struct tmio_mmc_host *host) DMA_DEV_TO_MEM, DMA_CTRL_ACK); if (desc) { + reinit_completion(&host->dma_dataend); + desc->callback = tmio_mmc_dma_callback; + desc->callback_param = host; + cookie = dmaengine_submit(desc); if (cookie < 0) { desc = NULL; @@ -162,6 +191,10 @@ static void tmio_mmc_start_dma_tx(struct tmio_mmc_host *host) DMA_MEM_TO_DEV, DMA_CTRL_ACK); if (desc) { + reinit_completion(&host->dma_dataend); + desc->callback = tmio_mmc_dma_callback; + desc->callback_param = host; + cookie = dmaengine_submit(desc); if (cookie < 0) { desc = NULL; @@ -221,29 +254,6 @@ static void tmio_mmc_issue_tasklet_fn(unsigned long priv) dma_async_issue_pending(chan); } -static void tmio_mmc_tasklet_fn(unsigned long arg) -{ - struct tmio_mmc_host *host = (struct tmio_mmc_host *)arg; - - spin_lock_irq(&host->lock); - - if (!host->data) - goto out; - - if (host->data->flags & MMC_DATA_READ) - dma_unmap_sg(host->chan_rx->device->dev, - host->sg_ptr, host->sg_len, - DMA_FROM_DEVICE); - else - dma_unmap_sg(host->chan_tx->device->dev, - host->sg_ptr, host->sg_len, - DMA_TO_DEVICE); - - tmio_mmc_do_data_irq(host); -out: - spin_unlock_irq(&host->lock); -} - void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdata) { /* We can only either use DMA for both Tx and Rx or not use it at all */ @@ -306,7 +316,7 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat if (!host->bounce_buf) goto ebouncebuf; - tasklet_init(&host->dma_complete, tmio_mmc_tasklet_fn, (unsigned long)host); + init_completion(&host->dma_dataend); tasklet_init(&host->dma_issue, tmio_mmc_issue_tasklet_fn, (unsigned long)host); } diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index 6b789a739d4dfe..c41f2252945eb7 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -596,11 +596,11 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host *host, unsigned int stat) if (done) { tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND); - tasklet_schedule(&host->dma_complete); + complete(&host->dma_dataend); } } else if (host->chan_rx && (data->flags & MMC_DATA_READ) && !host->force_pio) { tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND); - tasklet_schedule(&host->dma_complete); + complete(&host->dma_dataend); } else { tmio_mmc_do_data_irq(host); tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_READOP | TMIO_MASK_WRITEOP);
The current code assumes that DMA is finished before SD access end is flagged. Thus, it schedules the 'dma_complete' tasklet in the SD card interrupt routine when DATAEND is set. The assumption is not safe, though. Even by mounting an SD card, it can be seen that sometimes DMA complete is first, sometimes DATAEND. It seems they are usually close enough timewise to not cause problems. However, a customer reported that with CMD53 sometimes things really break apart. As a result, the BSP has a patch which introduces flags for both events and makes sure both flags are set before scheduling the tasklet. The customer accepted the patch, yet it doesn't seem a proper upstream solution to me. This patch refactors the code to replace the tasklet with already existing and more lightweight mechanisms. First of all, we set the callback in a DMA descriptor to automatically get notified when DMA is done. In the callback, we then use a completion to make sure the SD access has already ended. Then, we proceed as before. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Changes since V1/RFC: * removed #ifdef DEBUG * rebased to latest mmc/next * did some more performance testing. Couldn't spot any difference drivers/mmc/host/tmio_mmc.h | 2 +- drivers/mmc/host/tmio_mmc_dma.c | 58 ++++++++++++++++++++++++----------------- drivers/mmc/host/tmio_mmc_pio.c | 4 +-- 3 files changed, 37 insertions(+), 27 deletions(-)