| Message ID | 1350056012-18625-1-git-send-email-ulf.hansson@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 12, 2012 at 5:33 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote: > From: Ulf Hansson <ulf.hansson@linaro.org> > > The cookie is now used to indicate if dma_unmap_sg shall be > done in post_request. At DMA errors, the DMA job is immediately > not only terminated but also unmapped. To indicate that this > has been done the cookie is reset to zero. post_request will > thus only do dma_umap_sg for requests which has a cookie not set > to zero. > > Some corresponding duplicated code could then be removed and > moreover some corrections at DMA errors for terminating the same > DMA job twice has also been fixed. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Per Forlin <per.forlin@stericsson.com> It looks like it's both factoring out code and also adding some unmapping in hithereto unhandled cases, correct? It looks OK to me now atleast so Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij -- 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
Hi Linus, On 12 October 2012 23:32, Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Oct 12, 2012 at 5:33 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote: > >> From: Ulf Hansson <ulf.hansson@linaro.org> >> >> The cookie is now used to indicate if dma_unmap_sg shall be >> done in post_request. At DMA errors, the DMA job is immediately >> not only terminated but also unmapped. To indicate that this >> has been done the cookie is reset to zero. post_request will >> thus only do dma_umap_sg for requests which has a cookie not set >> to zero. >> >> Some corresponding duplicated code could then be removed and >> moreover some corrections at DMA errors for terminating the same >> DMA job twice has also been fixed. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> Signed-off-by: Per Forlin <per.forlin@stericsson.com> > > It looks like it's both factoring out code and also adding some unmapping > in hithereto unhandled cases, correct? It looks OK to me now atleast so > Acked-by: Linus Walleij <linus.walleij@linaro.org> This code has not been tested on a "legacy" ARM PL180 but only for ux500 boards. Even if it should affect DMA handling we should test this properly. Would be great if you were able to help out, I guess you still have available hardware for these tests? Kind regards Ulf Hansson -- 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
On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > This code has not been tested on a "legacy" ARM PL180 but only for > ux500 boards. Even if it should affect DMA handling we should test > this properly. Would be great if you were able to help out, I guess > you still have available hardware for these tests? I can test the Integrator/CP and Realview in IRQ/PIO mode and the U300 in DMA mode. I need some help to provoke errorpath in DMA mode though, so any hints are welcome... Yours, Linus Walleij -- 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
On 16 October 2012 10:17, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> This code has not been tested on a "legacy" ARM PL180 but only for >> ux500 boards. Even if it should affect DMA handling we should test >> this properly. Would be great if you were able to help out, I guess >> you still have available hardware for these tests? > > I can test the Integrator/CP and Realview in IRQ/PIO mode and > the U300 in DMA mode. Great! > > I need some help to provoke errorpath in DMA mode though, > so any hints are welcome... One good option could be to stress test card insert/removal sequences, while at the same time doing read/write requests. > > Yours, > Linus Walleij Kind regards Ulf Hansson -- 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
On 16 October 2012 13:44, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 16 October 2012 10:17, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >>> This code has not been tested on a "legacy" ARM PL180 but only for >>> ux500 boards. Even if it should affect DMA handling we should test >>> this properly. Would be great if you were able to help out, I guess >>> you still have available hardware for these tests? >> >> I can test the Integrator/CP and Realview in IRQ/PIO mode and >> the U300 in DMA mode. > > Great! > >> >> I need some help to provoke errorpath in DMA mode though, >> so any hints are welcome... > > One good option could be to stress test card insert/removal sequences, > while at the same time doing read/write requests. > >> >> Yours, >> Linus Walleij > > Kind regards > Ulf Hansson Just a kind reminder for Linus; did you manage to do some tests for these patches? Kind regards Ulf Hansson -- 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
On 21 November 2012 16:22, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 16 October 2012 13:44, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 16 October 2012 10:17, Linus Walleij <linus.walleij@linaro.org> wrote: >>> On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> >>>> This code has not been tested on a "legacy" ARM PL180 but only for >>>> ux500 boards. Even if it should affect DMA handling we should test >>>> this properly. Would be great if you were able to help out, I guess >>>> you still have available hardware for these tests? >>> >>> I can test the Integrator/CP and Realview in IRQ/PIO mode and >>> the U300 in DMA mode. >> >> Great! >> >>> >>> I need some help to provoke errorpath in DMA mode though, >>> so any hints are welcome... >> >> One good option could be to stress test card insert/removal sequences, >> while at the same time doing read/write requests. >> >>> >>> Yours, >>> Linus Walleij >> >> Kind regards >> Ulf Hansson > > Just a kind reminder for Linus; did you manage to do some tests for > these patches? > > Kind regards > Ulf Hansson I have now tested these patches for ARM-RealView PB1176. Looks good! I stress tested card insert/remove at the same time you write/read to the card. Error handling is stable. Kind regards Ulf Hansson -- 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
On 28 November 2012 15:00, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 21 November 2012 16:22, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 16 October 2012 13:44, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> On 16 October 2012 10:17, Linus Walleij <linus.walleij@linaro.org> wrote: >>>> On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> >>>>> This code has not been tested on a "legacy" ARM PL180 but only for >>>>> ux500 boards. Even if it should affect DMA handling we should test >>>>> this properly. Would be great if you were able to help out, I guess >>>>> you still have available hardware for these tests? >>>> >>>> I can test the Integrator/CP and Realview in IRQ/PIO mode and >>>> the U300 in DMA mode. >>> >>> Great! >>> >>>> >>>> I need some help to provoke errorpath in DMA mode though, >>>> so any hints are welcome... >>> >>> One good option could be to stress test card insert/removal sequences, >>> while at the same time doing read/write requests. >>> >>>> >>>> Yours, >>>> Linus Walleij >>> >>> Kind regards >>> Ulf Hansson >> >> Just a kind reminder for Linus; did you manage to do some tests for >> these patches? >> >> Kind regards >> Ulf Hansson > > I have now tested these patches for ARM-RealView PB1176. Looks good! I > stress tested card insert/remove at the same time you write/read to > the card. Error handling is stable. > > Kind regards > Ulf Hansson For clarification. Skip this patch. The next version is rebased on top of 3.7 and is thus not based on "mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant", which needs some rework. Kind regards Ulf Hansson -- 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/mmci.c b/drivers/mmc/host/mmci.c index ca6d128..3165808 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -383,10 +383,33 @@ static inline void mmci_dma_release(struct mmci_host *host) host->dma_rx_channel = host->dma_tx_channel = NULL; } +static void mmci_dma_data_error(struct mmci_host *host) +{ + dev_err(mmc_dev(host->mmc), "error during DMA transfer!\n"); + dmaengine_terminate_all(host->dma_current); + host->dma_current = NULL; + host->dma_desc_current = NULL; + host->data->host_cookie = 0; +} + static void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data) { - struct dma_chan *chan = host->dma_current; + struct dma_chan *chan; enum dma_data_direction dir; + + if (data->flags & MMC_DATA_READ) { + dir = DMA_FROM_DEVICE; + chan = host->dma_rx_channel; + } else { + dir = DMA_TO_DEVICE; + chan = host->dma_tx_channel; + } + + dma_unmap_sg(chan->device->dev, data->sg, data->sg_len, dir); +} + +static void mmci_dma_finalize(struct mmci_host *host, struct mmc_data *data) +{ u32 status; int i; @@ -405,19 +428,12 @@ static void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data) * contiguous buffers. On TX, we'll get a FIFO underrun error. */ if (status & MCI_RXDATAAVLBLMASK) { - dmaengine_terminate_all(chan); - if (!data->error) - data->error = -EIO; - } - - if (data->flags & MMC_DATA_WRITE) { - dir = DMA_TO_DEVICE; - } else { - dir = DMA_FROM_DEVICE; + data->error = -EIO; + mmci_dma_data_error(host); } if (!data->host_cookie) - dma_unmap_sg(chan->device->dev, data->sg, data->sg_len, dir); + mmci_dma_unmap(host, data); /* * Use of DMA with scatter-gather is impossible. @@ -427,16 +443,15 @@ static void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data) dev_err(mmc_dev(host->mmc), "buggy DMA detected. Taking evasive action.\n"); mmci_dma_release(host); } -} -static void mmci_dma_data_error(struct mmci_host *host) -{ - dev_err(mmc_dev(host->mmc), "error during DMA transfer!\n"); - dmaengine_terminate_all(host->dma_current); + host->dma_current = NULL; + host->dma_desc_current = NULL; } -static int mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data, - struct mmci_host_next *next) +/* prepares DMA channel and DMA descriptor, returns non-zero on failure */ +static int __mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data, + struct dma_chan **dma_chan, + struct dma_async_tx_descriptor **dma_desc) { struct variant_data *variant = host->variant; struct dma_slave_config conf = { @@ -454,16 +469,6 @@ static int mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data, enum dma_data_direction buffer_dirn; int nr_sg; - /* Check if next job is already prepared */ - if (data->host_cookie && !next && - host->dma_current && host->dma_desc_current) - return 0; - - if (!next) { - host->dma_current = NULL; - host->dma_desc_current = NULL; - } - if (data->flags & MMC_DATA_READ) { conf.direction = DMA_DEV_TO_MEM; buffer_dirn = DMA_FROM_DEVICE; @@ -497,30 +502,42 @@ static int mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data, if (!desc) goto unmap_exit; - if (next) { - next->dma_chan = chan; - next->dma_desc = desc; - } else { - host->dma_current = chan; - host->dma_desc_current = desc; - } + *dma_chan = chan; + *dma_desc = desc; return 0; unmap_exit: - if (!next) - dmaengine_terminate_all(chan); dma_unmap_sg(device->dev, data->sg, data->sg_len, buffer_dirn); return -ENOMEM; } +static inline int mmci_dma_prep_data(struct mmci_host *host, + struct mmc_data *data) +{ + /* Check if next job is already prepared. */ + if (host->dma_current && host->dma_desc_current) + return 0; + + /* No job were prepared thus do it now. */ + return __mmci_dma_prep_data(host, data, &host->dma_current, + &host->dma_desc_current); +} + +static inline int mmci_dma_prep_next(struct mmci_host *host, + struct mmc_data *data) +{ + struct mmci_host_next *nd = &host->next_data; + return __mmci_dma_prep_data(host, data, &nd->dma_chan, &nd->dma_desc); +} + static int mmci_dma_start_data(struct mmci_host *host, unsigned int datactrl) { int ret; struct mmc_data *data = host->data; struct variant_data *variant = host->variant; - ret = mmci_dma_prep_data(host, host->data, NULL); + ret = mmci_dma_prep_data(host, host->data); if (ret) return ret; @@ -555,19 +572,11 @@ static void mmci_get_next_data(struct mmci_host *host, struct mmc_data *data) { struct mmci_host_next *next = &host->next_data; - if (data->host_cookie && data->host_cookie != next->cookie) { - pr_warning("[%s] invalid cookie: data->host_cookie %d" - " host->next_data.cookie %d\n", - __func__, data->host_cookie, host->next_data.cookie); - data->host_cookie = 0; - } - - if (!data->host_cookie) - return; + WARN_ON(data->host_cookie && data->host_cookie != next->cookie); + WARN_ON(!data->host_cookie && (next->dma_desc || next->dma_chan)); host->dma_desc_current = next->dma_desc; host->dma_current = next->dma_chan; - next->dma_desc = NULL; next->dma_chan = NULL; } @@ -582,22 +591,13 @@ static void mmci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq, if (!data) return; - if (mmci_validate_data(host, mrq->data)) - return; + BUG_ON(data->host_cookie); - if (data->host_cookie) { - data->host_cookie = 0; + if (mmci_validate_data(host, data)) return; - } - /* if config for dma */ - if (((data->flags & MMC_DATA_WRITE) && host->dma_tx_channel) || - ((data->flags & MMC_DATA_READ) && host->dma_rx_channel)) { - if (mmci_dma_prep_data(host, data, nd)) - data->host_cookie = 0; - else - data->host_cookie = ++nd->cookie < 0 ? 1 : nd->cookie; - } + if (!mmci_dma_prep_next(host, data)) + data->host_cookie = ++nd->cookie < 0 ? 1 : nd->cookie; } static void mmci_post_request(struct mmc_host *mmc, struct mmc_request *mrq, @@ -605,29 +605,23 @@ static void mmci_post_request(struct mmc_host *mmc, struct mmc_request *mrq, { struct mmci_host *host = mmc_priv(mmc); struct mmc_data *data = mrq->data; - struct dma_chan *chan; - enum dma_data_direction dir; - if (!data) + if (!data || !data->host_cookie) return; - if (data->flags & MMC_DATA_READ) { - dir = DMA_FROM_DEVICE; - chan = host->dma_rx_channel; - } else { - dir = DMA_TO_DEVICE; - chan = host->dma_tx_channel; - } + mmci_dma_unmap(host, data); + if (err) { + struct mmci_host_next *next = &host->next_data; + struct dma_chan *chan; + if (data->flags & MMC_DATA_READ) + chan = host->dma_rx_channel; + else + chan = host->dma_tx_channel; + dmaengine_terminate_all(chan); - /* if config for dma */ - if (chan) { - if (err) - dmaengine_terminate_all(chan); - if (data->host_cookie) - dma_unmap_sg(mmc_dev(host->mmc), data->sg, - data->sg_len, dir); - mrq->data->host_cookie = 0; + next->dma_desc = NULL; + next->dma_chan = NULL; } } @@ -648,6 +642,11 @@ static inline void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data) { } +static inline void mmci_dma_finalize(struct mmci_host *host, + struct mmc_data *data) +{ +} + static inline void mmci_dma_data_error(struct mmci_host *host) { } @@ -772,8 +771,10 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, u32 remain, success; /* Terminate the DMA transfer */ - if (dma_inprogress(host)) + if (dma_inprogress(host)) { mmci_dma_data_error(host); + mmci_dma_unmap(host, data); + } /* * Calculate how far we are into the transfer. Note that @@ -812,7 +813,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, if (status & MCI_DATAEND || data->error) { if (dma_inprogress(host)) - mmci_dma_unmap(host, data); + mmci_dma_finalize(host, data); mmci_stop_data(host); if (!data->error) @@ -849,8 +850,10 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, if (!cmd->data || cmd->error) { if (host->data) { /* Terminate the DMA transfer */ - if (dma_inprogress(host)) + if (dma_inprogress(host)) { mmci_dma_data_error(host); + mmci_dma_unmap(host, host->data); + } mmci_stop_data(host); } mmci_request_end(host, cmd->mrq);