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

[Seungwon Jeon]

On Fri, July 11, 2014, Sonny Rao wrote:
> On Thu, Jul 10, 2014 at 5:28 AM, Seungwon Jeon <tgih.jun at 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