Messages in this thread | | | Date | Mon, 8 Apr 2013 16:09:39 -0700 | Subject | Re: [PATCH] RFC: mmc: dw_mmc: Always go to STATE_DATA_BUSY from STATE_DATA_ERROR | From | Doug Anderson <> |
| |
Seungwon,
On Mon, Apr 8, 2013 at 5:17 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > I guess Doug are debugging it with wifi, right?
Yes, we're debugging it on the Samsung ARM Chromebook on a part that has an SDIO WiFi module by Marvell. Bing Zhao (CCed) has a unit in hand that generates lots of CRC errors and has been testing patches I've sent him.
> The problem happens when dw_mci_stop_dma is called in the middle of data transfers. > If data error occurs in the end of block, EVENT_XFER_COMPLETE might be set. So, it's fine. > Actually, dw_mci_idmac_stop_dma stops the dma working, there is no further interrupt for dma completion.
That sounds right to me.
> There are two solutions we have applied.
I'm a little confused. Have you already applied one or both of the solutions you list below, or are you proposing them as alternates to the patch I submitted?
> #1. deferring the call of dw_mci_stop_dma until EVENT_XFER_COMPLETE flag is set into pending_events. > In this case, dma transfer will be continued with error. > > @@ -1062,7 +1062,6 @@ static void dw_mci_tasklet_func(unsigned long priv) > case STATE_SENDING_DATA: > if (test_and_clear_bit(EVENT_DATA_ERROR, > &host->pending_events)) { > - dw_mci_stop_dma(host); > if (data->stop) > send_stop_cmd(host, data); > state = STATE_DATA_ERROR; > @@ -1155,6 +1154,9 @@ static void dw_mci_tasklet_func(unsigned long priv) > &host->pending_events)) > break; > > + dw_mci_stop_dma(host); > + set_bit(EVENT_XFER_COMPLETE, &host->completed_events); > + > state = STATE_DATA_BUSY; > break;
I can't say that I'm quite familiar enough with the intricate details of the driver to know whether this is a good idea or guaranteed to work. Do we really think that we'll still get the end of the transfer properly if we've seen an error already? I worry that we won't.
> #2. set EVENT_XFER_COMPLETE flag when dw_mci_stop_dma is called regardless using_dma. > > @@ -299,10 +299,9 @@ static void dw_mci_stop_dma(struct dw_mci *host) > if (host->using_dma) { > host->dma_ops->stop(host); > host->dma_ops->cleanup(host); > - } else { > - /* Data transfer was stopped by the interrupt handler */ > - set_bit(EVENT_XFER_COMPLETE, &host->pending_events); > } > + > + set_bit(EVENT_XFER_COMPLETE, &host->pending_events); > }
This is fairly similar to my patch but goes further. I believe my patch has this effect but only for the call to dw_mci_stop_dma() in STATE_SENDING_DATA in the tasklet. Your affects all 3 calls to dw_mci_stop_dma().
This seems reasonable but I don't have confidence in my understanding of this driver's state machine (especially with regards to the error conditions) that I can say which is better. If you think that this is a more correct solution than mine then we can give it some testing.
Thanks!
-Doug
| |