[PATCH] mmc: dw_mmc: change to use recommended reset procedure

On Wed, May 28, 2014 at 10:17 PM, Jaehoon Chung [off-list ref] wrote:

Hi, Sonny.

On 05/29/2014 09:35 AM, Sonny Rao wrote:

quoted

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 <redacted>
Signed-off-by: Yuvaraj Kumar C D <redacted>
---
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

 drivers/mmc/host/dw_mmc.c | 85 +++++++++++++++++++++++++++++++++++------------
 drivers/mmc/host/dw_mmc.h |  1 +
 2 files changed, 64 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 55cd110..988492c 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,9 @@ static void dw_mci_work_routine_card(struct work_struct *work)
                      }

                      /* Power down slot */
-                     if (present == 0) {
+                     if (present == 0)
                              /* Clear down the FIFO */

Didn't Need to change the Comment?

Well, it does still clear the fifo, but I can remove the comment since
we are now doing more than that.

quoted

-                             dw_mci_fifo_reset(host);
-#ifdef CONFIG_MMC_DW_IDMAC
-                             dw_mci_idmac_reset(host);
-#endif
-
-                     }
+                             dw_mci_reset(host);

                      spin_unlock_bh(&host->lock);

@@ -2323,8 +2317,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 +2331,57 @@ 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 {
+             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
+
+             /* if the controller reset bit did clear, then set clock regs */

I have some confusion at this point. you added the above error message "Reset bits didn't clear".
But this comment is "if the controller reset bit did clear..".

I think that dev_err message would be better to locate into "if (!(mci_read(host, CTRL) & ...)".

Ok, I'll move this error message inside the if block and make it more specific.

quoted

+             if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET))
+                     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

@@ -2432,6 +2471,7 @@ int dw_mci_probe(struct dw_mci *host)
      const struct dw_mci_drv_data *drv_data = host->drv_data;
      int width, i, ret = 0;
      u32 fifo_size;
+     u32 flags;
      int init_slots = 0;

      if (!host->pdata) {

@@ -2555,7 +2595,8 @@ int dw_mci_probe(struct dw_mci *host)
      }

      /* Reset all blocks */
-     if (!dw_mci_ctrl_all_reset(host))
+     flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET | SDMMC_CTRL_DMA_RESET;

how about adding the SDMMC_CTRL_ALL_RESET macro into header file?
#define SDMMC_CTRL_ALL_RESET (.....)

ok I'll make a macro in dw_mmc.h for this.

quoted

+     if (!dw_mci_ctrl_reset(host, flags))
              return -ENODEV;

      host->dma_ops = host->pdata->dma_ops;

@@ -2744,7 +2785,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..2505804 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 | \