| Message ID | 20190326070415.11492-1-drake@endlessm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] mmc: alcor: don't write data before command has completed | expand |
On Tue, 26 Mar 2019 at 08:04, Daniel Drake <drake@endlessm.com> wrote: > > The alcor driver is setting up data transfer and submitting the associated > MMC command at the same time. While this works most of the time, it > occasionally causes problems upon write. > > In the working case, after setting up the data transfer and submitting > the MMC command, an interrupt comes in a moment later with CMD_END and > WRITE_BUF_RDY bits set. The data transfer then happens without problem. > > However, on occasion, the interrupt that arrives at that point only > has WRITE_BUF_RDY set. The hardware notifies that it's ready to write > data, but the associated MMC command is still running. Regardless, the > driver was proceeding to write data immediately, and that would then cause > another interrupt indicating data CRC error, and the write would fail. > > Additionally, the transfer setup function alcor_trigger_data_transfer() > was being called 3 times for each write operation, which was confusing > and may be contributing to this issue. > > Solve this by tweaking the driver behaviour to follow the sequence observed > in the original ampe_stor vendor driver: > 1. When starting request handling, write 0 to DATA_XFER_CTRL > 2. Submit the command > 3. Wait for CMD_END interrupt and then trigger data transfer > 4. For the PIO case, trigger the next step of the data transfer only > upon the following DATA_END interrupt, which occurs after the block has > been written. > > I confirmed that the read path still works (DMA & PIO) and also now > presents more consistency with the operations performed by ampe_stor. > > Signed-off-by: Daniel Drake <drake@endlessm.com> Applied for fixes, by adding a fixes and a stable tag, thanks! Kind regards Uffe > --- > drivers/mmc/host/alcor.c | 34 +++++++++++++--------------------- > 1 file changed, 13 insertions(+), 21 deletions(-) > > diff --git a/drivers/mmc/host/alcor.c b/drivers/mmc/host/alcor.c > index 82a97866e0cf4..7c8f203f9a24d 100644 > --- a/drivers/mmc/host/alcor.c > +++ b/drivers/mmc/host/alcor.c > @@ -48,7 +48,6 @@ struct alcor_sdmmc_host { > struct mmc_command *cmd; > struct mmc_data *data; > unsigned int dma_on:1; > - unsigned int early_data:1; > > struct mutex cmd_mutex; > > @@ -144,8 +143,7 @@ static void alcor_data_set_dma(struct alcor_sdmmc_host *host) > host->sg_count--; > } > > -static void alcor_trigger_data_transfer(struct alcor_sdmmc_host *host, > - bool early) > +static void alcor_trigger_data_transfer(struct alcor_sdmmc_host *host) > { > struct alcor_pci_priv *priv = host->alcor_pci; > struct mmc_data *data = host->data; > @@ -155,13 +153,6 @@ static void alcor_trigger_data_transfer(struct alcor_sdmmc_host *host, > ctrl |= AU6601_DATA_WRITE; > > if (data->host_cookie == COOKIE_MAPPED) { > - if (host->early_data) { > - host->early_data = false; > - return; > - } > - > - host->early_data = early; > - > alcor_data_set_dma(host); > ctrl |= AU6601_DATA_DMA_MODE; > host->dma_on = 1; > @@ -231,6 +222,7 @@ static void alcor_prepare_sg_miter(struct alcor_sdmmc_host *host) > static void alcor_prepare_data(struct alcor_sdmmc_host *host, > struct mmc_command *cmd) > { > + struct alcor_pci_priv *priv = host->alcor_pci; > struct mmc_data *data = cmd->data; > > if (!data) > @@ -248,7 +240,7 @@ static void alcor_prepare_data(struct alcor_sdmmc_host *host, > if (data->host_cookie != COOKIE_MAPPED) > alcor_prepare_sg_miter(host); > > - alcor_trigger_data_transfer(host, true); > + alcor_write8(priv, 0, AU6601_DATA_XFER_CTRL); > } > > static void alcor_send_cmd(struct alcor_sdmmc_host *host, > @@ -435,7 +427,7 @@ static int alcor_cmd_irq_done(struct alcor_sdmmc_host *host, u32 intmask) > if (!host->data) > return false; > > - alcor_trigger_data_transfer(host, false); > + alcor_trigger_data_transfer(host); > host->cmd = NULL; > return true; > } > @@ -456,7 +448,7 @@ static void alcor_cmd_irq_thread(struct alcor_sdmmc_host *host, u32 intmask) > if (!host->data) > alcor_request_complete(host, 1); > else > - alcor_trigger_data_transfer(host, false); > + alcor_trigger_data_transfer(host); > host->cmd = NULL; > } > > @@ -487,15 +479,9 @@ static int alcor_data_irq_done(struct alcor_sdmmc_host *host, u32 intmask) > break; > case AU6601_INT_READ_BUF_RDY: > alcor_trf_block_pio(host, true); > - if (!host->blocks) > - break; > - alcor_trigger_data_transfer(host, false); > return 1; > case AU6601_INT_WRITE_BUF_RDY: > alcor_trf_block_pio(host, false); > - if (!host->blocks) > - break; > - alcor_trigger_data_transfer(host, false); > return 1; > case AU6601_INT_DMA_END: > if (!host->sg_count) > @@ -508,8 +494,14 @@ static int alcor_data_irq_done(struct alcor_sdmmc_host *host, u32 intmask) > break; > } > > - if (intmask & AU6601_INT_DATA_END) > - return 0; > + if (intmask & AU6601_INT_DATA_END) { > + if (!host->dma_on && host->blocks) { > + alcor_trigger_data_transfer(host); > + return 1; > + } else { > + return 0; > + } > + } > > return 1; > } > -- > 2.19.1 >
diff --git a/drivers/mmc/host/alcor.c b/drivers/mmc/host/alcor.c index 82a97866e0cf4..7c8f203f9a24d 100644 --- a/drivers/mmc/host/alcor.c +++ b/drivers/mmc/host/alcor.c @@ -48,7 +48,6 @@ struct alcor_sdmmc_host { struct mmc_command *cmd; struct mmc_data *data; unsigned int dma_on:1; - unsigned int early_data:1; struct mutex cmd_mutex; @@ -144,8 +143,7 @@ static void alcor_data_set_dma(struct alcor_sdmmc_host *host) host->sg_count--; } -static void alcor_trigger_data_transfer(struct alcor_sdmmc_host *host, - bool early) +static void alcor_trigger_data_transfer(struct alcor_sdmmc_host *host) { struct alcor_pci_priv *priv = host->alcor_pci; struct mmc_data *data = host->data; @@ -155,13 +153,6 @@ static void alcor_trigger_data_transfer(struct alcor_sdmmc_host *host, ctrl |= AU6601_DATA_WRITE; if (data->host_cookie == COOKIE_MAPPED) { - if (host->early_data) { - host->early_data = false; - return; - } - - host->early_data = early; - alcor_data_set_dma(host); ctrl |= AU6601_DATA_DMA_MODE; host->dma_on = 1; @@ -231,6 +222,7 @@ static void alcor_prepare_sg_miter(struct alcor_sdmmc_host *host) static void alcor_prepare_data(struct alcor_sdmmc_host *host, struct mmc_command *cmd) { + struct alcor_pci_priv *priv = host->alcor_pci; struct mmc_data *data = cmd->data; if (!data) @@ -248,7 +240,7 @@ static void alcor_prepare_data(struct alcor_sdmmc_host *host, if (data->host_cookie != COOKIE_MAPPED) alcor_prepare_sg_miter(host); - alcor_trigger_data_transfer(host, true); + alcor_write8(priv, 0, AU6601_DATA_XFER_CTRL); } static void alcor_send_cmd(struct alcor_sdmmc_host *host, @@ -435,7 +427,7 @@ static int alcor_cmd_irq_done(struct alcor_sdmmc_host *host, u32 intmask) if (!host->data) return false; - alcor_trigger_data_transfer(host, false); + alcor_trigger_data_transfer(host); host->cmd = NULL; return true; } @@ -456,7 +448,7 @@ static void alcor_cmd_irq_thread(struct alcor_sdmmc_host *host, u32 intmask) if (!host->data) alcor_request_complete(host, 1); else - alcor_trigger_data_transfer(host, false); + alcor_trigger_data_transfer(host); host->cmd = NULL; } @@ -487,15 +479,9 @@ static int alcor_data_irq_done(struct alcor_sdmmc_host *host, u32 intmask) break; case AU6601_INT_READ_BUF_RDY: alcor_trf_block_pio(host, true); - if (!host->blocks) - break; - alcor_trigger_data_transfer(host, false); return 1; case AU6601_INT_WRITE_BUF_RDY: alcor_trf_block_pio(host, false); - if (!host->blocks) - break; - alcor_trigger_data_transfer(host, false); return 1; case AU6601_INT_DMA_END: if (!host->sg_count) @@ -508,8 +494,14 @@ static int alcor_data_irq_done(struct alcor_sdmmc_host *host, u32 intmask) break; } - if (intmask & AU6601_INT_DATA_END) - return 0; + if (intmask & AU6601_INT_DATA_END) { + if (!host->dma_on && host->blocks) { + alcor_trigger_data_transfer(host); + return 1; + } else { + return 0; + } + } return 1; }
The alcor driver is setting up data transfer and submitting the associated MMC command at the same time. While this works most of the time, it occasionally causes problems upon write. In the working case, after setting up the data transfer and submitting the MMC command, an interrupt comes in a moment later with CMD_END and WRITE_BUF_RDY bits set. The data transfer then happens without problem. However, on occasion, the interrupt that arrives at that point only has WRITE_BUF_RDY set. The hardware notifies that it's ready to write data, but the associated MMC command is still running. Regardless, the driver was proceeding to write data immediately, and that would then cause another interrupt indicating data CRC error, and the write would fail. Additionally, the transfer setup function alcor_trigger_data_transfer() was being called 3 times for each write operation, which was confusing and may be contributing to this issue. Solve this by tweaking the driver behaviour to follow the sequence observed in the original ampe_stor vendor driver: 1. When starting request handling, write 0 to DATA_XFER_CTRL 2. Submit the command 3. Wait for CMD_END interrupt and then trigger data transfer 4. For the PIO case, trigger the next step of the data transfer only upon the following DATA_END interrupt, which occurs after the block has been written. I confirmed that the read path still works (DMA & PIO) and also now presents more consistency with the operations performed by ampe_stor. Signed-off-by: Daniel Drake <drake@endlessm.com> --- drivers/mmc/host/alcor.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)