| Message ID | 1402351255-7423-1-git-send-email-sonnyrao@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sonny, I have missed this patch. You finally choose to take extra interrupt handling. If it is not harm, it's fine. Please check one thing below. On Tue, June 10, 2014, Sonny Rao wrote: > This patch changes the fifo reset code to follow the reset procedure > outlined in the documentation of Synopsys Mobile storage host databook. > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> > --- > v2: Add Generic DMA support > per the documentation, move interrupt clear before wait > make the test for DMA host->use_dma rather than host->using_dma > add proper return values (although it appears no caller checks) > v3: rename fifo reset function, and change callers > use this combined reset function in dw_mci_resume() > just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() > use DMA reset bit for all systems which use DMA > remove extra IDMAC reset in dw_mci_work_routine_card() > do CIU clock update in error path, if CIU reset cleared > v4: remove comment about FIFO reset in dw_mci_work_routine_card() > move down error message when control reset clears but others don't > and clarify the error stating that we will still update clocks > make flags for all reset bits a macro > > drivers/mmc/host/dw_mmc.c | 86 ++++++++++++++++++++++++++++++++++------------- > drivers/mmc/host/dw_mmc.h | 5 +++ > 2 files changed, 68 insertions(+), 23 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 55cd110..1d6d984 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = { > 0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee, > }; > > -static inline bool dw_mci_fifo_reset(struct dw_mci *host); > -static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host); > +static inline bool dw_mci_reset(struct dw_mci *host); > > #if defined(CONFIG_DEBUG_FS) > static int dw_mci_req_show(struct seq_file *s, void *v) > @@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data) > * After an error, there may be data lingering > * in the FIFO > */ > - dw_mci_fifo_reset(host); > + dw_mci_reset(host); > } else { > data->bytes_xfered = data->blocks * data->blksz; > data->error = 0; > @@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv) > > /* CMD error in data command */ > if (mrq->cmd->error && mrq->data) > - dw_mci_fifo_reset(host); > + dw_mci_reset(host); > > host->cmd = NULL; > host->data = NULL; > @@ -1982,14 +1981,8 @@ static void dw_mci_work_routine_card(struct work_struct *work) > } > > /* Power down slot */ > - if (present == 0) { > - /* Clear down the FIFO */ > - dw_mci_fifo_reset(host); > -#ifdef CONFIG_MMC_DW_IDMAC > - dw_mci_idmac_reset(host); > -#endif > - > - } > + if (present == 0) > + dw_mci_reset(host); > > spin_unlock_bh(&host->lock); > > @@ -2323,8 +2316,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset) > return false; > } > > -static inline bool dw_mci_fifo_reset(struct dw_mci *host) > +static inline bool dw_mci_reset(struct dw_mci *host) > { > + u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET; > + bool ret = false; > + > /* > * Reseting generates a block interrupt, hence setting > * the scatter-gather pointer to NULL. > @@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host) > host->sg = NULL; > } > > - return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET); > -} > + if (host->use_dma) > + flags |= SDMMC_CTRL_DMA_RESET; > > -static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host) > -{ > - return dw_mci_ctrl_reset(host, > - SDMMC_CTRL_FIFO_RESET | > - SDMMC_CTRL_RESET | > - SDMMC_CTRL_DMA_RESET); > + if (dw_mci_ctrl_reset(host, flags)) { > + /* > + * In all cases we clear the RAWINTS register to clear any > + * interrupts. > + */ > + mci_writel(host, RINTSTS, 0xFFFFFFFF); > + > + /* if using dma we wait for dma_req to clear */ > + if (host->use_dma) { > + unsigned long timeout = jiffies + msecs_to_jiffies(500); > + u32 status; > + do { > + status = mci_readl(host, STATUS); > + if (!(status & SDMMC_STATUS_DMA_REQ)) > + break; > + cpu_relax(); > + } while (time_before(jiffies, timeout)); > + > + if (status & SDMMC_STATUS_DMA_REQ) { > + dev_err(host->dev, > + "%s: Timeout waiting for dma_req to " > + "clear during reset", __func__); > + goto ciu_out; > + } > + > + /* when using DMA next we reset the fifo again */ > + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET)) > + goto ciu_out; > + } > + } else { > + /* if the controller reset bit did clear, then set clock regs */ > + if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) { > + dev_err(host->dev, "%s: fifo/dma reset bits didn't " > + "clear but ciu was reset, doing clock update.", > + __func__); > + goto ciu_out; > + } > + } > + > + if (IS_ENABLED(CONFIG_MMC_DW_IDMAC)) > + /* It is also recommended that we reset and reprogram idmac */ > + dw_mci_idmac_reset(host); > + > + ret = true; > + > +ciu_out: > + /* After a CTRL reset we need to have CIU set clock registers */ > + mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0); > + > + return ret; > } > > #ifdef CONFIG_OF > @@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host) > } > > /* Reset all blocks */ > - if (!dw_mci_ctrl_all_reset(host)) > + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) > return -ENODEV; > > host->dma_ops = host->pdata->dma_ops; > @@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host) > } > } > > - if (!dw_mci_ctrl_all_reset(host)) { > + if (!dw_mci_reset(host)) { Do you have any reason to use dw_mci_reset() unlike doing on probing? Thanks, Seungwon Jeon -- 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 Thu, Jul 10, 2014 at 5:28 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > Hi Sonny, > > I have missed this patch. > > You finally choose to take extra interrupt handling. > If it is not harm, it's fine. Hi, thanks for coming back to it. Based on my tracing, the interrupt seems to be okay and is just ignored. > > Please check one thing below. > > On Tue, June 10, 2014, Sonny Rao wrote: >> This patch changes the fifo reset code to follow the reset procedure >> outlined in the documentation of Synopsys Mobile storage host databook. >> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >> --- >> v2: Add Generic DMA support >> per the documentation, move interrupt clear before wait >> make the test for DMA host->use_dma rather than host->using_dma >> add proper return values (although it appears no caller checks) >> v3: rename fifo reset function, and change callers >> use this combined reset function in dw_mci_resume() >> just one caller left (probe), so get rid of dw_mci_ctrl_all_reset() >> use DMA reset bit for all systems which use DMA >> remove extra IDMAC reset in dw_mci_work_routine_card() >> do CIU clock update in error path, if CIU reset cleared >> v4: remove comment about FIFO reset in dw_mci_work_routine_card() >> move down error message when control reset clears but others don't >> and clarify the error stating that we will still update clocks >> make flags for all reset bits a macro >> >> drivers/mmc/host/dw_mmc.c | 86 ++++++++++++++++++++++++++++++++++------------- >> drivers/mmc/host/dw_mmc.h | 5 +++ >> 2 files changed, 68 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 55cd110..1d6d984 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = { >> 0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee, >> }; >> >> -static inline bool dw_mci_fifo_reset(struct dw_mci *host); >> -static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host); >> +static inline bool dw_mci_reset(struct dw_mci *host); >> >> #if defined(CONFIG_DEBUG_FS) >> static int dw_mci_req_show(struct seq_file *s, void *v) >> @@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data) >> * After an error, there may be data lingering >> * in the FIFO >> */ >> - dw_mci_fifo_reset(host); >> + dw_mci_reset(host); >> } else { >> data->bytes_xfered = data->blocks * data->blksz; >> data->error = 0; >> @@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv) >> >> /* CMD error in data command */ >> if (mrq->cmd->error && mrq->data) >> - dw_mci_fifo_reset(host); >> + dw_mci_reset(host); >> >> host->cmd = NULL; >> host->data = NULL; >> @@ -1982,14 +1981,8 @@ static void dw_mci_work_routine_card(struct work_struct *work) >> } >> >> /* Power down slot */ >> - if (present == 0) { >> - /* Clear down the FIFO */ >> - dw_mci_fifo_reset(host); >> -#ifdef CONFIG_MMC_DW_IDMAC >> - dw_mci_idmac_reset(host); >> -#endif >> - >> - } >> + if (present == 0) >> + dw_mci_reset(host); >> >> spin_unlock_bh(&host->lock); >> >> @@ -2323,8 +2316,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset) >> return false; >> } >> >> -static inline bool dw_mci_fifo_reset(struct dw_mci *host) >> +static inline bool dw_mci_reset(struct dw_mci *host) >> { >> + u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET; >> + bool ret = false; >> + >> /* >> * Reseting generates a block interrupt, hence setting >> * the scatter-gather pointer to NULL. >> @@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host) >> host->sg = NULL; >> } >> >> - return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET); >> -} >> + if (host->use_dma) >> + flags |= SDMMC_CTRL_DMA_RESET; >> >> -static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host) >> -{ >> - return dw_mci_ctrl_reset(host, >> - SDMMC_CTRL_FIFO_RESET | >> - SDMMC_CTRL_RESET | >> - SDMMC_CTRL_DMA_RESET); >> + if (dw_mci_ctrl_reset(host, flags)) { >> + /* >> + * In all cases we clear the RAWINTS register to clear any >> + * interrupts. >> + */ >> + mci_writel(host, RINTSTS, 0xFFFFFFFF); >> + >> + /* if using dma we wait for dma_req to clear */ >> + if (host->use_dma) { >> + unsigned long timeout = jiffies + msecs_to_jiffies(500); >> + u32 status; >> + do { >> + status = mci_readl(host, STATUS); >> + if (!(status & SDMMC_STATUS_DMA_REQ)) >> + break; >> + cpu_relax(); >> + } while (time_before(jiffies, timeout)); >> + >> + if (status & SDMMC_STATUS_DMA_REQ) { >> + dev_err(host->dev, >> + "%s: Timeout waiting for dma_req to " >> + "clear during reset", __func__); >> + goto ciu_out; >> + } >> + >> + /* when using DMA next we reset the fifo again */ >> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET)) >> + goto ciu_out; >> + } >> + } else { >> + /* if the controller reset bit did clear, then set clock regs */ >> + if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) { >> + dev_err(host->dev, "%s: fifo/dma reset bits didn't " >> + "clear but ciu was reset, doing clock update.", >> + __func__); >> + goto ciu_out; >> + } >> + } >> + >> + if (IS_ENABLED(CONFIG_MMC_DW_IDMAC)) >> + /* It is also recommended that we reset and reprogram idmac */ >> + dw_mci_idmac_reset(host); >> + >> + ret = true; >> + >> +ciu_out: >> + /* After a CTRL reset we need to have CIU set clock registers */ >> + mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0); >> + >> + return ret; >> } >> >> #ifdef CONFIG_OF >> @@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host) >> } >> >> /* Reset all blocks */ >> - if (!dw_mci_ctrl_all_reset(host)) >> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) >> return -ENODEV; >> >> host->dma_ops = host->pdata->dma_ops; >> @@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host) >> } >> } >> >> - if (!dw_mci_ctrl_all_reset(host)) { >> + if (!dw_mci_reset(host)) { > Do you have any reason to use dw_mci_reset() unlike doing on probing? I really wanted to use dw_mci_reset() everwhere, including probe, because that would be simplest, where there is just one reset function always, but the host structure is not completely set up at probe time, so the code in dw_mci_reset() where we try to send a command to update clock fails, so that's why I had to just do a reset. > Thanks, > Seungwon Jeon > -- 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 Fri, July 11, 2014, Sonny Rao wrote: > On Thu, Jul 10, 2014 at 5:28 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > > Hi Sonny, > > > > I have missed this patch. > > > > You finally choose to take extra interrupt handling. > > If it is not harm, it's fine. > > Hi, thanks for coming back to it. Based on my tracing, the interrupt > seems to be okay and is just ignored. > > >> -static inline bool dw_mci_fifo_reset(struct dw_mci *host) > >> +static inline bool dw_mci_reset(struct dw_mci *host) "inline" is no needed anymore. > >> { > >> + u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET; > >> + bool ret = false; > >> + > >> /* > >> * Reseting generates a block interrupt, hence setting > >> * the scatter-gather pointer to NULL. > >> @@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host) > >> host->sg = NULL; > >> } > >> > >> - return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET); > >> -} > >> + if (host->use_dma) > >> + flags |= SDMMC_CTRL_DMA_RESET; > >> > >> -static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host) > >> -{ > >> - return dw_mci_ctrl_reset(host, > >> - SDMMC_CTRL_FIFO_RESET | > >> - SDMMC_CTRL_RESET | > >> - SDMMC_CTRL_DMA_RESET); > >> + if (dw_mci_ctrl_reset(host, flags)) { > >> + /* > >> + * In all cases we clear the RAWINTS register to clear any > >> + * interrupts. > >> + */ > >> + mci_writel(host, RINTSTS, 0xFFFFFFFF); > >> + > >> + /* if using dma we wait for dma_req to clear */ > >> + if (host->use_dma) { > >> + unsigned long timeout = jiffies + msecs_to_jiffies(500); > >> + u32 status; > >> + do { > >> + status = mci_readl(host, STATUS); > >> + if (!(status & SDMMC_STATUS_DMA_REQ)) > >> + break; > >> + cpu_relax(); > >> + } while (time_before(jiffies, timeout)); > >> + > >> + if (status & SDMMC_STATUS_DMA_REQ) { > >> + dev_err(host->dev, > >> + "%s: Timeout waiting for dma_req to " > >> + "clear during reset", __func__); > >> + goto ciu_out; > >> + } > >> + > >> + /* when using DMA next we reset the fifo again */ > >> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET)) > >> + goto ciu_out; > >> + } > >> + } else { > >> + /* if the controller reset bit did clear, then set clock regs */ > >> + if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) { > >> + dev_err(host->dev, "%s: fifo/dma reset bits didn't " > >> + "clear but ciu was reset, doing clock update.", > >> + __func__); > >> + goto ciu_out; > >> + } > >> + } > >> + > >> + if (IS_ENABLED(CONFIG_MMC_DW_IDMAC)) > >> + /* It is also recommended that we reset and reprogram idmac */ > >> + dw_mci_idmac_reset(host); > >> + > >> + ret = true; > >> + > >> +ciu_out: > >> + /* After a CTRL reset we need to have CIU set clock registers */ > >> + mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0); > >> + > >> + return ret; > >> } > >> > >> #ifdef CONFIG_OF > >> @@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host) > >> } > >> > >> /* Reset all blocks */ > >> - if (!dw_mci_ctrl_all_reset(host)) > >> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) > >> return -ENODEV; > >> > >> host->dma_ops = host->pdata->dma_ops; > >> @@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host) > >> } > >> } > >> > >> - if (!dw_mci_ctrl_all_reset(host)) { > >> + if (!dw_mci_reset(host)) { > > Do you have any reason to use dw_mci_reset() unlike doing on probing? > > I really wanted to use dw_mci_reset() everwhere, including probe, > because that would be simplest, where there is just one reset function > always, but the host structure is not completely set up at probe time, > so the code in dw_mci_reset() where we try to send a command to update > clock fails, so that's why I had to just do a reset. Yes, if we can keep one interface, it would be good. But can you check below? Did you see the kernel panic on probing with "host->cur_slot" is NULL? If right, resume seems not different from probe in case of removable type. And dw_mci_idmac_reset() is redundant when dw_mci_reset() is used at resume. I think dw_mci_ctrl_reset() can be also used at resume like at probe for simple way. For safety's sake, "host->cur_slot" should be guaranteed it's not NULL. Thanks, Seungwon Jeon -- 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 Fri, Jul 11, 2014 at 3:20 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > On Fri, July 11, 2014, Sonny Rao wrote: >> On Thu, Jul 10, 2014 at 5:28 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: >> > Hi Sonny, >> > >> > I have missed this patch. >> > >> > You finally choose to take extra interrupt handling. >> > If it is not harm, it's fine. >> >> Hi, thanks for coming back to it. Based on my tracing, the interrupt >> seems to be okay and is just ignored. >> >> >> -static inline bool dw_mci_fifo_reset(struct dw_mci *host) >> >> +static inline bool dw_mci_reset(struct dw_mci *host) > "inline" is no needed anymore. > >> >> { >> >> + u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET; >> >> + bool ret = false; >> >> + >> >> /* >> >> * Reseting generates a block interrupt, hence setting >> >> * the scatter-gather pointer to NULL. >> >> @@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host) >> >> host->sg = NULL; >> >> } >> >> >> >> - return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET); >> >> -} >> >> + if (host->use_dma) >> >> + flags |= SDMMC_CTRL_DMA_RESET; >> >> >> >> -static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host) >> >> -{ >> >> - return dw_mci_ctrl_reset(host, >> >> - SDMMC_CTRL_FIFO_RESET | >> >> - SDMMC_CTRL_RESET | >> >> - SDMMC_CTRL_DMA_RESET); >> >> + if (dw_mci_ctrl_reset(host, flags)) { >> >> + /* >> >> + * In all cases we clear the RAWINTS register to clear any >> >> + * interrupts. >> >> + */ >> >> + mci_writel(host, RINTSTS, 0xFFFFFFFF); >> >> + >> >> + /* if using dma we wait for dma_req to clear */ >> >> + if (host->use_dma) { >> >> + unsigned long timeout = jiffies + msecs_to_jiffies(500); >> >> + u32 status; >> >> + do { >> >> + status = mci_readl(host, STATUS); >> >> + if (!(status & SDMMC_STATUS_DMA_REQ)) >> >> + break; >> >> + cpu_relax(); >> >> + } while (time_before(jiffies, timeout)); >> >> + >> >> + if (status & SDMMC_STATUS_DMA_REQ) { >> >> + dev_err(host->dev, >> >> + "%s: Timeout waiting for dma_req to " >> >> + "clear during reset", __func__); >> >> + goto ciu_out; >> >> + } >> >> + >> >> + /* when using DMA next we reset the fifo again */ >> >> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET)) >> >> + goto ciu_out; >> >> + } >> >> + } else { >> >> + /* if the controller reset bit did clear, then set clock regs */ >> >> + if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) { >> >> + dev_err(host->dev, "%s: fifo/dma reset bits didn't " >> >> + "clear but ciu was reset, doing clock update.", >> >> + __func__); >> >> + goto ciu_out; >> >> + } >> >> + } >> >> + >> >> + if (IS_ENABLED(CONFIG_MMC_DW_IDMAC)) >> >> + /* It is also recommended that we reset and reprogram idmac */ >> >> + dw_mci_idmac_reset(host); >> >> + >> >> + ret = true; >> >> + >> >> +ciu_out: >> >> + /* After a CTRL reset we need to have CIU set clock registers */ >> >> + mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0); >> >> + >> >> + return ret; >> >> } >> >> >> >> #ifdef CONFIG_OF >> >> @@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host) >> >> } >> >> >> >> /* Reset all blocks */ >> >> - if (!dw_mci_ctrl_all_reset(host)) >> >> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) >> >> return -ENODEV; >> >> >> >> host->dma_ops = host->pdata->dma_ops; >> >> @@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host) >> >> } >> >> } >> >> >> >> - if (!dw_mci_ctrl_all_reset(host)) { >> >> + if (!dw_mci_reset(host)) { >> > Do you have any reason to use dw_mci_reset() unlike doing on probing? >> >> I really wanted to use dw_mci_reset() everwhere, including probe, >> because that would be simplest, where there is just one reset function >> always, but the host structure is not completely set up at probe time, >> so the code in dw_mci_reset() where we try to send a command to update >> clock fails, so that's why I had to just do a reset. > > Yes, if we can keep one interface, it would be good. > But can you check below? > Did you see the kernel panic on probing with "host->cur_slot" is NULL? Yes, I think that was the one. > If right, resume seems not different from probe in case of removable type. > And dw_mci_idmac_reset() is redundant when dw_mci_reset() is used at resume. > > I think dw_mci_ctrl_reset() can be also used at resume like at probe for simple way. > For safety's sake, "host->cur_slot" should be guaranteed it's not NULL. Ok, I didn't try on removable, but I can change it to match probe, thanks for looking at this. > > Thanks, > Seungwon Jeon > -- 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/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 55cd110..1d6d984 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -111,8 +111,7 @@ static const u8 tuning_blk_pattern_8bit[] = { 0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee, }; -static inline bool dw_mci_fifo_reset(struct dw_mci *host); -static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host); +static inline bool dw_mci_reset(struct dw_mci *host); #if defined(CONFIG_DEBUG_FS) static int dw_mci_req_show(struct seq_file *s, void *v) @@ -1254,7 +1253,7 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data) * After an error, there may be data lingering * in the FIFO */ - dw_mci_fifo_reset(host); + dw_mci_reset(host); } else { data->bytes_xfered = data->blocks * data->blksz; data->error = 0; @@ -1371,7 +1370,7 @@ static void dw_mci_tasklet_func(unsigned long priv) /* CMD error in data command */ if (mrq->cmd->error && mrq->data) - dw_mci_fifo_reset(host); + dw_mci_reset(host); host->cmd = NULL; host->data = NULL; @@ -1982,14 +1981,8 @@ static void dw_mci_work_routine_card(struct work_struct *work) } /* Power down slot */ - if (present == 0) { - /* Clear down the FIFO */ - dw_mci_fifo_reset(host); -#ifdef CONFIG_MMC_DW_IDMAC - dw_mci_idmac_reset(host); -#endif - - } + if (present == 0) + dw_mci_reset(host); spin_unlock_bh(&host->lock); @@ -2323,8 +2316,11 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset) return false; } -static inline bool dw_mci_fifo_reset(struct dw_mci *host) +static inline bool dw_mci_reset(struct dw_mci *host) { + u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET; + bool ret = false; + /* * Reseting generates a block interrupt, hence setting * the scatter-gather pointer to NULL. @@ -2334,15 +2330,59 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host) host->sg = NULL; } - return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET); -} + if (host->use_dma) + flags |= SDMMC_CTRL_DMA_RESET; -static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host) -{ - return dw_mci_ctrl_reset(host, - SDMMC_CTRL_FIFO_RESET | - SDMMC_CTRL_RESET | - SDMMC_CTRL_DMA_RESET); + if (dw_mci_ctrl_reset(host, flags)) { + /* + * In all cases we clear the RAWINTS register to clear any + * interrupts. + */ + mci_writel(host, RINTSTS, 0xFFFFFFFF); + + /* if using dma we wait for dma_req to clear */ + if (host->use_dma) { + unsigned long timeout = jiffies + msecs_to_jiffies(500); + u32 status; + do { + status = mci_readl(host, STATUS); + if (!(status & SDMMC_STATUS_DMA_REQ)) + break; + cpu_relax(); + } while (time_before(jiffies, timeout)); + + if (status & SDMMC_STATUS_DMA_REQ) { + dev_err(host->dev, + "%s: Timeout waiting for dma_req to " + "clear during reset", __func__); + goto ciu_out; + } + + /* when using DMA next we reset the fifo again */ + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET)) + goto ciu_out; + } + } else { + /* if the controller reset bit did clear, then set clock regs */ + if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) { + dev_err(host->dev, "%s: fifo/dma reset bits didn't " + "clear but ciu was reset, doing clock update.", + __func__); + goto ciu_out; + } + } + + if (IS_ENABLED(CONFIG_MMC_DW_IDMAC)) + /* It is also recommended that we reset and reprogram idmac */ + dw_mci_idmac_reset(host); + + ret = true; + +ciu_out: + /* After a CTRL reset we need to have CIU set clock registers */ + mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0); + + return ret; } #ifdef CONFIG_OF @@ -2555,7 +2595,7 @@ int dw_mci_probe(struct dw_mci *host) } /* Reset all blocks */ - if (!dw_mci_ctrl_all_reset(host)) + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) return -ENODEV; host->dma_ops = host->pdata->dma_ops; @@ -2744,7 +2784,7 @@ int dw_mci_resume(struct dw_mci *host) } } - if (!dw_mci_ctrl_all_reset(host)) { + if (!dw_mci_reset(host)) { ret = -ENODEV; return ret; } diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 6bf24ab..9ab8ccd 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -129,6 +129,7 @@ #define SDMMC_CMD_INDX(n) ((n) & 0x1F) /* Status register defines */ #define SDMMC_GET_FCNT(x) (((x)>>17) & 0x1FFF) +#define SDMMC_STATUS_DMA_REQ BIT(31) /* FIFOTH register defines */ #define SDMMC_SET_FIFOTH(m, r, t) (((m) & 0x7) << 28 | \ ((r) & 0xFFF) << 16 | \ @@ -150,6 +151,10 @@ /* Card read threshold */ #define SDMMC_SET_RD_THLD(v, x) (((v) & 0x1FFF) << 16 | (x)) +/* All ctrl reset bits */ +#define SDMMC_CTRL_ALL_RESET_FLAGS \ + (SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET | SDMMC_CTRL_DMA_RESET) + /* Register access macros */ #define mci_readl(dev, reg) \ __raw_readl((dev)->regs + SDMMC_##reg)