Re: [PATCH v2 3/5] mmc: mediatek: Add PM support for MMC driver

Re: [PATCH v2 3/5] mmc: mediatek: Add PM support for MMC driver

[Ulf Hansson]

On 17 March 2015 at 04:13, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> Add PM support for Mediatek MMC driver
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 86c999b..e02f6a6 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -21,6 +21,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_gpio.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spinlock.h>
>
> @@ -223,6 +224,7 @@
>  #define MSDC_OCR_AVAIL      (MMC_VDD_28_29 | MMC_VDD_29_30 \
>                 | MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33)
>
> +#define MTK_MMC_AUTOSUSPEND_DELAY      500
>  #define CMD_TIMEOUT         (HZ/10 * 5)        /* 100ms x5 */
>  #define DAT_TIMEOUT         (HZ    * 5)        /* 1000ms x5 */
>
> @@ -535,6 +537,38 @@ static void msdc_set_mclk(struct msdc_host *host, int ddr, u32 hz)
>         dev_dbg(host->dev, "sclk: %d, ddr: %d\n", host->sclk, ddr);
>  }
>
> +#ifdef CONFIG_PM

I suggest you move this complete code snippets within the ifdefs for
where the runtime PM callbacks is implemented.

> +static int msdc_gate_clock(struct msdc_host *host)
> +{
> +       int ret = 0;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       /* disable SD/MMC/SDIO bus clock */
> +       sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_MODE, MSDC_MS);

This seems like it also belongs in the ->set_ios() function, when the
rate requested by the core is zero!? Maybe that's already dealt with?

> +       /* turn off SDHC functional clock */
> +       clk_disable(host->src_clk);

Change to clk_disable_unprepare() and move it outside the spin_lock.

> +       spin_unlock_irqrestore(&host->lock, flags);
> +       return ret;
> +}
> +
> +static void msdc_ungate_clock(struct msdc_host *host)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       clk_enable(host->src_clk);

Change to clk_prepare_enable() and move it outside the spin_lock.

> +       /* enable SD/MMC/SDIO bus clock:
> +        * it will be automatically gated when the bus is idle
> +        * (set MSDC_CFG_CKPDN bit to have it always on)
> +        */
> +       sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_MODE, MSDC_SDMMC);
> +       while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
> +               cpu_relax();

I guess you already have this piece of code in ->set_ios() in some form!?

What's missing here is a kind of a save/restore mechanism of the
clock. You need to save the value for the clock in msdc_ungate_clock()
and restore the clock here. Else you might end up ungating the clock
when it actually should remain gated.

> +       spin_unlock_irqrestore(&host->lock, flags);
> +}
> +#endif
> +
>  static inline u32 msdc_cmd_find_resp(struct msdc_host *host,
>                 struct mmc_request *mrq, struct mmc_command *cmd)
>  {
> @@ -702,6 +736,9 @@ static void msdc_request_done(struct msdc_host *host, struct mmc_request *mrq)
>         if (mrq->data)
>                 msdc_unprepare_data(host, mrq);
>         mmc_request_done(host->mmc, mrq);
> +
> +       pm_runtime_mark_last_busy(host->dev);
> +       pm_runtime_put_autosuspend(host->dev);
>  }
>
>  /* returns true if command is fully handled; returns false otherwise */
> @@ -863,6 +900,8 @@ static void msdc_ops_request(struct mmc_host *mmc, struct mmc_request *mrq)
>         }
>         spin_unlock_irqrestore(&host->lock, flags);
>
> +       pm_runtime_get_sync(host->dev);
> +
>         if (mrq->data)
>                 msdc_prepare_data(host, mrq);
>
> @@ -1003,6 +1042,8 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
>
>         if (!IS_ERR(mmc->supply.vqmmc)) {
>
> +               pm_runtime_get_sync(host->dev);
> +
>                 if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
>                         min_uv = 3300000;
>                         max_uv = 3300000;
> @@ -1011,6 +1052,9 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
>                         max_uv = 1800000;
>                 } else {
>                         dev_err(host->dev, "Unsupported signal voltage!\n");
> +
> +                       pm_runtime_mark_last_busy(host->dev);
> +                       pm_runtime_put_autosuspend(host->dev);
>                         return -EINVAL;

I suggest to assign a local "ret" variable which value you can return
at the end of this function. Thus you won't need to duplicate the
pm_runtime*() calls.

>                 }
>
> @@ -1022,6 +1066,8 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
>                 }
>         }
>
> +       pm_runtime_mark_last_busy(host->dev);
> +       pm_runtime_put_autosuspend(host->dev);
>         return ret;
>  }
>
> @@ -1186,6 +1232,8 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>         int ret;
>         u32 ddr = 0;
>
> +       pm_runtime_get_sync(host->dev);
> +
>         if (ios->timing == MMC_TIMING_UHS_DDR50)
>                 ddr = 1;
>
> @@ -1230,6 +1278,9 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
>         if (host->mclk != ios->clock || host->ddr != ddr)
>                 msdc_set_mclk(host, ddr, ios->clock);
> +
> +       pm_runtime_mark_last_busy(host->dev);
> +       pm_runtime_put_autosuspend(host->dev);
>  }
>
>  static struct mmc_host_ops mt_msdc_ops = {
> @@ -1341,6 +1392,11 @@ static int msdc_drv_probe(struct platform_device *pdev)
>         platform_set_drvdata(pdev, mmc);
>         clk_prepare(host->src_clk);
>
> +       pm_runtime_enable(host->dev);
> +       pm_runtime_get_sync(host->dev);
> +       pm_runtime_set_autosuspend_delay(host->dev, MTK_MMC_AUTOSUSPEND_DELAY);
> +       pm_runtime_use_autosuspend(host->dev);

This shall be changed to the following:

pm_runtime_set_active();
pm_runtime_set_autosuspend_delay(host->dev, MTK_MMC_AUTOSUSPEND_DELAY);
pm_runtime_use_autosuspend(host->dev);
pm_runtime_enable(host->dev);

... and to simplify error handling, move it just prior mmc_add_host().

> +
>         ret = devm_request_irq(&pdev->dev, (unsigned int) host->irq, msdc_irq,
>                 IRQF_TRIGGER_LOW | IRQF_ONESHOT, pdev->name, host);
>         if (ret)
> @@ -1348,10 +1404,15 @@ static int msdc_drv_probe(struct platform_device *pdev)
>
>         ret = mmc_add_host(mmc);
>         if (ret)
> -               goto release;
> +               goto end;
>
> +       pm_runtime_mark_last_busy(host->dev);
> +       pm_runtime_put_autosuspend(host->dev);

According to above, remove these pm_runtime_*() calls.

>         return 0;
>
> +end:
> +       pm_runtime_put_sync(host->dev);

According to above, remove the pm_runtime_put_sync().

> +       pm_runtime_disable(host->dev);
>  release:
>         platform_set_drvdata(pdev, NULL);
>         msdc_deinit_hw(host);
> @@ -1364,6 +1425,7 @@ release_mem:
>                 dma_free_coherent(&pdev->dev,
>                         MAX_BD_NUM * sizeof(struct mt_bdma_desc),
>                         host->dma.bd, host->dma.bd_addr);
> +
>  host_free:
>         mmc_free_host(mmc);
>
> @@ -1378,10 +1440,14 @@ static int msdc_drv_remove(struct platform_device *pdev)
>         mmc = platform_get_drvdata(pdev);
>         host = mmc_priv(mmc);
>
> +       pm_runtime_get_sync(host->dev);
> +
>         platform_set_drvdata(pdev, NULL);
>         mmc_remove_host(host->mmc);
>         msdc_deinit_hw(host);
>
> +       pm_runtime_put_sync(host->dev);

Change to pm_runtime_put_noidle() and reverse the order of the call to
pm_runtime_disable().

> +       pm_runtime_disable(host->dev);
>         dma_free_coherent(&pdev->dev,
>                         MAX_GPD_NUM * sizeof(struct mt_gpdma_desc),
>                         host->dma.gpd, host->dma.gpd_addr);
> @@ -1393,6 +1459,31 @@ static int msdc_drv_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +#ifdef CONFIG_PM
> +static int msdc_runtime_suspend(struct device *dev)
> +{
> +       int ret = 0;
> +       struct mmc_host *mmc = dev_get_drvdata(dev);
> +       struct msdc_host *host = mmc_priv(mmc);
> +
> +       ret = msdc_gate_clock(host);
> +       return ret;
> +}
> +
> +static int msdc_runtime_resume(struct device *dev)
> +{
> +       struct mmc_host *mmc = dev_get_drvdata(dev);
> +       struct msdc_host *host = mmc_priv(mmc);
> +
> +       msdc_ungate_clock(host);
> +       return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops msdc_dev_pm_ops = {
> +       SET_RUNTIME_PM_OPS(msdc_runtime_suspend, msdc_runtime_resume, NULL)
> +};
> +
>  static const struct of_device_id msdc_of_ids[] = {
>         {   .compatible = "mediatek,mt8135-mmc", },
>         {}
> @@ -1404,6 +1495,7 @@ static struct platform_driver mt_msdc_driver = {
>         .driver = {
>                 .name = "mtk-msdc",
>                 .of_match_table = msdc_of_ids,
> +               .pm = &msdc_dev_pm_ops,
>         },
>  };
>
> --
> 1.8.1.1.dirty
>

Kind regards
Uffe

答复: [PATCH v2 3/5] mmc: mediatek: Add PM support for MMC driver

[Chaotian Jing (井朝天)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 12267 bytes --]

Dear Ulf,

Thanks for your review,
Please help to check my comments:

> -----邮件原件-----
> 发件人: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> 发送时间: 2015年3月31日 22:47
> 收件人: Chaotian Jing (井朝天)
> 抄送: Rob Herring; Matthias Brugger; Chris Ball; Mark Rutland; JamesJJ Liao
> (廖建智); srv_heupstream; Arnd Bergmann; devicetree@vger.kernel.org;
> HONGZHOU Yang; Catalin Marinas; linux-mmc; Will Deacon;
> linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; Sascha Hauer;
> Yingjoe Chen (陳英洲); Eddie Huang (黃智傑); Bin Zhang (章斌);
> linux-arm-kernel@lists.infradead.org; linux-mediatek@lists.infradead.org
> 主题: Re: [PATCH v2 3/5] mmc: mediatek: Add PM support for MMC driver
> 
> On 17 March 2015 at 04:13, Chaotian Jing <chaotian.jing@mediatek.com>
> wrote:
> > Add PM support for Mediatek MMC driver
> >
> > Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> > ---
> >  drivers/mmc/host/mtk-sd.c | 94
> > ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 93 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 86c999b..e02f6a6 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/of_irq.h>
> >  #include <linux/of_gpio.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>  #include <linux/spinlock.h>
> >
> > @@ -223,6 +224,7 @@
> >  #define MSDC_OCR_AVAIL      (MMC_VDD_28_29 | MMC_VDD_29_30
> \
> >                 | MMC_VDD_30_31 | MMC_VDD_31_32 |
> MMC_VDD_32_33)
> >
> > +#define MTK_MMC_AUTOSUSPEND_DELAY      500
> >  #define CMD_TIMEOUT         (HZ/10 * 5)        /* 100ms x5 */
> >  #define DAT_TIMEOUT         (HZ    * 5)        /* 1000ms x5 */
> >
> > @@ -535,6 +537,38 @@ static void msdc_set_mclk(struct msdc_host *host,
> int ddr, u32 hz)
> >         dev_dbg(host->dev, "sclk: %d, ddr: %d\n", host->sclk, ddr);  }
> >
> > +#ifdef CONFIG_PM
> 
> I suggest you move this complete code snippets within the ifdefs for where the
> runtime PM callbacks is implemented.
> 
I will remove the CONFIG_PM, because that if the kernel config do not enable CONFIG_PM, it still need do gate/ungate clock.
> > +static int msdc_gate_clock(struct msdc_host *host) {
> > +       int ret = 0;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       /* disable SD/MMC/SDIO bus clock */
> > +       sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_MODE,
> MSDC_MS);
> 
> This seems like it also belongs in the ->set_ios() function, when the rate
> requested by the core is zero!? Maybe that's already dealt with?
Yes, but it is not the same, the set_ios() set the bus clock to 0, only need set the MODE to MSDC_MS.
But the gate clock will gate the source clock of MSDC host, when the host is idle, runtime pm will gate it, but the ios->clock is not 0.
> 
> > +       /* turn off SDHC functional clock */
> > +       clk_disable(host->src_clk);
> 
> Change to clk_disable_unprepare() and move it outside the spin_lock.
> 
OK, will fix it at next revision.
> > +       spin_unlock_irqrestore(&host->lock, flags);
> > +       return ret;
> > +}
> > +
> > +static void msdc_ungate_clock(struct msdc_host *host) {
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       clk_enable(host->src_clk);
> 
> Change to clk_prepare_enable() and move it outside the spin_lock.
> 
OK, will fix it at next revision,
> > +       /* enable SD/MMC/SDIO bus clock:
> > +        * it will be automatically gated when the bus is idle
> > +        * (set MSDC_CFG_CKPDN bit to have it always on)
> > +        */
> > +       sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_MODE,
> MSDC_SDMMC);
> > +       while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
> > +               cpu_relax();
> 
> I guess you already have this piece of code in ->set_ios() in some form!?
> 
> What's missing here is a kind of a save/restore mechanism of the clock. You
> need to save the value for the clock in msdc_ungate_clock() and restore the
> clock here. Else you might end up ungating the clock when it actually should
> remain gated.
> 
When we ungate the clock or change the frequency of the clock, both need wait the clock stable.

> > +       spin_unlock_irqrestore(&host->lock, flags); } #endif
> > +
> >  static inline u32 msdc_cmd_find_resp(struct msdc_host *host,
> >                 struct mmc_request *mrq, struct mmc_command *cmd)
> {
> > @@ -702,6 +736,9 @@ static void msdc_request_done(struct msdc_host
> *host, struct mmc_request *mrq)
> >         if (mrq->data)
> >                 msdc_unprepare_data(host, mrq);
> >         mmc_request_done(host->mmc, mrq);
> > +
> > +       pm_runtime_mark_last_busy(host->dev);
> > +       pm_runtime_put_autosuspend(host->dev);
> >  }
> >
> >  /* returns true if command is fully handled; returns false otherwise
> > */ @@ -863,6 +900,8 @@ static void msdc_ops_request(struct mmc_host
> *mmc, struct mmc_request *mrq)
> >         }
> >         spin_unlock_irqrestore(&host->lock, flags);
> >
> > +       pm_runtime_get_sync(host->dev);
> > +
> >         if (mrq->data)
> >                 msdc_prepare_data(host, mrq);
> >
> > @@ -1003,6 +1042,8 @@ static int msdc_ops_switch_volt(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> >
> >         if (!IS_ERR(mmc->supply.vqmmc)) {
> >
> > +               pm_runtime_get_sync(host->dev);
> > +
> >                 if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> >                         min_uv = 3300000;
> >                         max_uv = 3300000; @@ -1011,6 +1052,9 @@
> static
> > int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
> >                         max_uv = 1800000;
> >                 } else {
> >                         dev_err(host->dev, "Unsupported signal
> > voltage!\n");
> > +
> > +                       pm_runtime_mark_last_busy(host->dev);
> > +                       pm_runtime_put_autosuspend(host->dev);
> >                         return -EINVAL;
> 
> I suggest to assign a local "ret" variable which value you can return at the end
> of this function. Thus you won't need to duplicate the
> pm_runtime*() calls.
> 
OK, will fix at next revision.
> >                 }
> >
> > @@ -1022,6 +1066,8 @@ static int msdc_ops_switch_volt(struct mmc_host
> *mmc, struct mmc_ios *ios)
> >                 }
> >         }
> >
> > +       pm_runtime_mark_last_busy(host->dev);
> > +       pm_runtime_put_autosuspend(host->dev);
> >         return ret;
> >  }
> >
> > @@ -1186,6 +1232,8 @@ static void msdc_ops_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios)
> >         int ret;
> >         u32 ddr = 0;
> >
> > +       pm_runtime_get_sync(host->dev);
> > +
> >         if (ios->timing == MMC_TIMING_UHS_DDR50)
> >                 ddr = 1;
> >
> > @@ -1230,6 +1278,9 @@ static void msdc_ops_set_ios(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> >
> >         if (host->mclk != ios->clock || host->ddr != ddr)
> >                 msdc_set_mclk(host, ddr, ios->clock);
> > +
> > +       pm_runtime_mark_last_busy(host->dev);
> > +       pm_runtime_put_autosuspend(host->dev);
> >  }
> >
> >  static struct mmc_host_ops mt_msdc_ops = { @@ -1341,6 +1392,11 @@
> > static int msdc_drv_probe(struct platform_device *pdev)
> >         platform_set_drvdata(pdev, mmc);
> >         clk_prepare(host->src_clk);
> >
> > +       pm_runtime_enable(host->dev);
> > +       pm_runtime_get_sync(host->dev);
> > +       pm_runtime_set_autosuspend_delay(host->dev,
> MTK_MMC_AUTOSUSPEND_DELAY);
> > +       pm_runtime_use_autosuspend(host->dev);
> 
> This shall be changed to the following:
> 
Ok, will fix it.
> pm_runtime_set_active();
> pm_runtime_set_autosuspend_delay(host->dev,
> MTK_MMC_AUTOSUSPEND_DELAY);
> pm_runtime_use_autosuspend(host->dev);
> pm_runtime_enable(host->dev);
> 
> ... and to simplify error handling, move it just prior mmc_add_host().
> 	
Will fix it at next revision.
> > +
> >         ret = devm_request_irq(&pdev->dev, (unsigned int) host->irq,
> msdc_irq,
> >                 IRQF_TRIGGER_LOW | IRQF_ONESHOT, pdev->name,
> host);
> >         if (ret)
> > @@ -1348,10 +1404,15 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >
> >         ret = mmc_add_host(mmc);
> >         if (ret)
> > -               goto release;
> > +               goto end;
> >
> > +       pm_runtime_mark_last_busy(host->dev);
> > +       pm_runtime_put_autosuspend(host->dev);
> 
> According to above, remove these pm_runtime_*() calls.
> 
Okay.
> >         return 0;
> >
> > +end:
> > +       pm_runtime_put_sync(host->dev);
> 
> According to above, remove the pm_runtime_put_sync().
> Will remove it at next revision,
> > +       pm_runtime_disable(host->dev);
> >  release:
> >         platform_set_drvdata(pdev, NULL);
> >         msdc_deinit_hw(host);
> > @@ -1364,6 +1425,7 @@ release_mem:
> >                 dma_free_coherent(&pdev->dev,
> >                         MAX_BD_NUM * sizeof(struct
> mt_bdma_desc),
> >                         host->dma.bd, host->dma.bd_addr);
> > +
> >  host_free:
> >         mmc_free_host(mmc);
> >
> > @@ -1378,10 +1440,14 @@ static int msdc_drv_remove(struct
> platform_device *pdev)
> >         mmc = platform_get_drvdata(pdev);
> >         host = mmc_priv(mmc);
> >
> > +       pm_runtime_get_sync(host->dev);
> > +
> >         platform_set_drvdata(pdev, NULL);
> >         mmc_remove_host(host->mmc);
> >         msdc_deinit_hw(host);
> >
> > +       pm_runtime_put_sync(host->dev);
> 
> Change to pm_runtime_put_noidle() and reverse the order of the call to
> pm_runtime_disable().
> 
Ok. Will fix it at next revision,
> > +       pm_runtime_disable(host->dev);
> >         dma_free_coherent(&pdev->dev,
> >                         MAX_GPD_NUM * sizeof(struct
> mt_gpdma_desc),
> >                         host->dma.gpd, host->dma.gpd_addr); @@
> -1393,6
> > +1459,31 @@ static int msdc_drv_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > +#ifdef CONFIG_PM
> > +static int msdc_runtime_suspend(struct device *dev) {
> > +       int ret = 0;
> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
> > +       struct msdc_host *host = mmc_priv(mmc);
> > +
> > +       ret = msdc_gate_clock(host);
> > +       return ret;
> > +}
> > +
> > +static int msdc_runtime_resume(struct device *dev) {
> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
> > +       struct msdc_host *host = mmc_priv(mmc);
> > +
> > +       msdc_ungate_clock(host);
> > +       return 0;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops msdc_dev_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(msdc_runtime_suspend,
> msdc_runtime_resume,
> > +NULL) };
> > +
> >  static const struct of_device_id msdc_of_ids[] = {
> >         {   .compatible = "mediatek,mt8135-mmc", },
> >         {}
> > @@ -1404,6 +1495,7 @@ static struct platform_driver mt_msdc_driver = {
> >         .driver = {
> >                 .name = "mtk-msdc",
> >                 .of_match_table = msdc_of_ids,
> > +               .pm = &msdc_dev_pm_ops,
> >         },
> >  };
> >
> > --
> > 1.8.1.1.dirty
> >
> 
> Kind regards
> Uffe

************* Email Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 3/5] mmc: mediatek: Add PM support for MMC driver

[Sascha Hauer]

On Tue, Mar 17, 2015 at 11:13:53AM +0800, Chaotian Jing wrote:
> Add PM support for Mediatek MMC driver
> 
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 1 deletion(-)
> 
> @@ -1003,6 +1042,8 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
>  
>  	if (!IS_ERR(mmc->supply.vqmmc)) {
>  
> +		pm_runtime_get_sync(host->dev);
> +
>  		if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
>  			min_uv = 3300000;
>  			max_uv = 3300000;
> @@ -1011,6 +1052,9 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
>  			max_uv = 1800000;
>  		} else {
>  			dev_err(host->dev, "Unsupported signal voltage!\n");
> +
> +			pm_runtime_mark_last_busy(host->dev);
> +			pm_runtime_put_autosuspend(host->dev);
>  			return -EINVAL;
>  		}
>  
> @@ -1022,6 +1066,8 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
>  		}
>  	}
>  
> +	pm_runtime_mark_last_busy(host->dev);
> +	pm_runtime_put_autosuspend(host->dev);

This is unbalanced. You do a pm_runtime_get_sync() depending on
!IS_ERR(mmc->supply.vqmmc), but you call pm_runtime_put_autosuspend()
unconditionally. Besides, I don't see that the pm_ops are necessary here
at all. This function does nothing on the controller.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 3/5] mmc: mediatek: Add PM support for MMC driver

[Sascha Hauer]

On Tue, Mar 17, 2015 at 11:13:53AM +0800, Chaotian Jing wrote:
> Add PM support for Mediatek MMC driver
> 
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 86c999b..e02f6a6 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -1186,6 +1232,8 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  	int ret;
>  	u32 ddr = 0;
>  
> +	pm_runtime_get_sync(host->dev);
> +
>  	if (ios->timing == MMC_TIMING_UHS_DDR50)
>  		ddr = 1;
>  
> @@ -1230,6 +1278,9 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  
>  	if (host->mclk != ios->clock || host->ddr != ddr)
>  		msdc_set_mclk(host, ddr, ios->clock);
> +
> +	pm_runtime_mark_last_busy(host->dev);
> +	pm_runtime_put_autosuspend(host->dev);

This may also be imbalanced. This function can return between
pm_runtime_get_sync() and pm_runtime_put_autosuspend().

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver

[Ulf Hansson]

On 17 March 2015 at 04:13, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> Add Mediatek MMC driver code

Would be nice to get simple overview of what features the
driver/controller supports. Please add that to the commit message.

>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/host/Kconfig  |    8 +
>  drivers/mmc/host/Makefile |    1 +
>  drivers/mmc/host/mtk-sd.c | 1412 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1421 insertions(+)
>  create mode 100644 drivers/mmc/host/mtk-sd.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 61ac63a..8b64377 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -768,3 +768,11 @@ config MMC_TOSHIBA_PCI
>         tristate "Toshiba Type A SD/MMC Card Interface Driver"
>         depends on PCI
>         help
> +
> +config MMC_MTK
> +       tristate "MediaTek SD/MMC Card Interface support"
> +       help
> +         This selects the MediaTek(R) Secure digital and Multimedia card Interface.
> +         If you have a machine with a integrated SD/MMC card reader, say Y or M here.
> +         This is needed if support for any SD/SDIO/MMC devices is required.
> +         If unsure, say N.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 6a7cfe0..161b3e4 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_MMC_SDHCI_F_SDH30)       += sdhci_f_sdh30.o
>  obj-$(CONFIG_MMC_SDHCI_SPEAR)  += sdhci-spear.o
>  obj-$(CONFIG_MMC_WBSD)         += wbsd.o
>  obj-$(CONFIG_MMC_AU1X)         += au1xmmc.o
> +obj-$(CONFIG_MMC_MTK)           += mtk-sd.o
>  obj-$(CONFIG_MMC_OMAP)         += omap.o
>  obj-$(CONFIG_MMC_OMAP_HS)      += omap_hsmmc.o
>  obj-$(CONFIG_MMC_ATMELMCI)     += atmel-mci.o
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> new file mode 100644
> index 0000000..86c999b
> --- /dev/null
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -0,0 +1,1412 @@
> +/*
> + * Copyright (c) 2014-2015 MediaTek Inc.
> + * Author: Chaotian.Jing <chaotian.jing@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/ioport.h>
> +#include <linux/irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spinlock.h>
> +
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/core.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/sdio.h>
> +
> +#define MAX_GPD_NUM         (1 + 1)    /* one null gpd */
> +#define MAX_BD_NUM          1024
> +#define MAX_BD_PER_GPD      MAX_BD_NUM
> +
> +/*--------------------------------------------------------------------------*/
> +/* Common Definition                                                        */
> +/*--------------------------------------------------------------------------*/
> +#define MSDC_MS                 0
> +#define MSDC_SDMMC              1

Please convert to hex values.

> +
> +#define MSDC_BUS_1BITS          0
> +#define MSDC_BUS_4BITS          1
> +#define MSDC_BUS_8BITS          2

Please convert to hex values.

> +
> +#define MSDC_BURST_8B           3
> +#define MSDC_BURST_16B          4
> +#define MSDC_BURST_32B          5
> +#define MSDC_BURST_64B          6

I don't find the first three being used, should we remove them?

Also, I think converting them to hex values make sense.


> +
> +/*--------------------------------------------------------------------------*/
> +/* Register Offset                                                          */
> +/*--------------------------------------------------------------------------*/
> +#define MSDC_CFG         0x0
> +#define MSDC_IOCON       0x04
> +#define MSDC_PS          0x08
> +#define MSDC_INT         0x0c
> +#define MSDC_INTEN       0x10
> +#define MSDC_FIFOCS      0x14
> +#define SDC_CFG          0x30
> +#define SDC_CMD          0x34
> +#define SDC_ARG          0x38
> +#define SDC_STS          0x3c
> +#define SDC_RESP0        0x40
> +#define SDC_RESP1        0x44
> +#define SDC_RESP2        0x48
> +#define SDC_RESP3        0x4c
> +#define SDC_BLK_NUM      0x50
> +#define SDC_ACMD_RESP    0x80
> +#define MSDC_DMA_SA      0x90
> +#define MSDC_DMA_CTRL    0x98
> +#define MSDC_DMA_CFG     0x9c
> +#define MSDC_PATCH_BIT   0xb0
> +#define MSDC_PATCH_BIT1  0xb4
> +#define MSDC_PAD_TUNE    0xec
> +
> +/*--------------------------------------------------------------------------*/
> +/* Register Mask                                                            */
> +/*--------------------------------------------------------------------------*/
> +
> +/* MSDC_CFG mask */
> +#define MSDC_CFG_MODE           (1 << 0)       /* RW */
> +#define MSDC_CFG_CKPDN          (1 << 1)       /* RW */
> +#define MSDC_CFG_RST            (1 << 2)       /* RW */
> +#define MSDC_CFG_PIO            (1 << 3)       /* RW */
> +#define MSDC_CFG_CKDRVEN        (1 << 4)       /* RW */
> +#define MSDC_CFG_BV18SDT        (1 << 5)       /* RW */
> +#define MSDC_CFG_BV18PSS        (1 << 6)       /* R  */
> +#define MSDC_CFG_CKSTB          (1 << 7)       /* R  */
> +#define MSDC_CFG_CKDIV          (0xff << 8)    /* RW */
> +#define MSDC_CFG_CKMOD          (0x3 << 16)    /* RW */
> +
> +/* MSDC_IOCON mask */
> +#define MSDC_IOCON_SDR104CKS    (1 << 0)       /* RW */
> +#define MSDC_IOCON_RSPL         (1 << 1)       /* RW */
> +#define MSDC_IOCON_DSPL         (1 << 2)       /* RW */
> +#define MSDC_IOCON_DDLSEL       (1 << 3)       /* RW */
> +#define MSDC_IOCON_DDR50CKD     (1 << 4)       /* RW */
> +#define MSDC_IOCON_DSPLSEL      (1 << 5)       /* RW */
> +#define MSDC_IOCON_W_DSPL       (1 << 8)       /* RW */
> +#define MSDC_IOCON_D0SPL        (1 << 16)      /* RW */
> +#define MSDC_IOCON_D1SPL        (1 << 17)      /* RW */
> +#define MSDC_IOCON_D2SPL        (1 << 18)      /* RW */
> +#define MSDC_IOCON_D3SPL        (1 << 19)      /* RW */
> +#define MSDC_IOCON_D4SPL        (1 << 20)      /* RW */
> +#define MSDC_IOCON_D5SPL        (1 << 21)      /* RW */
> +#define MSDC_IOCON_D6SPL        (1 << 22)      /* RW */
> +#define MSDC_IOCON_D7SPL        (1 << 23)      /* RW */
> +#define MSDC_IOCON_RISCSZ       (0x3 << 24)    /* RW */
> +
> +/* MSDC_PS mask */
> +#define MSDC_PS_CDEN            (1 << 0)       /* RW */
> +#define MSDC_PS_CDSTS           (1 << 1)       /* R  */
> +#define MSDC_PS_CDDEBOUNCE      (0xf << 12)    /* RW */
> +#define MSDC_PS_DAT             (0xff << 16)   /* R  */
> +#define MSDC_PS_CMD             (1 << 24)      /* R  */
> +#define MSDC_PS_WP              (1 << 31)      /* R  */
> +
> +/* MSDC_INT mask */
> +#define MSDC_INT_MMCIRQ         (1 << 0)       /* W1C */
> +#define MSDC_INT_CDSC           (1 << 1)       /* W1C */
> +#define MSDC_INT_ACMDRDY        (1 << 3)       /* W1C */
> +#define MSDC_INT_ACMDTMO        (1 << 4)       /* W1C */
> +#define MSDC_INT_ACMDCRCERR     (1 << 5)       /* W1C */
> +#define MSDC_INT_DMAQ_EMPTY     (1 << 6)       /* W1C */
> +#define MSDC_INT_SDIOIRQ        (1 << 7)       /* W1C */
> +#define MSDC_INT_CMDRDY         (1 << 8)       /* W1C */
> +#define MSDC_INT_CMDTMO         (1 << 9)       /* W1C */
> +#define MSDC_INT_RSPCRCERR      (1 << 10)      /* W1C */
> +#define MSDC_INT_CSTA           (1 << 11)      /* R */
> +#define MSDC_INT_XFER_COMPL     (1 << 12)      /* W1C */
> +#define MSDC_INT_DXFER_DONE     (1 << 13)      /* W1C */
> +#define MSDC_INT_DATTMO         (1 << 14)      /* W1C */
> +#define MSDC_INT_DATCRCERR      (1 << 15)      /* W1C */
> +#define MSDC_INT_ACMD19_DONE    (1 << 16)      /* W1C */
> +#define MSDC_INT_DMA_BDCSERR    (1 << 17)      /* W1C */
> +#define MSDC_INT_DMA_GPDCSERR   (1 << 18)      /* W1C */
> +#define MSDC_INT_DMA_PROTECT    (1 << 19)      /* W1C */
> +
> +/* MSDC_INTEN mask */
> +#define MSDC_INTEN_MMCIRQ       (1 << 0)       /* RW */
> +#define MSDC_INTEN_CDSC         (1 << 1)       /* RW */
> +#define MSDC_INTEN_ACMDRDY      (1 << 3)       /* RW */
> +#define MSDC_INTEN_ACMDTMO      (1 << 4)       /* RW */
> +#define MSDC_INTEN_ACMDCRCERR   (1 << 5)       /* RW */
> +#define MSDC_INTEN_DMAQ_EMPTY   (1 << 6)       /* RW */
> +#define MSDC_INTEN_SDIOIRQ      (1 << 7)       /* RW */
> +#define MSDC_INTEN_CMDRDY       (1 << 8)       /* RW */
> +#define MSDC_INTEN_CMDTMO       (1 << 9)       /* RW */
> +#define MSDC_INTEN_RSPCRCERR    (1 << 10)      /* RW */
> +#define MSDC_INTEN_CSTA         (1 << 11)      /* RW */
> +#define MSDC_INTEN_XFER_COMPL   (1 << 12)      /* RW */
> +#define MSDC_INTEN_DXFER_DONE   (1 << 13)      /* RW */
> +#define MSDC_INTEN_DATTMO       (1 << 14)      /* RW */
> +#define MSDC_INTEN_DATCRCERR    (1 << 15)      /* RW */
> +#define MSDC_INTEN_ACMD19_DONE  (1 << 16)      /* RW */
> +#define MSDC_INTEN_DMA_BDCSERR  (1 << 17)      /* RW */
> +#define MSDC_INTEN_DMA_GPDCSERR (1 << 18)      /* RW */
> +#define MSDC_INTEN_DMA_PROTECT  (1 << 19)      /* RW */
> +
> +/* MSDC_FIFOCS mask */
> +#define MSDC_FIFOCS_RXCNT       (0xff << 0)    /* R */
> +#define MSDC_FIFOCS_TXCNT       (0xff << 16)   /* R */
> +#define MSDC_FIFOCS_CLR         (1 << 31)      /* RW */
> +
> +/* SDC_CFG mask */
> +#define SDC_CFG_SDIOINTWKUP     (1 << 0)       /* RW */
> +#define SDC_CFG_INSWKUP         (1 << 1)       /* RW */
> +#define SDC_CFG_BUSWIDTH        (0x3 << 16)    /* RW */
> +#define SDC_CFG_SDIO            (1 << 19)      /* RW */
> +#define SDC_CFG_SDIOIDE         (1 << 20)      /* RW */
> +#define SDC_CFG_INTATGAP        (1 << 21)      /* RW */
> +#define SDC_CFG_DTOC            (0xff << 24)   /* RW */
> +
> +/* SDC_STS mask */
> +#define SDC_STS_SDCBUSY         (1 << 0)       /* RW */
> +#define SDC_STS_CMDBUSY         (1 << 1)       /* RW */
> +#define SDC_STS_SWR_COMPL       (1 << 31)      /* RW */
> +
> +/* MSDC_DMA_CTRL mask */
> +#define MSDC_DMA_CTRL_START     (1 << 0)       /* W */
> +#define MSDC_DMA_CTRL_STOP      (1 << 1)       /* W */
> +#define MSDC_DMA_CTRL_RESUME    (1 << 2)       /* W */
> +#define MSDC_DMA_CTRL_MODE      (1 << 8)       /* RW */
> +#define MSDC_DMA_CTRL_LASTBUF   (1 << 10)      /* RW */
> +#define MSDC_DMA_CTRL_BRUSTSZ   (0x7 << 12)    /* RW */
> +
> +/* MSDC_DMA_CFG mask */
> +#define MSDC_DMA_CFG_STS        (1 << 0)       /* R */
> +#define MSDC_DMA_CFG_DECSEN     (1 << 1)       /* RW */
> +#define MSDC_DMA_CFG_AHBHPROT2  (0x2 << 8)     /* RW */
> +#define MSDC_DMA_CFG_ACTIVEEN   (0x2 << 12)    /* RW */
> +#define MSDC_DMA_CFG_CS12B16B   (1 << 16)      /* RW */
> +
> +/* MSDC_PATCH_BIT mask */
> +#define MSDC_PATCH_BIT_ODDSUPP    (1 <<  1)    /* RW */
> +#define MSDC_INT_DAT_LATCH_CK_SEL (0x7 <<  7)
> +#define MSDC_CKGEN_MSDC_DLY_SEL   (0x1f << 10)
> +#define MSDC_PATCH_BIT_IODSSEL    (1 << 16)    /* RW */
> +#define MSDC_PATCH_BIT_IOINTSEL   (1 << 17)    /* RW */
> +#define MSDC_PATCH_BIT_BUSYDLY    (0xf << 18)  /* RW */
> +#define MSDC_PATCH_BIT_WDOD       (0xf << 22)  /* RW */
> +#define MSDC_PATCH_BIT_IDRTSEL    (1 << 26)    /* RW */
> +#define MSDC_PATCH_BIT_CMDFSEL    (1 << 27)    /* RW */
> +#define MSDC_PATCH_BIT_INTDLSEL   (1 << 28)    /* RW */
> +#define MSDC_PATCH_BIT_SPCPUSH    (1 << 29)    /* RW */
> +#define MSDC_PATCH_BIT_DECRCTMO   (1 << 30)    /* RW */
> +
> +#define REQ_CMD_EIO  (1 << 0)
> +#define REQ_CMD_TMO  (1 << 1)
> +#define REQ_DAT_ERR  (1 << 2)
> +#define REQ_STOP_EIO (1 << 3)
> +#define REQ_STOP_TMO (1 << 4)
> +#define REQ_CMD_BUSY (1 << 5)
> +
> +#define MSDC_PREPARE_FLAG (1 << 0)
> +#define MSDC_ASYNC_FLAG (1 << 1)
> +#define MSDC_MMAP_FLAG (1 << 2)
> +
> +#define HOST_MAX_BLKSZ      2048
> +#define MSDC_OCR_AVAIL      (MMC_VDD_28_29 | MMC_VDD_29_30 \
> +               | MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33)

Both these aren't used. Remove please.

> +
> +#define CMD_TIMEOUT         (HZ/10 * 5)        /* 100ms x5 */
> +#define DAT_TIMEOUT         (HZ    * 5)        /* 1000ms x5 */
> +
> +/*--------------------------------------------------------------------------*/
> +/* Descriptor Structure                                                     */
> +/*--------------------------------------------------------------------------*/
> +struct mt_gpdma_desc {
> +       u32 first_u32;
> +#define GPDMA_DESC_HWO         (1 << 0)
> +#define GPDMA_DESC_BDP         (1 << 1)
> +#define GPDMA_DESC_CHECKSUM    (0xff << 8) /* bit8 ~ bit15 */
> +#define GPDMA_DESC_INT         (1 << 16)
> +       u32 next;
> +       u32 ptr;
> +       u32 second_u32;
> +#define GPDMA_DESC_BUFLEN      (0xffff) /* bit0 ~ bit15 */
> +#define GPDMA_DESC_EXTLEN      (0xff << 16) /* bit16 ~ bit23 */
> +       u32 arg;
> +       u32 blknum;
> +       u32 cmd;
> +};
> +
> +struct mt_bdma_desc {
> +       u32 first_u32;
> +#define BDMA_DESC_EOL          (1 << 0)
> +#define BDMA_DESC_CHECKSUM     (0xff << 8) /* bit8 ~ bit15 */
> +#define BDMA_DESC_BLKPAD       (1 << 17)
> +#define BDMA_DESC_DWPAD                (1 << 18)
> +       u32 next;
> +       u32 ptr;
> +       u32 second_u32;
> +#define BDMA_DESC_BUFLEN       (0xffff) /* bit0 ~ bit15 */
> +};
> +
> +struct msdc_dma {
> +       struct scatterlist *sg; /* I/O scatter list */
> +       struct mt_gpdma_desc *gpd;              /* pointer to gpd array */
> +       struct mt_bdma_desc *bd;                /* pointer to bd array */
> +       dma_addr_t gpd_addr;    /* the physical address of gpd array */
> +       dma_addr_t bd_addr;     /* the physical address of bd array */
> +};

This looks weird from DMA perspective. Can you elaborate on why you
can't use the dmaengine API?

> +
> +struct msdc_host {
> +       struct device *dev;
> +       struct mmc_host *mmc;   /* mmc structure */
> +       int cmd_rsp;
> +
> +       spinlock_t lock;
> +       struct mmc_request *mrq;
> +       struct mmc_command *cmd;
> +       struct mmc_data *data;
> +       int error;
> +
> +       void __iomem *base;             /* host base address */
> +
> +       struct msdc_dma dma;    /* dma channel */
> +
> +       u32 timeout_ns;         /* data timeout ns */
> +       u32 timeout_clks;       /* data timeout clks */
> +
> +       struct pinctrl *pinctrl;
> +       struct pinctrl_state *pins_default;
> +       struct pinctrl_state *pins_uhs;
> +       struct delayed_work req_timeout;
> +       int irq;                /* host interrupt */
> +
> +       struct clk *src_clk;    /* msdc source clock */
> +       u32 mclk;               /* mmc subsystem clock */
> +       u32 hclk;               /* host clock speed */
> +       u32 sclk;               /* SD/MS clock speed */
> +       bool ddr;
> +};
> +
> +static void sdr_set_bits(void __iomem *reg, u32 bs)
> +{
> +       u32 val = readl(reg);
> +
> +       val |= bs;
> +       writel(val, reg);
> +}
> +
> +static void sdr_clr_bits(void __iomem *reg, u32 bs)
> +{
> +       u32 val = readl(reg);
> +
> +       val &= ~(u32)bs;
> +       writel(val, reg);
> +}
> +
> +static void sdr_set_field(void __iomem *reg, u32 field, u32 val)
> +{
> +       unsigned int tv = readl(reg);
> +
> +       tv &= ~field;
> +       tv |= ((val) << (ffs((unsigned int)field) - 1));
> +       writel(tv, reg);
> +}

A common thought for all the three above functions:

Have you considered using a cache variable for those registers that
often gets updated? In that way you would have to read the register
value every time when you want to write to it. It should improve
performance a bit.

> +
> +static void sdr_get_field(void __iomem *reg, u32 field, u32 *val)
> +{
> +       unsigned int tv = readl(reg);
> +
> +       *val = ((tv & field) >> (ffs((unsigned int)field) - 1));
> +}
> +
> +static void msdc_reset_hw(struct msdc_host *host)
> +{
> +       u32 val;
> +
> +       sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_RST);
> +       while (readl(host->base + MSDC_CFG) & MSDC_CFG_RST)
> +               cpu_relax();
> +
> +       sdr_set_bits(host->base + MSDC_FIFOCS, MSDC_FIFOCS_CLR);
> +       while (readl(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_CLR)
> +               cpu_relax();
> +
> +       val = readl(host->base + MSDC_INT);
> +       writel(val, host->base + MSDC_INT);
> +}
> +
> +static void msdc_cmd_next(struct msdc_host *host,
> +               struct mmc_request *mrq, struct mmc_command *cmd);

Please investigate whether you can move around the code to prevent
this static declaration of the function.

> +
> +static inline u32 msdc_data_ints_mask(struct msdc_host *host)
> +{
> +       u32 wints =
> +               /* data xfer */
> +               MSDC_INTEN_XFER_COMPL |
> +               MSDC_INTEN_DATTMO |
> +               MSDC_INTEN_DATCRCERR |
> +               /* DMA events */
> +               MSDC_INTEN_DMA_BDCSERR |
> +               MSDC_INTEN_DMA_GPDCSERR |
> +               MSDC_INTEN_DMA_PROTECT;
> +
> +       return wints;
> +}

This should be a static variable instead of a function.

> +
> +static u8 msdc_dma_calcs(u8 *buf, u32 len)
> +{
> +       u32 i, sum = 0;
> +
> +       for (i = 0; i < len; i++)
> +               sum += buf[i];
> +       return 0xff - (u8) sum;
> +}
> +
> +static inline void msdc_dma_setup(struct msdc_host *host, struct msdc_dma *dma,
> +               struct mmc_data *data)
> +{
> +       u32 sglen, j;
> +       u32 dma_address, dma_len;
> +       struct scatterlist *sg;
> +       struct mt_gpdma_desc *gpd;
> +       struct mt_bdma_desc *bd;
> +
> +       sglen = data->sg_len;
> +       sg = data->sg;
> +
> +       gpd = dma->gpd;
> +       bd = dma->bd;
> +
> +       /* modify gpd */
> +       gpd->first_u32 |= GPDMA_DESC_HWO;
> +       gpd->first_u32 |= GPDMA_DESC_BDP;
> +       /* need to clear first. use these bits to calc checksum */
> +       gpd->first_u32 &= ~GPDMA_DESC_CHECKSUM;
> +       gpd->first_u32 |= msdc_dma_calcs((u8 *) gpd, 16) << 8;
> +
> +       /* modify bd */
> +       for (j = 0; j < sglen; j++) {
> +               dma_address = sg_dma_address(sg);
> +               dma_len = sg_dma_len(sg);
> +
> +               /* init bd */
> +               bd[j].first_u32 &= ~BDMA_DESC_BLKPAD;
> +               bd[j].first_u32 &= ~BDMA_DESC_DWPAD;
> +               bd[j].ptr = (u32)dma_address;
> +               bd[j].second_u32 &= ~BDMA_DESC_BUFLEN;
> +               bd[j].second_u32 |= (dma_len & BDMA_DESC_BUFLEN);
> +
> +               if (j == sglen - 1) /* the last bd */
> +                       bd[j].first_u32 |= BDMA_DESC_EOL;
> +               else
> +                       bd[j].first_u32 &= ~BDMA_DESC_EOL;
> +
> +               /* checksume need to clear first */
> +               bd[j].first_u32 &= ~BDMA_DESC_CHECKSUM;
> +               bd[j].first_u32 |= msdc_dma_calcs((u8 *)(&bd[j]), 16) << 8;
> +               sg++;
> +       }
> +
> +       sdr_set_field(host->base + MSDC_DMA_CFG, MSDC_DMA_CFG_DECSEN, 1);
> +       sdr_set_field(host->base + MSDC_DMA_CTRL, MSDC_DMA_CTRL_BRUSTSZ,
> +                       MSDC_BURST_64B);
> +       sdr_set_field(host->base + MSDC_DMA_CTRL, MSDC_DMA_CTRL_MODE, 1);
> +
> +       writel((u32)dma->gpd_addr, host->base + MSDC_DMA_SA);
> +}
> +
> +static void msdc_prepare_data(struct msdc_host *host, struct mmc_request *mrq)
> +{
> +       struct mmc_data *data = mrq->data;
> +
> +       if (!(data->host_cookie & MSDC_PREPARE_FLAG)) {
> +               bool read = (data->flags & MMC_DATA_READ) != 0;
> +
> +               data->host_cookie |= MSDC_PREPARE_FLAG;
> +               dma_map_sg(host->dev, data->sg, data->sg_len,
> +                          read ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
> +       }
> +}
> +
> +static void msdc_unprepare_data(struct msdc_host *host, struct mmc_request *mrq)
> +{
> +       struct mmc_data *data = mrq->data;
> +
> +       if (data->host_cookie & MSDC_ASYNC_FLAG)
> +               return;
> +
> +       if ((data->host_cookie & MSDC_PREPARE_FLAG)) {
> +               bool read = (data->flags & MMC_DATA_READ) != 0;
> +
> +               dma_unmap_sg(host->dev, data->sg, data->sg_len,
> +                            read ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
> +               data->host_cookie &= ~MSDC_PREPARE_FLAG;
> +       }
> +}
> +
> +/* clock control primitives */
> +static void msdc_set_timeout(struct msdc_host *host, u32 ns, u32 clks)
> +{
> +       u32 timeout, clk_ns;
> +       u32 mode = 0;
> +
> +       host->timeout_ns = ns;
> +       host->timeout_clks = clks;
> +       if (host->sclk == 0) {
> +               timeout = 0;
> +       } else {
> +               clk_ns  = 1000000000UL / host->sclk;
> +               timeout = (ns + clk_ns - 1) / clk_ns + clks;
> +               /* in 1048576 sclk cycle unit */
> +               timeout = (timeout + (1 << 20) - 1) >> 20;
> +               sdr_get_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD, &mode);
> +               /*DDR mode will double the clk cycles for data timeout */

How do you know you will be using DDR at this point? Don't you need to
check for that?

> +               timeout = mode >= 2 ? timeout * 2 : timeout;
> +               timeout = timeout > 1 ? timeout - 1 : 0;
> +               timeout = timeout > 255 ? 255 : timeout;
> +       }
> +       sdr_set_field(host->base + SDC_CFG, SDC_CFG_DTOC, timeout);
> +}
> +
> +static void msdc_set_mclk(struct msdc_host *host, int ddr, u32 hz)
> +{
> +       u32 mode;
> +       u32 flags;
> +       u32 div;
> +       u32 sclk;
> +       u32 hclk = host->hclk;
> +
> +       if (!hz) {
> +               dev_err(host->dev, "set mclk to 0\n");
> +               host->mclk = 0;

This isn't supposed to be treated as an error. The mmc core expect the
host to gate the clock when the rate is zero.

This is also needed when dealing with I/O voltage switch for UHS cards.

> +               msdc_reset_hw(host);
> +               return;
> +       }
> +
> +       flags = readl(host->base + MSDC_INTEN);
> +       sdr_clr_bits(host->base + MSDC_INTEN, flags);
> +
> +       if (ddr) { /* may need to modify later */
> +               mode = 0x2; /* ddr mode and use divisor */
> +               if (hz >= (hclk >> 2)) {
> +                       div = 0; /* mean div = 1/4 */
> +                       sclk = hclk >> 2; /* sclk = clk / 4 */
> +               } else {
> +                       div = (hclk + ((hz << 2) - 1)) / (hz << 2);
> +                       sclk = (hclk >> 2) / div;
> +                       div = (div >> 1);
> +               }
> +       } else if (hz >= hclk) {
> +               mode = 0x1; /* no divisor */
> +               div = 0;
> +               sclk = hclk;
> +       } else {
> +               mode = 0x0; /* use divisor */
> +               if (hz >= (hclk >> 1)) {
> +                       div = 0; /* mean div = 1/2 */
> +                       sclk = hclk >> 1; /* sclk = clk / 2 */
> +               } else {
> +                       div = (hclk + ((hz << 2) - 1)) / (hz << 2);
> +                       sclk = (hclk >> 2) / div;
> +               }
> +       }
> +
> +       sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD | MSDC_CFG_CKDIV,
> +                       (mode << 8) | (div % 0xff));
> +       while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
> +               cpu_relax();
> +
> +       host->sclk = sclk;
> +       host->mclk = hz;
> +       host->ddr = ddr;
> +       /* need because clk changed. */
> +       msdc_set_timeout(host, host->timeout_ns, host->timeout_clks);
> +       sdr_set_bits(host->base + MSDC_INTEN, flags);
> +
> +       dev_dbg(host->dev, "sclk: %d, ddr: %d\n", host->sclk, ddr);
> +}
> +
> +static inline u32 msdc_cmd_find_resp(struct msdc_host *host,
> +               struct mmc_request *mrq, struct mmc_command *cmd)
> +{
> +       u32 resp;
> +
> +       switch (mmc_resp_type(cmd)) {
> +               /* Actually, R1, R5, R6, R7 are the same */
> +       case MMC_RSP_R1:
> +               resp = 1;
> +               break;
> +       case MMC_RSP_R1B:
> +               resp = 7;
> +               break;
> +       case MMC_RSP_R2:
> +               resp = 2;
> +               break;
> +       case MMC_RSP_R3:
> +               resp = 3;
> +               break;
> +       case MMC_RSP_NONE:
> +       default:
> +               resp = 0;
> +               break;
> +       }
> +
> +       return resp;
> +}

I would rather see that the above function return hex values instead
of decimals.

> +
> +static inline u32 msdc_cmd_prepare_raw_cmd(struct msdc_host *host,
> +               struct mmc_request *mrq, struct mmc_command *cmd)
> +{
> +       /* rawcmd :
> +        * vol_swt << 30 | auto_cmd << 28 | blklen << 16 | go_irq << 15 |
> +        * stop << 14 | rw << 13 | dtype << 11 | rsptyp << 7 | brk << 6 | opcode
> +        */
> +       u32 opcode = cmd->opcode;
> +       u32 resp = msdc_cmd_find_resp(host, mrq, cmd);
> +       u32 rawcmd = (opcode & 0x3f) | ((resp & 7) << 7);

Would be nice to maintain consistency and thus stick to use hex values
for bitwise OR/AND.

> +
> +       host->cmd_rsp = resp;
> +
> +       if ((opcode == SD_IO_RW_DIRECT && cmd->flags == (unsigned int) -1) ||
> +                       opcode == MMC_STOP_TRANSMISSION)
> +               rawcmd |= (1 << 14);
> +       else if (opcode == SD_SWITCH_VOLTAGE)
> +               rawcmd |= (1 << 30);
> +       else if ((opcode == SD_APP_SEND_SCR) ||
> +                       (opcode == SD_APP_SEND_NUM_WR_BLKS) ||
> +                       (opcode == SD_SWITCH &&
> +                       (mmc_cmd_type(cmd) == MMC_CMD_ADTC)) ||
> +                       (opcode == SD_APP_SD_STATUS &&
> +                       (mmc_cmd_type(cmd) == MMC_CMD_ADTC)) ||
> +                       (opcode == MMC_SEND_EXT_CSD &&
> +                       (mmc_cmd_type(cmd) == MMC_CMD_ADTC)))
> +               rawcmd |= (1 << 11);
> +
> +       if (cmd->data) {
> +               struct mmc_data *data = cmd->data;
> +
> +               if (mmc_op_multi(opcode)) {
> +                       if (mmc_card_mmc(host->mmc->card) && mrq->sbc &&
> +                                       !(mrq->sbc->arg & 0xFFFF0000))
> +                               rawcmd |= 2 << 28; /* AutoCMD23 */

Hex values and lower-case letters in the hex values, to keep consistency...

I will stop commenting on this from now on, since I realize it applies
to the complete code. Please go through the code and fix this.

> +               }
> +
> +               rawcmd |= ((data->blksz & 0xFFF) << 16);
> +               if (data->flags & MMC_DATA_WRITE)
> +                       rawcmd |= (1 << 13);
> +               if (data->blocks > 1)
> +                       rawcmd |= (2 << 11);
> +               else
> +                       rawcmd |= (1 << 11);
> +               /* Always use dma mode */
> +               sdr_clr_bits(host->base + MSDC_CFG, MSDC_CFG_PIO);
> +
> +               if (((host->timeout_ns != data->timeout_ns) ||
> +                               (host->timeout_clks != data->timeout_clks))) {
> +                       msdc_set_timeout(host, data->timeout_ns,
> +                                       data->timeout_clks);
> +               }
> +
> +               writel(data->blocks, host->base + SDC_BLK_NUM);
> +       }
> +       return rawcmd;
> +}
> +
> +static void msdc_start_data(struct msdc_host *host, struct mmc_request *mrq,
> +                           struct mmc_command *cmd, struct mmc_data *data)
> +{
> +       bool read;
> +       unsigned long flags;
> +       u32 wints;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       if (!host->data) {
> +               host->data = data;
> +       } else {
> +               dev_err(host->dev, "%s: CMD%d [%d blocks] is active",
> +                               __func__, host->mrq->cmd->opcode,
> +                               host->data->blocks);
> +               dev_err(host->dev, "but new CMD%d [%d blocks] started\n",
> +                       cmd->opcode, data->blocks);
> +       }
> +       spin_unlock_irqrestore(&host->lock, flags);

How about replacing the above error check with a single WARN_ON() instead?

> +       read = (data->flags & MMC_DATA_READ) != 0;

Simplify to:

read = data->flags & MMC_DATA_READ;

> +       data->error = 0;

You shouldn't need to reset that, handle by the mmc core.

> +       if (data->stop)
> +               data->stop->error = 0;

You shouldn't need to reset that, handle by the mmc core.

> +
> +       mod_delayed_work(system_wq, &host->req_timeout, DAT_TIMEOUT);
> +       msdc_dma_setup(host, &host->dma, data);
> +       wints = msdc_data_ints_mask(host);
> +       sdr_set_bits(host->base + MSDC_INTEN, wints);
> +       mb(); /* wait for pending IO to finish */
> +       sdr_set_field(host->base + MSDC_DMA_CTRL, MSDC_DMA_CTRL_START, 1);
> +       dev_dbg(host->dev, "DMA start\n");
> +       dev_dbg(host->dev, "%s: cmd=%d DMA data: %d blocks; read=%d\n",
> +                       __func__, cmd->opcode, data->blocks, read);
> +}
> +
> +static int msdc_auto_cmd_done(struct msdc_host *host, int events,
> +               struct mmc_command *cmd)
> +{
> +       u32 *rsp = cmd->resp;
> +
> +       rsp[0] = readl(host->base + SDC_ACMD_RESP);
> +
> +       if (events & MSDC_INT_ACMDRDY) {
> +               cmd->error = 0;
> +       } else {
> +               msdc_reset_hw(host);
> +               if (events & MSDC_INT_ACMDCRCERR) {
> +                       cmd->error = (unsigned int) -EILSEQ;
> +                       host->error |= REQ_STOP_EIO;
> +               } else if (events & MSDC_INT_ACMDTMO) {
> +                       cmd->error = (unsigned int) -ETIMEDOUT;
> +                       host->error |= REQ_STOP_TMO;
> +               }
> +               dev_err(host->dev,
> +                       "%s: AUTO_CMD%d arg=%08X; rsp %08X; cmd_error=%d\n",
> +                       __func__, cmd->opcode, cmd->arg, rsp[0],
> +                       (int)cmd->error);
> +       }
> +       return (int)(cmd->error);

This is silly. The struct mmc_command|data->error field should be of
type int and not unsigned int, which isn't the case currently.

Host driver shouldn't need to deal with this casting. I have just sent
a patch to change "error" to an int. Please align to that change, in
all places where you use the "error" field.

> +}
> +
> +static void msdc_track_cmd_data(struct msdc_host *host,
> +                               struct mmc_command *cmd, struct mmc_data *data)
> +{
> +       if (host->error)
> +               dev_dbg(host->dev, "%s: cmd=%d arg=%08X; host->error=0x%08X\n",
> +                       __func__, cmd->opcode, cmd->arg, host->error);
> +}
> +
> +static void msdc_request_done(struct msdc_host *host, struct mmc_request *mrq)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       cancel_delayed_work(&host->req_timeout);

This looks racy.

Don't you need a cancel_delayed_work_sync() from somewhere, to be sure
that work isn't preemted after the "host->mrq" has been reset here? Or
maybe there is another way!? Of course that can't be done with the
spin_locks held, but I asume you get my point.

> +       host->mrq = NULL;
> +       spin_unlock_irqrestore(&host->lock, flags);
> +
> +       msdc_track_cmd_data(host, mrq->cmd, mrq->data);
> +       if (mrq->data)
> +               msdc_unprepare_data(host, mrq);
> +       mmc_request_done(host->mmc, mrq);
> +}
> +
> +/* returns true if command is fully handled; returns false otherwise */
> +static bool msdc_cmd_done(struct msdc_host *host, int events,
> +                         struct mmc_request *mrq, struct mmc_command *cmd)
> +{
> +       bool done = false;
> +       bool sbc_error;
> +       unsigned long flags;
> +       u32 *rsp = cmd->resp;
> +
> +       if (mrq->sbc && cmd == mrq->cmd &&
> +                       (events & (MSDC_INT_ACMDRDY | MSDC_INT_ACMDCRCERR
> +                                  | MSDC_INT_ACMDTMO)))
> +               msdc_auto_cmd_done(host, events, mrq->sbc);
> +
> +       sbc_error = mrq->sbc && mrq->sbc->error;
> +
> +       if (!sbc_error && !(events & (MSDC_INT_CMDRDY
> +                                       | MSDC_INT_RSPCRCERR
> +                                       | MSDC_INT_CMDTMO)))
> +               return done;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       done = !host->cmd;
> +       host->cmd = NULL;
> +       spin_unlock_irqrestore(&host->lock, flags);
> +
> +       if (done)
> +               return true;
> +
> +       sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_CMDRDY |
> +                       MSDC_INTEN_RSPCRCERR | MSDC_INTEN_CMDTMO |
> +                       MSDC_INTEN_ACMDRDY | MSDC_INTEN_ACMDCRCERR |
> +                       MSDC_INTEN_ACMDTMO);
> +       writel(cmd->arg, host->base + SDC_ARG);
> +
> +       switch (host->cmd_rsp) {
> +       case 0:
> +               break;
> +       case 2:
> +               rsp[0] = readl(host->base + SDC_RESP3);
> +               rsp[1] = readl(host->base + SDC_RESP2);
> +               rsp[2] = readl(host->base + SDC_RESP1);
> +               rsp[3] = readl(host->base + SDC_RESP0);
> +               break;
> +       default: /* Response types 1, 3, 4, 5, 6, 7(1b) */
> +               rsp[0] = readl(host->base + SDC_RESP0);
> +               break;
> +       }
> +       if (!sbc_error && !(events & MSDC_INT_CMDRDY)) {
> +               msdc_reset_hw(host);
> +               if (events & MSDC_INT_RSPCRCERR) {
> +                       cmd->error = (unsigned int) -EILSEQ;
> +                       host->error |= REQ_CMD_EIO;
> +               } else if (events & MSDC_INT_CMDTMO) {
> +                       cmd->error = (unsigned int) -ETIMEDOUT;
> +                       host->error |= REQ_CMD_TMO;
> +               }
> +       }
> +       if (cmd->error)
> +               dev_dbg(host->dev,
> +                               "%s: cmd=%d arg=%08X; rsp %08X; cmd_error=%d\n",
> +                               __func__, cmd->opcode, cmd->arg, rsp[0],
> +                               (int)cmd->error);
> +
> +       msdc_cmd_next(host, mrq, cmd);
> +       done = true;
> +       return done;
> +}
> +
> +static inline bool msdc_cmd_is_ready(struct msdc_host *host,
> +               struct mmc_request *mrq, struct mmc_command *cmd)
> +{
> +       unsigned long tmo = jiffies + msecs_to_jiffies(20);
> +
> +       while ((readl(host->base + SDC_STS) & SDC_STS_CMDBUSY)
> +                       && time_before(jiffies, tmo))
> +               continue;
> +
> +       if (readl(host->base + SDC_STS) & SDC_STS_CMDBUSY) {
> +               dev_err(host->dev, "CMD bus busy detected\n");
> +               host->error |= REQ_CMD_BUSY;
> +               msdc_cmd_done(host, MSDC_INT_CMDTMO, mrq, cmd);
> +               return false;
> +       }
> +
> +       if (mmc_resp_type(cmd) == MMC_RSP_R1B || cmd->data) {
> +               /* R1B or with data, should check SDCBUSY */
> +               while (readl(host->base + SDC_STS) & SDC_STS_SDCBUSY)
> +                       cpu_relax();
> +       }
> +       return true;
> +}
> +
> +static void msdc_start_command(struct msdc_host *host,
> +               struct mmc_request *mrq, struct mmc_command *cmd)
> +{
> +       u32 rawcmd;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       if (host->cmd) {
> +               dev_err(host->dev, "%s: CMD%d is active, but new CMD%d is sent\n",
> +                               __func__, host->cmd->opcode, cmd->opcode);
> +       } else {
> +               host->cmd = cmd;
> +       }
> +       spin_unlock_irqrestore(&host->lock, flags);
> +

How about replacing the above error check with a single WARN_ON() instead?

> +       if (!msdc_cmd_is_ready(host, mrq, cmd))
> +               return;
> +
> +       if ((readl(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_TXCNT) >> 16
> +               || (readl(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_RXCNT)) {
> +               dev_err(host->dev, "TX/RX FIFO non-empty before start of IO. Reset\n");
> +               msdc_reset_hw(host);
> +       }
> +
> +       cmd->error = 0;
> +       rawcmd = msdc_cmd_prepare_raw_cmd(host, mrq, cmd);
> +       mod_delayed_work(system_wq, &host->req_timeout, DAT_TIMEOUT);
> +
> +       sdr_set_bits(host->base + MSDC_INTEN, MSDC_INTEN_CMDRDY |
> +                       MSDC_INTEN_RSPCRCERR | MSDC_INTEN_CMDTMO |
> +                       MSDC_INTEN_ACMDRDY | MSDC_INTEN_ACMDCRCERR |
> +                       MSDC_INTEN_ACMDTMO);
> +       writel(cmd->arg, host->base + SDC_ARG);
> +       writel(rawcmd, host->base + SDC_CMD);
> +}
> +
> +static void msdc_cmd_next(struct msdc_host *host,
> +               struct mmc_request *mrq, struct mmc_command *cmd)
> +{
> +       if (cmd->error || (mrq->sbc && mrq->sbc->error))
> +               msdc_request_done(host, mrq);
> +       else if (cmd == mrq->sbc)
> +               msdc_start_command(host, mrq, mrq->cmd);
> +       else if (!cmd->data)
> +               msdc_request_done(host, mrq);
> +       else
> +               msdc_start_data(host, mrq, cmd, cmd->data);
> +}
> +
> +static void msdc_ops_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> +       unsigned long flags;
> +       struct msdc_host *host = mmc_priv(mmc);
> +
> +       host->error = 0;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       if (!host->mrq) {
> +               host->mrq = mrq;
> +       } else {
> +               dev_err(host->dev,
> +                       "%s: req [CMD%d] is active, new req [CMD%d] is sent\n",
> +                       __func__, host->mrq->cmd->opcode, mrq->cmd->opcode);
> +       }
> +       spin_unlock_irqrestore(&host->lock, flags);

How about replacing the above error check with a single WARN_ON() instead?

> +
> +       if (mrq->data)
> +               msdc_prepare_data(host, mrq);
> +
> +       /* if SBC is required, we have HW option and SW option.
> +        * if HW option is enabled, and SBC does not have "special" flags,
> +        * use HW option,  otherwise use SW option
> +        */
> +       if (mrq->sbc && (!mmc_card_mmc(mmc->card) ||
> +           (mrq->sbc->arg & 0xFFFF0000)))
> +               msdc_start_command(host, mrq, mrq->sbc);
> +       else
> +               msdc_start_command(host, mrq, mrq->cmd);
> +}
> +
> +static void msdc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
> +               bool is_first_req)
> +{
> +       struct msdc_host *host = mmc_priv(mmc);
> +       struct mmc_data *data = mrq->data;
> +
> +       if (!data)
> +               return;
> +
> +       msdc_prepare_data(host, mrq);
> +       data->host_cookie |= MSDC_ASYNC_FLAG;
> +}
> +
> +static void msdc_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
> +               int err)
> +{
> +       struct msdc_host *host = mmc_priv(mmc);
> +       struct mmc_data *data;
> +
> +       data = mrq->data;
> +       if (!data)
> +               return;
> +       if (data->host_cookie) {
> +               data->host_cookie &= ~MSDC_ASYNC_FLAG;
> +               msdc_unprepare_data(host, mrq);
> +       }
> +}
> +
> +static void msdc_data_xfer_next(struct msdc_host *host,
> +                               struct mmc_request *mrq, struct mmc_data *data)
> +{
> +       if (mmc_op_multi(mrq->cmd->opcode) && mrq->stop && !mrq->stop->error &&
> +                       (!data->bytes_xfered || !mrq->sbc))
> +               msdc_start_command(host, mrq, mrq->stop);
> +       else
> +               msdc_request_done(host, mrq);
> +}
> +
> +static bool msdc_data_xfer_done(struct msdc_host *host, u32 events,
> +                               struct mmc_request *mrq, struct mmc_data *data)
> +{
> +       struct mmc_command *stop = data->stop;
> +       unsigned long flags;
> +       bool done;
> +       u32 wints;
> +
> +       bool check_data = (events &
> +           (MSDC_INT_XFER_COMPL | MSDC_INT_DATCRCERR | MSDC_INT_DATTMO
> +            | MSDC_INT_DMA_BDCSERR | MSDC_INT_DMA_GPDCSERR
> +            | MSDC_INT_DMA_PROTECT));
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       done = !host->data;
> +       if (check_data)
> +               host->data = NULL;
> +       spin_unlock_irqrestore(&host->lock, flags);
> +
> +       if (done)
> +               return true;
> +
> +       if (check_data || (stop && stop->error)) {
> +               dev_dbg(host->dev, "DMA status: 0x%8X\n",
> +                               readl(host->base + MSDC_DMA_CFG));
> +               sdr_set_field(host->base + MSDC_DMA_CTRL, MSDC_DMA_CTRL_STOP,
> +                               1);
> +               while (readl(host->base + MSDC_DMA_CFG) & MSDC_DMA_CFG_STS)
> +                       ;
> +               mb(); /* wait for pending IO to finish */
> +               wints = msdc_data_ints_mask(host);
> +               sdr_clr_bits(host->base + MSDC_INTEN, wints);
> +               dev_dbg(host->dev, "DMA stop\n");
> +
> +               if ((events & MSDC_INT_XFER_COMPL) && (!stop || !stop->error)) {
> +                       data->bytes_xfered = data->blocks * data->blksz;
> +               } else {
> +                       dev_err(host->dev, "interrupt events: %x\n", events);
> +                       msdc_reset_hw(host);
> +                       host->error |= REQ_DAT_ERR;
> +                       data->bytes_xfered = 0;
> +
> +                       if (events & MSDC_INT_DATTMO)
> +                               data->error = (unsigned int) -ETIMEDOUT;
> +
> +                       dev_err(host->dev, "%s: cmd=%d; blocks=%d",
> +                               __func__, mrq->cmd->opcode, data->blocks);
> +                       dev_err(host->dev, "data_error=%d xfer_size=%d\n",
> +                                       (int)data->error, data->bytes_xfered);
> +               }
> +
> +               msdc_data_xfer_next(host, mrq, data);
> +               done = true;
> +       }
> +       return done;
> +}
> +
> +static void msdc_set_buswidth(struct msdc_host *host, u32 width)
> +{
> +       u32 val = readl(host->base + SDC_CFG);
> +
> +       val &= ~SDC_CFG_BUSWIDTH;
> +
> +       switch (width) {
> +       default:
> +       case MMC_BUS_WIDTH_1:
> +               val |= (MSDC_BUS_1BITS << 16);
> +               break;
> +       case MMC_BUS_WIDTH_4:
> +               val |= (MSDC_BUS_4BITS << 16);
> +               break;
> +       case MMC_BUS_WIDTH_8:
> +               val |= (MSDC_BUS_8BITS << 16);
> +               break;
> +       }
> +
> +       writel(val, host->base + SDC_CFG);
> +       dev_dbg(host->dev, "Bus Width = %d", width);
> +}
> +
> +static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       struct msdc_host *host = mmc_priv(mmc);
> +       int min_uv, max_uv;
> +       int ret = 0;
> +
> +       if (!IS_ERR(mmc->supply.vqmmc)) {
> +
> +               if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> +                       min_uv = 3300000;
> +                       max_uv = 3300000;
> +               } else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> +                       min_uv = 1800000;
> +                       max_uv = 1800000;
> +               } else {
> +                       dev_err(host->dev, "Unsupported signal voltage!\n");
> +                       return -EINVAL;
> +               }
> +
> +               ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
> +               if (ret) {
> +                       dev_err(host->dev,
> +                                       "Regulator set error %d: %d - %d\n",
> +                                       ret, min_uv, max_uv);
> +               }
> +       }
> +
> +       return ret;
> +}
> +
> +static void msdc_request_timeout(struct work_struct *work)
> +{
> +       struct msdc_host *host = container_of(work, struct msdc_host,
> +                       req_timeout.work);
> +       unsigned long flags;
> +       struct mmc_request *mrq;
> +       struct mmc_command *cmd;
> +       struct mmc_data *data;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       mrq = ACCESS_ONCE(host->mrq);
> +       cmd = ACCESS_ONCE(host->cmd);
> +       data = ACCESS_ONCE(host->data);

Why do you need the ACCESS_ONCE here. Aren't these pointers already
protected by the spin_lock?

> +       spin_unlock_irqrestore(&host->lock, flags);
> +
> +       /* simulate HW timeout status */
> +       dev_err(host->dev, "%s: aborting cmd/data/mrq\n", __func__);
> +       if (mrq) {
> +               dev_err(host->dev, "%s: aborting mrq=%p cmd=%d\n", __func__,
> +                       mrq, mrq->cmd->opcode);
> +               if (cmd) {
> +                       dev_err(host->dev, "%s: aborting cmd=%d\n",
> +                                       __func__, cmd->opcode);
> +                       msdc_cmd_done(host, MSDC_INT_CMDTMO, mrq, cmd);
> +               } else if (data) {
> +                       dev_err(host->dev, "%s: abort data: cmd%d; %d blocks\n",
> +                               __func__, mrq->cmd->opcode, data->blocks);
> +                       msdc_data_xfer_done(host, MSDC_INT_DATTMO, mrq, data);
> +               }
> +       }
> +}
> +
> +static irqreturn_t msdc_irq(int irq, void *dev_id)
> +{
> +       struct msdc_host *host = (struct msdc_host *) dev_id;
> +
> +       while (true) {
> +               unsigned long flags;
> +               struct mmc_request *mrq;
> +               struct mmc_command *cmd;
> +               struct mmc_data *data;
> +               u32 events, event_mask;
> +
> +               spin_lock_irqsave(&host->lock, flags);
> +               events = readl(host->base + MSDC_INT);
> +               event_mask = readl(host->base + MSDC_INTEN);
> +               /* clear interrupts */
> +               writel(events, host->base + MSDC_INT);
> +
> +               mrq = ACCESS_ONCE(host->mrq);
> +               cmd = ACCESS_ONCE(host->cmd);
> +               data = ACCESS_ONCE(host->data);

Why do you need the ACCESS_ONCE here. Aren't these pointers already
protected by the spin_lock?

> +               spin_unlock_irqrestore(&host->lock, flags);
> +
> +               if (!(events & event_mask))
> +                       break;
> +
> +               if (!mrq) {
> +                       dev_err(host->dev,
> +                               "%s: MRQ=NULL; events=%08X; event_mask=%08X\n",
> +                               __func__, events, event_mask);
> +                       WARN_ON(1);
> +                       break;
> +               }
> +
> +               dev_dbg(host->dev, "%s: events=%08X\n", __func__, events);
> +
> +               if (cmd)
> +                       msdc_cmd_done(host, events, mrq, cmd);
> +               else if (data)
> +                       msdc_data_xfer_done(host, events, mrq, data);
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void msdc_init_hw(struct msdc_host *host)
> +{
> +       u32 val;
> +       /* Configure to MMC/SD mode */
> +       sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_MODE, MSDC_SDMMC);
> +
> +       /* Reset */
> +       msdc_reset_hw(host);
> +
> +       /* Disable card detection */
> +       sdr_clr_bits(host->base + MSDC_PS, MSDC_PS_CDEN);
> +
> +       /* Disable and clear all interrupts */
> +       writel(0, host->base + MSDC_INTEN);
> +       val = readl(host->base + MSDC_INT);
> +       writel(val, host->base + MSDC_INT);
> +
> +       writel(0, host->base + MSDC_PAD_TUNE);
> +       writel(0, host->base + MSDC_IOCON);
> +       sdr_set_field(host->base + MSDC_IOCON, MSDC_IOCON_DDLSEL, 1);
> +       writel(0x403c004f, host->base + MSDC_PATCH_BIT);
> +       sdr_set_field(host->base + MSDC_PATCH_BIT, MSDC_CKGEN_MSDC_DLY_SEL, 1);
> +       writel(0xffff0089, host->base + MSDC_PATCH_BIT1);
> +       /* Configure to enable SDIO mode.
> +          it's must otherwise sdio cmd5 failed */
> +       sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIO);
> +
> +       /* disable detect SDIO device interrupt function */
> +       sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> +
> +       /* Configure to default data timeout */
> +       sdr_set_field(host->base + SDC_CFG, SDC_CFG_DTOC, 3);
> +       msdc_set_buswidth(host, MMC_BUS_WIDTH_1);
> +
> +       dev_dbg(host->dev, "init hardware done!");
> +}
> +
> +static void msdc_deinit_hw(struct msdc_host *host)
> +{
> +       u32 val;
> +
> +       /* Disable and clear all interrupts */
> +       writel(0, host->base + MSDC_INTEN);
> +
> +       val = readl(host->base + MSDC_INT);
> +       writel(val, host->base + MSDC_INT);
> +
> +       clk_disable(host->src_clk);
> +       clk_unprepare(host->src_clk);

Please use clk_disable_unprepare() instead.

> +}
> +
> +/* init gpd and bd list in msdc_drv_probe */
> +static void msdc_init_gpd_bd(struct msdc_host *host, struct msdc_dma *dma)
> +{
> +       struct mt_gpdma_desc *gpd = dma->gpd;
> +       struct mt_bdma_desc *bd = dma->bd;
> +       int i;
> +
> +       memset(gpd, 0, sizeof(struct mt_gpdma_desc) * MAX_GPD_NUM);
> +       gpd->next = (u32)dma->gpd_addr + sizeof(struct mt_gpdma_desc);
> +
> +       gpd->first_u32 |= GPDMA_DESC_BDP; /* hwo, cs, bd pointer */
> +       gpd->ptr = (u32)dma->bd_addr; /* physical address */
> +
> +       memset(bd, 0, sizeof(struct mt_bdma_desc) * MAX_BD_PER_GPD);
> +
> +       for (i = 0; i < (MAX_BD_PER_GPD - 1); i++)
> +               bd[i].next = (u32)dma->bd_addr + sizeof(*bd) * (i + 1);
> +}
> +
> +static int timing_is_uhs(unsigned char timing)
> +{
> +       if (timing != MMC_TIMING_LEGACY &&
> +                       timing != MMC_TIMING_MMC_HS &&
> +                       timing != MMC_TIMING_SD_HS)

I don't like these assumtions. There may be other cases which isn't
covered here that isn't UHS.

> +               return 1;
> +       return 0;
> +}
> +
> +static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       struct msdc_host *host = mmc_priv(mmc);
> +       int ret;
> +       u32 ddr = 0;
> +
> +       if (ios->timing == MMC_TIMING_UHS_DDR50)
> +               ddr = 1;

I am not sure if this controller supports eMMC cards as well, but if
that's the case and it also supports DDR you may also want to check
for MMC_TIMING_MMC_DDR52.

> +
> +       msdc_set_buswidth(host, ios->bus_width);
> +
> +       /* Suspend/Resume will do power off/on */
> +       switch (ios->power_mode) {
> +       case MMC_POWER_UP:
> +               msdc_init_hw(host);
> +               if (!IS_ERR(mmc->supply.vmmc)) {
> +                       ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> +                                       ios->vdd);
> +                       if (ret) {
> +                               dev_err(host->dev, "Failed to set vmmc power!\n");
> +                               return;
> +                       }
> +               }
> +               break;
> +       case MMC_POWER_ON:
> +               if (!IS_ERR(mmc->supply.vqmmc)) {
> +                       ret = regulator_enable(mmc->supply.vqmmc);

The calls to regulator_enable|disable() for the vqmmc will not be
balanced properly here. You need a local cache variable like
"is_enabled" to keep track of this.

> +                       if (ret)
> +                               dev_err(host->dev, "Failed to set vqmmc power!\n");
> +               }
> +               break;
> +       case MMC_POWER_OFF:
> +               if (!IS_ERR(mmc->supply.vmmc))
> +                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> +
> +               if (!IS_ERR(mmc->supply.vqmmc))
> +                       regulator_disable(mmc->supply.vqmmc);
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       /* Apply different pinctrl settings for different timing */
> +       if (timing_is_uhs(ios->timing))
> +               pinctrl_select_state(host->pinctrl, host->pins_uhs);
> +       else
> +               pinctrl_select_state(host->pinctrl, host->pins_default);
> +
> +       if (host->mclk != ios->clock || host->ddr != ddr)
> +               msdc_set_mclk(host, ddr, ios->clock);
> +}
> +
> +static struct mmc_host_ops mt_msdc_ops = {
> +       .post_req = msdc_post_req,
> +       .pre_req = msdc_pre_req,
> +       .request = msdc_ops_request,
> +       .set_ios = msdc_ops_set_ios,
> +       .start_signal_voltage_switch = msdc_ops_switch_volt,
> +};
> +
> +static int msdc_drv_probe(struct platform_device *pdev)
> +{
> +       struct mmc_host *mmc;
> +       struct msdc_host *host;
> +       struct resource *res;
> +       int ret;
> +
> +       if (!pdev->dev.of_node) {
> +               dev_err(&pdev->dev, "No DT found\n");
> +               return -EINVAL;
> +       }
> +       /* Allocate MMC host for this device */
> +       mmc = mmc_alloc_host(sizeof(struct msdc_host), &pdev->dev);
> +       if (!mmc)
> +               return -ENOMEM;
> +
> +       host = mmc_priv(mmc);
> +       ret = mmc_of_parse(mmc);
> +       if (ret)
> +               goto host_free;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       host->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(host->base)) {
> +               ret = PTR_ERR(host->base);
> +               goto host_free;
> +       }
> +
> +       ret = mmc_regulator_get_supply(mmc);
> +       if (ret == -EPROBE_DEFER)
> +               goto host_free;
> +
> +       host->src_clk = devm_clk_get(&pdev->dev, "source");
> +       if (IS_ERR(host->src_clk)) {
> +               dev_err(&pdev->dev,
> +                               "Invalid source clock from the device tree!\n");

You can remove this print, it's already handled by the common clk framework.

> +               ret = PTR_ERR(host->src_clk);
> +               goto host_free;
> +       }
> +
> +       host->irq = platform_get_irq(pdev, 0);
> +       if (host->irq < 0) {
> +               ret = -EINVAL;
> +               goto host_free;
> +       }
> +
> +       host->pinctrl = devm_pinctrl_get(&pdev->dev);
> +       if (IS_ERR(host->pinctrl)) {
> +               ret = PTR_ERR(host->pinctrl);
> +               dev_err(&pdev->dev, "Cannot find pinctrl!\n");
> +               goto host_free;
> +       }
> +
> +       host->pins_default = pinctrl_lookup_state(host->pinctrl, "default");
> +       if (IS_ERR(host->pins_default)) {
> +               ret = PTR_ERR(host->pins_default);
> +               dev_err(&pdev->dev, "Cannot find pinctrl default!\n");
> +               goto host_free;
> +       }
> +
> +       host->pins_uhs = pinctrl_lookup_state(host->pinctrl, "state_uhs");
> +       if (IS_ERR(host->pins_uhs)) {
> +               ret = PTR_ERR(host->pins_uhs);
> +               dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> +               goto host_free;
> +       }
> +
> +       host->dev = &pdev->dev;
> +       host->mmc = mmc;
> +       /* Set host parameters to mmc */
> +       mmc->ops = &mt_msdc_ops;
> +       mmc->f_min = 260000;

Doesn't f_min depend on some internal divider algorithm and the clock
rate? So, then it should maybe be calculated instead of fixed?

> +       mmc->ocr_avail = MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33;

"ocr_avail" should be fetched from the vmmc regulator, when you invoke
mmc_regulator_get_supply() above.

> +
> +       mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
> +       /* MMC core transfer sizes tunable parameters */
> +       mmc->max_segs = MAX_BD_NUM;
> +       mmc->max_seg_size = 64 * 1024;
> +       mmc->max_blk_size = 2048;
> +       mmc->max_req_size = 512 * 1024;
> +       mmc->max_blk_count = mmc->max_req_size;
> +
> +       host->hclk = clk_get_rate(host->src_clk);
> +       host->timeout_clks = 3 * 1048576;
> +       host->dma.gpd = dma_alloc_coherent(&pdev->dev,
> +                               MAX_GPD_NUM * sizeof(struct mt_gpdma_desc),
> +                               &host->dma.gpd_addr, GFP_KERNEL);
> +       host->dma.bd = dma_alloc_coherent(&pdev->dev,
> +                               MAX_BD_NUM * sizeof(struct mt_bdma_desc),
> +                               &host->dma.bd_addr, GFP_KERNEL);
> +       if ((!host->dma.gpd) || (!host->dma.bd)) {
> +               ret = -ENOMEM;
> +               goto release_mem;
> +       }
> +       msdc_init_gpd_bd(host, &host->dma);
> +       INIT_DELAYED_WORK(&host->req_timeout, msdc_request_timeout);
> +       spin_lock_init(&host->lock);
> +
> +       platform_set_drvdata(pdev, mmc);
> +       clk_prepare(host->src_clk);

This should be clk_prepare_enable().

> +
> +       ret = devm_request_irq(&pdev->dev, (unsigned int) host->irq, msdc_irq,
> +               IRQF_TRIGGER_LOW | IRQF_ONESHOT, pdev->name, host);
> +       if (ret)
> +               goto release;
> +
> +       ret = mmc_add_host(mmc);
> +       if (ret)
> +               goto release;
> +
> +       return 0;
> +
> +release:
> +       platform_set_drvdata(pdev, NULL);
> +       msdc_deinit_hw(host);
> +release_mem:
> +       if (host->dma.gpd)
> +               dma_free_coherent(&pdev->dev,
> +                       MAX_GPD_NUM * sizeof(struct mt_gpdma_desc),
> +                       host->dma.gpd, host->dma.gpd_addr);
> +       if (host->dma.bd)
> +               dma_free_coherent(&pdev->dev,
> +                       MAX_BD_NUM * sizeof(struct mt_bdma_desc),
> +                       host->dma.bd, host->dma.bd_addr);
> +host_free:
> +       mmc_free_host(mmc);
> +
> +       return ret;
> +}
> +
> +static int msdc_drv_remove(struct platform_device *pdev)
> +{
> +       struct mmc_host *mmc;
> +       struct msdc_host *host;
> +
> +       mmc = platform_get_drvdata(pdev);
> +       host = mmc_priv(mmc);
> +
> +       platform_set_drvdata(pdev, NULL);
> +       mmc_remove_host(host->mmc);
> +       msdc_deinit_hw(host);
> +
> +       dma_free_coherent(&pdev->dev,
> +                       MAX_GPD_NUM * sizeof(struct mt_gpdma_desc),
> +                       host->dma.gpd, host->dma.gpd_addr);
> +       dma_free_coherent(&pdev->dev, MAX_BD_NUM * sizeof(struct mt_bdma_desc),
> +                       host->dma.bd, host->dma.bd_addr);
> +
> +       mmc_free_host(host->mmc);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id msdc_of_ids[] = {
> +       {   .compatible = "mediatek,mt8135-mmc", },
> +       {}
> +};
> +
> +static struct platform_driver mt_msdc_driver = {
> +       .probe = msdc_drv_probe,
> +       .remove = msdc_drv_remove,
> +       .driver = {
> +               .name = "mtk-msdc",
> +               .of_match_table = msdc_of_ids,
> +       },
> +};
> +
> +module_platform_driver(mt_msdc_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MediaTek SD/MMC Card Driver");
> --
> 1.8.1.1.dirty
>

Kind regards
Uffe

答复: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver

[Chaotian Jing (井朝天)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 66128 bytes --]

Dear Ulf,

Thanks for your review!
Please help to check my comment.

> -----邮件原件-----
> 发件人: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> 发送时间: 2015年3月31日 20:23
> 收件人: Chaotian Jing (井朝天)
> 抄送: Rob Herring; Matthias Brugger; Chris Ball; Mark Rutland; JamesJJ Liao
> (廖建智); srv_heupstream; Arnd Bergmann; devicetree@vger.kernel.org;
> HONGZHOU Yang; Catalin Marinas; linux-mmc; Will Deacon;
> linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; Sascha Hauer;
> Yingjoe Chen (陳英洲); Eddie Huang (黃智傑); Bin Zhang (章斌);
> linux-arm-kernel@lists.infradead.org; linux-mediatek@lists.infradead.org
> 主题: Re: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver
> 
> On 17 March 2015 at 04:13, Chaotian Jing <chaotian.jing@mediatek.com>
> wrote:
> > Add Mediatek MMC driver code
> 
> Would be nice to get simple overview of what features the
> driver/controller supports. Please add that to the commit message.
> 
Ok, will add it at next revision.
> >
> > Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> > ---
> >  drivers/mmc/host/Kconfig  |    8 +
> >  drivers/mmc/host/Makefile |    1 +
> >  drivers/mmc/host/mtk-sd.c | 1412
> +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 1421 insertions(+)
> >  create mode 100644 drivers/mmc/host/mtk-sd.c
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 61ac63a..8b64377 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -768,3 +768,11 @@ config MMC_TOSHIBA_PCI
> >         tristate "Toshiba Type A SD/MMC Card Interface Driver"
> >         depends on PCI
> >         help
> > +
> > +config MMC_MTK
> > +       tristate "MediaTek SD/MMC Card Interface support"
> > +       help
> > +         This selects the MediaTek(R) Secure digital and Multimedia card
> Interface.
> > +         If you have a machine with a integrated SD/MMC card reader,
> say Y or M here.
> > +         This is needed if support for any SD/SDIO/MMC devices is
> required.
> > +         If unsure, say N.
> > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> > index 6a7cfe0..161b3e4 100644
> > --- a/drivers/mmc/host/Makefile
> > +++ b/drivers/mmc/host/Makefile
> > @@ -20,6 +20,7 @@ obj-$(CONFIG_MMC_SDHCI_F_SDH30)       +=
> sdhci_f_sdh30.o
> >  obj-$(CONFIG_MMC_SDHCI_SPEAR)  += sdhci-spear.o
> >  obj-$(CONFIG_MMC_WBSD)         += wbsd.o
> >  obj-$(CONFIG_MMC_AU1X)         += au1xmmc.o
> > +obj-$(CONFIG_MMC_MTK)           += mtk-sd.o
> >  obj-$(CONFIG_MMC_OMAP)         += omap.o
> >  obj-$(CONFIG_MMC_OMAP_HS)      += omap_hsmmc.o
> >  obj-$(CONFIG_MMC_ATMELMCI)     += atmel-mci.o
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > new file mode 100644
> > index 0000000..86c999b
> > --- /dev/null
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -0,0 +1,1412 @@
> > +/*
> > + * Copyright (c) 2014-2015 MediaTek Inc.
> > + * Author: Chaotian.Jing <chaotian.jing@mediatek.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/ioport.h>
> > +#include <linux/irq.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spinlock.h>
> > +
> > +#include <linux/mmc/card.h>
> > +#include <linux/mmc/core.h>
> > +#include <linux/mmc/host.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/mmc/sd.h>
> > +#include <linux/mmc/sdio.h>
> > +
> > +#define MAX_GPD_NUM         (1 + 1)    /* one null gpd */
> > +#define MAX_BD_NUM          1024
> > +#define MAX_BD_PER_GPD      MAX_BD_NUM
> > +
> > +/*--------------------------------------------------------------------------*/
> > +/* Common Definition
> */
> > +/*--------------------------------------------------------------------------*/
> > +#define MSDC_MS                 0
> > +#define MSDC_SDMMC              1
> 
> Please convert to hex values.
Ok, will fix at next revision.
> 
> > +
> > +#define MSDC_BUS_1BITS          0
> > +#define MSDC_BUS_4BITS          1
> > +#define MSDC_BUS_8BITS          2
> 
> Please convert to hex values.
> 
Ok, will fix at next revision.
> > +
> > +#define MSDC_BURST_8B           3
> > +#define MSDC_BURST_16B          4
> > +#define MSDC_BURST_32B          5
> > +#define MSDC_BURST_64B          6
> 
> I don't find the first three being used, should we remove them?
> 
> Also, I think converting them to hex values make sense.
> 
Ok, will remove them and convert it to hex value
> 
> > +
> > +/*--------------------------------------------------------------------------*/
> > +/* Register Offset
> */
> > +/*--------------------------------------------------------------------------*/
> > +#define MSDC_CFG         0x0
> > +#define MSDC_IOCON       0x04
> > +#define MSDC_PS          0x08
> > +#define MSDC_INT         0x0c
> > +#define MSDC_INTEN       0x10
> > +#define MSDC_FIFOCS      0x14
> > +#define SDC_CFG          0x30
> > +#define SDC_CMD          0x34
> > +#define SDC_ARG          0x38
> > +#define SDC_STS          0x3c
> > +#define SDC_RESP0        0x40
> > +#define SDC_RESP1        0x44
> > +#define SDC_RESP2        0x48
> > +#define SDC_RESP3        0x4c
> > +#define SDC_BLK_NUM      0x50
> > +#define SDC_ACMD_RESP    0x80
> > +#define MSDC_DMA_SA      0x90
> > +#define MSDC_DMA_CTRL    0x98
> > +#define MSDC_DMA_CFG     0x9c
> > +#define MSDC_PATCH_BIT   0xb0
> > +#define MSDC_PATCH_BIT1  0xb4
> > +#define MSDC_PAD_TUNE    0xec
> > +
> > +/*--------------------------------------------------------------------------*/
> > +/* Register Mask
> */
> > +/*--------------------------------------------------------------------------*/
> > +
> > +/* MSDC_CFG mask */
> > +#define MSDC_CFG_MODE           (1 << 0)       /* RW */
> > +#define MSDC_CFG_CKPDN          (1 << 1)       /* RW */
> > +#define MSDC_CFG_RST            (1 << 2)       /* RW */
> > +#define MSDC_CFG_PIO            (1 << 3)       /* RW */
> > +#define MSDC_CFG_CKDRVEN        (1 << 4)       /* RW */
> > +#define MSDC_CFG_BV18SDT        (1 << 5)       /* RW */
> > +#define MSDC_CFG_BV18PSS        (1 << 6)       /* R  */
> > +#define MSDC_CFG_CKSTB          (1 << 7)       /* R  */
> > +#define MSDC_CFG_CKDIV          (0xff << 8)    /* RW */
> > +#define MSDC_CFG_CKMOD          (0x3 << 16)    /* RW */
> > +
> > +/* MSDC_IOCON mask */
> > +#define MSDC_IOCON_SDR104CKS    (1 << 0)       /* RW */
> > +#define MSDC_IOCON_RSPL         (1 << 1)       /* RW */
> > +#define MSDC_IOCON_DSPL         (1 << 2)       /* RW */
> > +#define MSDC_IOCON_DDLSEL       (1 << 3)       /* RW */
> > +#define MSDC_IOCON_DDR50CKD     (1 << 4)       /* RW */
> > +#define MSDC_IOCON_DSPLSEL      (1 << 5)       /* RW */
> > +#define MSDC_IOCON_W_DSPL       (1 << 8)       /* RW */
> > +#define MSDC_IOCON_D0SPL        (1 << 16)      /* RW */
> > +#define MSDC_IOCON_D1SPL        (1 << 17)      /* RW */
> > +#define MSDC_IOCON_D2SPL        (1 << 18)      /* RW */
> > +#define MSDC_IOCON_D3SPL        (1 << 19)      /* RW */
> > +#define MSDC_IOCON_D4SPL        (1 << 20)      /* RW */
> > +#define MSDC_IOCON_D5SPL        (1 << 21)      /* RW */
> > +#define MSDC_IOCON_D6SPL        (1 << 22)      /* RW */
> > +#define MSDC_IOCON_D7SPL        (1 << 23)      /* RW */
> > +#define MSDC_IOCON_RISCSZ       (0x3 << 24)    /* RW */
> > +
> > +/* MSDC_PS mask */
> > +#define MSDC_PS_CDEN            (1 << 0)       /* RW */
> > +#define MSDC_PS_CDSTS           (1 << 1)       /* R  */
> > +#define MSDC_PS_CDDEBOUNCE      (0xf << 12)    /* RW */
> > +#define MSDC_PS_DAT             (0xff << 16)   /* R  */
> > +#define MSDC_PS_CMD             (1 << 24)      /* R  */
> > +#define MSDC_PS_WP              (1 << 31)      /* R  */
> > +
> > +/* MSDC_INT mask */
> > +#define MSDC_INT_MMCIRQ         (1 << 0)       /* W1C */
> > +#define MSDC_INT_CDSC           (1 << 1)       /* W1C */
> > +#define MSDC_INT_ACMDRDY        (1 << 3)       /* W1C */
> > +#define MSDC_INT_ACMDTMO        (1 << 4)       /* W1C */
> > +#define MSDC_INT_ACMDCRCERR     (1 << 5)       /* W1C */
> > +#define MSDC_INT_DMAQ_EMPTY     (1 << 6)       /* W1C */
> > +#define MSDC_INT_SDIOIRQ        (1 << 7)       /* W1C */
> > +#define MSDC_INT_CMDRDY         (1 << 8)       /* W1C */
> > +#define MSDC_INT_CMDTMO         (1 << 9)       /* W1C */
> > +#define MSDC_INT_RSPCRCERR      (1 << 10)      /* W1C */
> > +#define MSDC_INT_CSTA           (1 << 11)      /* R */
> > +#define MSDC_INT_XFER_COMPL     (1 << 12)      /* W1C */
> > +#define MSDC_INT_DXFER_DONE     (1 << 13)      /* W1C */
> > +#define MSDC_INT_DATTMO         (1 << 14)      /* W1C */
> > +#define MSDC_INT_DATCRCERR      (1 << 15)      /* W1C */
> > +#define MSDC_INT_ACMD19_DONE    (1 << 16)      /* W1C */
> > +#define MSDC_INT_DMA_BDCSERR    (1 << 17)      /* W1C */
> > +#define MSDC_INT_DMA_GPDCSERR   (1 << 18)      /* W1C */
> > +#define MSDC_INT_DMA_PROTECT    (1 << 19)      /* W1C */
> > +
> > +/* MSDC_INTEN mask */
> > +#define MSDC_INTEN_MMCIRQ       (1 << 0)       /* RW */
> > +#define MSDC_INTEN_CDSC         (1 << 1)       /* RW */
> > +#define MSDC_INTEN_ACMDRDY      (1 << 3)       /* RW */
> > +#define MSDC_INTEN_ACMDTMO      (1 << 4)       /* RW */
> > +#define MSDC_INTEN_ACMDCRCERR   (1 << 5)       /* RW */
> > +#define MSDC_INTEN_DMAQ_EMPTY   (1 << 6)       /* RW */
> > +#define MSDC_INTEN_SDIOIRQ      (1 << 7)       /* RW */
> > +#define MSDC_INTEN_CMDRDY       (1 << 8)       /* RW */
> > +#define MSDC_INTEN_CMDTMO       (1 << 9)       /* RW */
> > +#define MSDC_INTEN_RSPCRCERR    (1 << 10)      /* RW */
> > +#define MSDC_INTEN_CSTA         (1 << 11)      /* RW */
> > +#define MSDC_INTEN_XFER_COMPL   (1 << 12)      /* RW */
> > +#define MSDC_INTEN_DXFER_DONE   (1 << 13)      /* RW */
> > +#define MSDC_INTEN_DATTMO       (1 << 14)      /* RW */
> > +#define MSDC_INTEN_DATCRCERR    (1 << 15)      /* RW */
> > +#define MSDC_INTEN_ACMD19_DONE  (1 << 16)      /* RW */
> > +#define MSDC_INTEN_DMA_BDCSERR  (1 << 17)      /* RW */
> > +#define MSDC_INTEN_DMA_GPDCSERR (1 << 18)      /* RW */
> > +#define MSDC_INTEN_DMA_PROTECT  (1 << 19)      /* RW */
> > +
> > +/* MSDC_FIFOCS mask */
> > +#define MSDC_FIFOCS_RXCNT       (0xff << 0)    /* R */
> > +#define MSDC_FIFOCS_TXCNT       (0xff << 16)   /* R */
> > +#define MSDC_FIFOCS_CLR         (1 << 31)      /* RW */
> > +
> > +/* SDC_CFG mask */
> > +#define SDC_CFG_SDIOINTWKUP     (1 << 0)       /* RW */
> > +#define SDC_CFG_INSWKUP         (1 << 1)       /* RW */
> > +#define SDC_CFG_BUSWIDTH        (0x3 << 16)    /* RW */
> > +#define SDC_CFG_SDIO            (1 << 19)      /* RW */
> > +#define SDC_CFG_SDIOIDE         (1 << 20)      /* RW */
> > +#define SDC_CFG_INTATGAP        (1 << 21)      /* RW */
> > +#define SDC_CFG_DTOC            (0xff << 24)   /* RW */
> > +
> > +/* SDC_STS mask */
> > +#define SDC_STS_SDCBUSY         (1 << 0)       /* RW */
> > +#define SDC_STS_CMDBUSY         (1 << 1)       /* RW */
> > +#define SDC_STS_SWR_COMPL       (1 << 31)      /* RW */
> > +
> > +/* MSDC_DMA_CTRL mask */
> > +#define MSDC_DMA_CTRL_START     (1 << 0)       /* W */
> > +#define MSDC_DMA_CTRL_STOP      (1 << 1)       /* W */
> > +#define MSDC_DMA_CTRL_RESUME    (1 << 2)       /* W */
> > +#define MSDC_DMA_CTRL_MODE      (1 << 8)       /* RW */
> > +#define MSDC_DMA_CTRL_LASTBUF   (1 << 10)      /* RW */
> > +#define MSDC_DMA_CTRL_BRUSTSZ   (0x7 << 12)    /* RW */
> > +
> > +/* MSDC_DMA_CFG mask */
> > +#define MSDC_DMA_CFG_STS        (1 << 0)       /* R */
> > +#define MSDC_DMA_CFG_DECSEN     (1 << 1)       /* RW */
> > +#define MSDC_DMA_CFG_AHBHPROT2  (0x2 << 8)     /* RW */
> > +#define MSDC_DMA_CFG_ACTIVEEN   (0x2 << 12)    /* RW */
> > +#define MSDC_DMA_CFG_CS12B16B   (1 << 16)      /* RW */
> > +
> > +/* MSDC_PATCH_BIT mask */
> > +#define MSDC_PATCH_BIT_ODDSUPP    (1 <<  1)    /* RW */
> > +#define MSDC_INT_DAT_LATCH_CK_SEL (0x7 <<  7)
> > +#define MSDC_CKGEN_MSDC_DLY_SEL   (0x1f << 10)
> > +#define MSDC_PATCH_BIT_IODSSEL    (1 << 16)    /* RW */
> > +#define MSDC_PATCH_BIT_IOINTSEL   (1 << 17)    /* RW */
> > +#define MSDC_PATCH_BIT_BUSYDLY    (0xf << 18)  /* RW */
> > +#define MSDC_PATCH_BIT_WDOD       (0xf << 22)  /* RW */
> > +#define MSDC_PATCH_BIT_IDRTSEL    (1 << 26)    /* RW */
> > +#define MSDC_PATCH_BIT_CMDFSEL    (1 << 27)    /* RW */
> > +#define MSDC_PATCH_BIT_INTDLSEL   (1 << 28)    /* RW */
> > +#define MSDC_PATCH_BIT_SPCPUSH    (1 << 29)    /* RW */
> > +#define MSDC_PATCH_BIT_DECRCTMO   (1 << 30)    /* RW */
> > +
> > +#define REQ_CMD_EIO  (1 << 0)
> > +#define REQ_CMD_TMO  (1 << 1)
> > +#define REQ_DAT_ERR  (1 << 2)
> > +#define REQ_STOP_EIO (1 << 3)
> > +#define REQ_STOP_TMO (1 << 4)
> > +#define REQ_CMD_BUSY (1 << 5)
> > +
> > +#define MSDC_PREPARE_FLAG (1 << 0)
> > +#define MSDC_ASYNC_FLAG (1 << 1)
> > +#define MSDC_MMAP_FLAG (1 << 2)
> > +
> > +#define HOST_MAX_BLKSZ      2048
> > +#define MSDC_OCR_AVAIL      (MMC_VDD_28_29 | MMC_VDD_29_30 \
> > +               | MMC_VDD_30_31 | MMC_VDD_31_32 |
> MMC_VDD_32_33)
> 
> Both these aren't used. Remove please.
> 
Ok, will remove it at next revision.
> > +
> > +#define CMD_TIMEOUT         (HZ/10 * 5)        /* 100ms x5 */
> > +#define DAT_TIMEOUT         (HZ    * 5)        /* 1000ms x5 */
> > +
> > +/*--------------------------------------------------------------------------*/
> > +/* Descriptor Structure
> */
> > +/*--------------------------------------------------------------------------*/
> > +struct mt_gpdma_desc {
> > +       u32 first_u32;
> > +#define GPDMA_DESC_HWO         (1 << 0)
> > +#define GPDMA_DESC_BDP         (1 << 1)
> > +#define GPDMA_DESC_CHECKSUM    (0xff << 8) /* bit8 ~ bit15 */
> > +#define GPDMA_DESC_INT         (1 << 16)
> > +       u32 next;
> > +       u32 ptr;
> > +       u32 second_u32;
> > +#define GPDMA_DESC_BUFLEN      (0xffff) /* bit0 ~ bit15 */
> > +#define GPDMA_DESC_EXTLEN      (0xff << 16) /* bit16 ~ bit23 */
> > +       u32 arg;
> > +       u32 blknum;
> > +       u32 cmd;
> > +};
> > +
> > +struct mt_bdma_desc {
> > +       u32 first_u32;
> > +#define BDMA_DESC_EOL          (1 << 0)
> > +#define BDMA_DESC_CHECKSUM     (0xff << 8) /* bit8 ~ bit15 */
> > +#define BDMA_DESC_BLKPAD       (1 << 17)
> > +#define BDMA_DESC_DWPAD                (1 << 18)
> > +       u32 next;
> > +       u32 ptr;
> > +       u32 second_u32;
> > +#define BDMA_DESC_BUFLEN       (0xffff) /* bit0 ~ bit15 */
> > +};
> > +
> > +struct msdc_dma {
> > +       struct scatterlist *sg; /* I/O scatter list */
> > +       struct mt_gpdma_desc *gpd;              /* pointer to gpd
> array */
> > +       struct mt_bdma_desc *bd;                /* pointer to bd
> array */
> > +       dma_addr_t gpd_addr;    /* the physical address of gpd array */
> > +       dma_addr_t bd_addr;     /* the physical address of bd array */
> > +};
> 
> This looks weird from DMA perspective. Can you elaborate on why you
> can't use the dmaengine API?
> 
The gpd and bd structure are determined by the MSDC hw, and, the DMA controller is a part of the MSDC hw, different with the chain dma implemented by the kernel.
> > +
> > +struct msdc_host {
> > +       struct device *dev;
> > +       struct mmc_host *mmc;   /* mmc structure */
> > +       int cmd_rsp;
> > +
> > +       spinlock_t lock;
> > +       struct mmc_request *mrq;
> > +       struct mmc_command *cmd;
> > +       struct mmc_data *data;
> > +       int error;
> > +
> > +       void __iomem *base;             /* host base address */
> > +
> > +       struct msdc_dma dma;    /* dma channel */
> > +
> > +       u32 timeout_ns;         /* data timeout ns */
> > +       u32 timeout_clks;       /* data timeout clks */
> > +
> > +       struct pinctrl *pinctrl;
> > +       struct pinctrl_state *pins_default;
> > +       struct pinctrl_state *pins_uhs;
> > +       struct delayed_work req_timeout;
> > +       int irq;                /* host interrupt */
> > +
> > +       struct clk *src_clk;    /* msdc source clock */
> > +       u32 mclk;               /* mmc subsystem clock */
> > +       u32 hclk;               /* host clock speed */
> > +       u32 sclk;               /* SD/MS clock speed */
> > +       bool ddr;
> > +};
> > +
> > +static void sdr_set_bits(void __iomem *reg, u32 bs)
> > +{
> > +       u32 val = readl(reg);
> > +
> > +       val |= bs;
> > +       writel(val, reg);
> > +}
> > +
> > +static void sdr_clr_bits(void __iomem *reg, u32 bs)
> > +{
> > +       u32 val = readl(reg);
> > +
> > +       val &= ~(u32)bs;
> > +       writel(val, reg);
> > +}
> > +
> > +static void sdr_set_field(void __iomem *reg, u32 field, u32 val)
> > +{
> > +       unsigned int tv = readl(reg);
> > +
> > +       tv &= ~field;
> > +       tv |= ((val) << (ffs((unsigned int)field) - 1));
> > +       writel(tv, reg);
> > +}
> 
> A common thought for all the three above functions:
> 
> Have you considered using a cache variable for those registers that
> often gets updated? In that way you would have to read the register
> value every time when you want to write to it. It should improve
> performance a bit.
> 
These register can be modified by the MSDC hw, cannot cache it.
> > +
> > +static void sdr_get_field(void __iomem *reg, u32 field, u32 *val)
> > +{
> > +       unsigned int tv = readl(reg);
> > +
> > +       *val = ((tv & field) >> (ffs((unsigned int)field) - 1));
> > +}
> > +
> > +static void msdc_reset_hw(struct msdc_host *host)
> > +{
> > +       u32 val;
> > +
> > +       sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_RST);
> > +       while (readl(host->base + MSDC_CFG) & MSDC_CFG_RST)
> > +               cpu_relax();
> > +
> > +       sdr_set_bits(host->base + MSDC_FIFOCS, MSDC_FIFOCS_CLR);
> > +       while (readl(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_CLR)
> > +               cpu_relax();
> > +
> > +       val = readl(host->base + MSDC_INT);
> > +       writel(val, host->base + MSDC_INT);
> > +}
> > +
> > +static void msdc_cmd_next(struct msdc_host *host,
> > +               struct mmc_request *mrq, struct mmc_command *cmd);
> 
> Please investigate whether you can move around the code to prevent
> this static declaration of the function.
> 
It is hard to do it, because the first cmd may be CMD23, and do the next is send the mrq->cmd, so, there is a loop here, at least on method need do static declaration,
> > +
> > +static inline u32 msdc_data_ints_mask(struct msdc_host *host)
> > +{
> > +       u32 wints =
> > +               /* data xfer */
> > +               MSDC_INTEN_XFER_COMPL |
> > +               MSDC_INTEN_DATTMO |
> > +               MSDC_INTEN_DATCRCERR |
> > +               /* DMA events */
> > +               MSDC_INTEN_DMA_BDCSERR |
> > +               MSDC_INTEN_DMA_GPDCSERR |
> > +               MSDC_INTEN_DMA_PROTECT;
> > +
> > +       return wints;
> > +}
> 
> This should be a static variable instead of a function.
> 
Ok, will fix at next revision.
> > +
> > +static u8 msdc_dma_calcs(u8 *buf, u32 len)
> > +{
> > +       u32 i, sum = 0;
> > +
> > +       for (i = 0; i < len; i++)
> > +               sum += buf[i];
> > +       return 0xff - (u8) sum;
> > +}
> > +
> > +static inline void msdc_dma_setup(struct msdc_host *host, struct
> msdc_dma *dma,
> > +               struct mmc_data *data)
> > +{
> > +       u32 sglen, j;
> > +       u32 dma_address, dma_len;
> > +       struct scatterlist *sg;
> > +       struct mt_gpdma_desc *gpd;
> > +       struct mt_bdma_desc *bd;
> > +
> > +       sglen = data->sg_len;
> > +       sg = data->sg;
> > +
> > +       gpd = dma->gpd;
> > +       bd = dma->bd;
> > +
> > +       /* modify gpd */
> > +       gpd->first_u32 |= GPDMA_DESC_HWO;
> > +       gpd->first_u32 |= GPDMA_DESC_BDP;
> > +       /* need to clear first. use these bits to calc checksum */
> > +       gpd->first_u32 &= ~GPDMA_DESC_CHECKSUM;
> > +       gpd->first_u32 |= msdc_dma_calcs((u8 *) gpd, 16) << 8;
> > +
> > +       /* modify bd */
> > +       for (j = 0; j < sglen; j++) {
> > +               dma_address = sg_dma_address(sg);
> > +               dma_len = sg_dma_len(sg);
> > +
> > +               /* init bd */
> > +               bd[j].first_u32 &= ~BDMA_DESC_BLKPAD;
> > +               bd[j].first_u32 &= ~BDMA_DESC_DWPAD;
> > +               bd[j].ptr = (u32)dma_address;
> > +               bd[j].second_u32 &= ~BDMA_DESC_BUFLEN;
> > +               bd[j].second_u32 |= (dma_len & BDMA_DESC_BUFLEN);
> > +
> > +               if (j == sglen - 1) /* the last bd */
> > +                       bd[j].first_u32 |= BDMA_DESC_EOL;
> > +               else
> > +                       bd[j].first_u32 &= ~BDMA_DESC_EOL;
> > +
> > +               /* checksume need to clear first */
> > +               bd[j].first_u32 &= ~BDMA_DESC_CHECKSUM;
> > +               bd[j].first_u32 |= msdc_dma_calcs((u8 *)(&bd[j]), 16) <<
> 8;
> > +               sg++;
> > +       }
> > +
> > +       sdr_set_field(host->base + MSDC_DMA_CFG,
> MSDC_DMA_CFG_DECSEN, 1);
> > +       sdr_set_field(host->base + MSDC_DMA_CTRL,
> MSDC_DMA_CTRL_BRUSTSZ,
> > +                       MSDC_BURST_64B);
> > +       sdr_set_field(host->base + MSDC_DMA_CTRL,
> MSDC_DMA_CTRL_MODE, 1);
> > +
> > +       writel((u32)dma->gpd_addr, host->base + MSDC_DMA_SA);
> > +}
> > +
> > +static void msdc_prepare_data(struct msdc_host *host, struct
> mmc_request *mrq)
> > +{
> > +       struct mmc_data *data = mrq->data;
> > +
> > +       if (!(data->host_cookie & MSDC_PREPARE_FLAG)) {
> > +               bool read = (data->flags & MMC_DATA_READ) != 0;
> > +
> > +               data->host_cookie |= MSDC_PREPARE_FLAG;
> > +               dma_map_sg(host->dev, data->sg, data->sg_len,
> > +                          read ? DMA_FROM_DEVICE :
> DMA_TO_DEVICE);
> > +       }
> > +}
> > +
> > +static void msdc_unprepare_data(struct msdc_host *host, struct
> mmc_request *mrq)
> > +{
> > +       struct mmc_data *data = mrq->data;
> > +
> > +       if (data->host_cookie & MSDC_ASYNC_FLAG)
> > +               return;
> > +
> > +       if ((data->host_cookie & MSDC_PREPARE_FLAG)) {
> > +               bool read = (data->flags & MMC_DATA_READ) != 0;
> > +
> > +               dma_unmap_sg(host->dev, data->sg, data->sg_len,
> > +                            read ? DMA_FROM_DEVICE :
> DMA_TO_DEVICE);
> > +               data->host_cookie &= ~MSDC_PREPARE_FLAG;
> > +       }
> > +}
> > +
> > +/* clock control primitives */
> > +static void msdc_set_timeout(struct msdc_host *host, u32 ns, u32 clks)
> > +{
> > +       u32 timeout, clk_ns;
> > +       u32 mode = 0;
> > +
> > +       host->timeout_ns = ns;
> > +       host->timeout_clks = clks;
> > +       if (host->sclk == 0) {
> > +               timeout = 0;
> > +       } else {
> > +               clk_ns  = 1000000000UL / host->sclk;
> > +               timeout = (ns + clk_ns - 1) / clk_ns + clks;
> > +               /* in 1048576 sclk cycle unit */
> > +               timeout = (timeout + (1 << 20) - 1) >> 20;
> > +               sdr_get_field(host->base + MSDC_CFG,
> MSDC_CFG_CKMOD, &mode);
> > +               /*DDR mode will double the clk cycles for data timeout */
> 
> How do you know you will be using DDR at this point? Don't you need to
> check for that?
> 
The MSDC_CFG_CKMODE can show current mode(DDR or SDR).

> > +               timeout = mode >= 2 ? timeout * 2 : timeout;
> > +               timeout = timeout > 1 ? timeout - 1 : 0;
> > +               timeout = timeout > 255 ? 255 : timeout;
> > +       }
> > +       sdr_set_field(host->base + SDC_CFG, SDC_CFG_DTOC, timeout);
> > +}
> > +
> > +static void msdc_set_mclk(struct msdc_host *host, int ddr, u32 hz)
> > +{
> > +       u32 mode;
> > +       u32 flags;
> > +       u32 div;
> > +       u32 sclk;
> > +       u32 hclk = host->hclk;
> > +
> > +       if (!hz) {
> > +               dev_err(host->dev, "set mclk to 0\n");
> > +               host->mclk = 0;
> 
> This isn't supposed to be treated as an error. The mmc core expect the
> host to gate the clock when the rate is zero.
> 
> This is also needed when dealing with I/O voltage switch for UHS cards.
> 
Yes, will only set the host mode to MS(which will stop the bus clock) and give a dev_dbg message.

> > +               msdc_reset_hw(host);
> > +               return;
> > +       }
> > +
> > +       flags = readl(host->base + MSDC_INTEN);
> > +       sdr_clr_bits(host->base + MSDC_INTEN, flags);
> > +
> > +       if (ddr) { /* may need to modify later */
> > +               mode = 0x2; /* ddr mode and use divisor */
> > +               if (hz >= (hclk >> 2)) {
> > +                       div = 0; /* mean div = 1/4 */
> > +                       sclk = hclk >> 2; /* sclk = clk / 4 */
> > +               } else {
> > +                       div = (hclk + ((hz << 2) - 1)) / (hz << 2);
> > +                       sclk = (hclk >> 2) / div;
> > +                       div = (div >> 1);
> > +               }
> > +       } else if (hz >= hclk) {
> > +               mode = 0x1; /* no divisor */
> > +               div = 0;
> > +               sclk = hclk;
> > +       } else {
> > +               mode = 0x0; /* use divisor */
> > +               if (hz >= (hclk >> 1)) {
> > +                       div = 0; /* mean div = 1/2 */
> > +                       sclk = hclk >> 1; /* sclk = clk / 2 */
> > +               } else {
> > +                       div = (hclk + ((hz << 2) - 1)) / (hz << 2);
> > +                       sclk = (hclk >> 2) / div;
> > +               }
> > +       }
> > +
> > +       sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD |
> MSDC_CFG_CKDIV,
> > +                       (mode << 8) | (div % 0xff));
> > +       while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
> > +               cpu_relax();
> > +
> > +       host->sclk = sclk;
> > +       host->mclk = hz;
> > +       host->ddr = ddr;
> > +       /* need because clk changed. */
> > +       msdc_set_timeout(host, host->timeout_ns, host->timeout_clks);
> > +       sdr_set_bits(host->base + MSDC_INTEN, flags);
> > +
> > +       dev_dbg(host->dev, "sclk: %d, ddr: %d\n", host->sclk, ddr);
> > +}
> > +
> > +static inline u32 msdc_cmd_find_resp(struct msdc_host *host,
> > +               struct mmc_request *mrq, struct mmc_command *cmd)
> > +{
> > +       u32 resp;
> > +
> > +       switch (mmc_resp_type(cmd)) {
> > +               /* Actually, R1, R5, R6, R7 are the same */
> > +       case MMC_RSP_R1:
> > +               resp = 1;
> > +               break;
> > +       case MMC_RSP_R1B:
> > +               resp = 7;
> > +               break;
> > +       case MMC_RSP_R2:
> > +               resp = 2;
> > +               break;
> > +       case MMC_RSP_R3:
> > +               resp = 3;
> > +               break;
> > +       case MMC_RSP_NONE:
> > +       default:
> > +               resp = 0;
> > +               break;
> > +       }
> > +
> > +       return resp;
> > +}
> 
> I would rather see that the above function return hex values instead
> of decimals.
> 
Sure. Will fix it.
> > +
> > +static inline u32 msdc_cmd_prepare_raw_cmd(struct msdc_host *host,
> > +               struct mmc_request *mrq, struct mmc_command *cmd)
> > +{
> > +       /* rawcmd :
> > +        * vol_swt << 30 | auto_cmd << 28 | blklen << 16 | go_irq << 15 |
> > +        * stop << 14 | rw << 13 | dtype << 11 | rsptyp << 7 | brk << 6 |
> opcode
> > +        */
> > +       u32 opcode = cmd->opcode;
> > +       u32 resp = msdc_cmd_find_resp(host, mrq, cmd);
> > +       u32 rawcmd = (opcode & 0x3f) | ((resp & 7) << 7);
> 
> Would be nice to maintain consistency and thus stick to use hex values
> for bitwise OR/AND.
> 
OK, fix it at next revision.
> > +
> > +       host->cmd_rsp = resp;
> > +
> > +       if ((opcode == SD_IO_RW_DIRECT && cmd->flags == (unsigned int)
> -1) ||
> > +                       opcode == MMC_STOP_TRANSMISSION)
> > +               rawcmd |= (1 << 14);
> > +       else if (opcode == SD_SWITCH_VOLTAGE)
> > +               rawcmd |= (1 << 30);
> > +       else if ((opcode == SD_APP_SEND_SCR) ||
> > +                       (opcode == SD_APP_SEND_NUM_WR_BLKS) ||
> > +                       (opcode == SD_SWITCH &&
> > +                       (mmc_cmd_type(cmd) == MMC_CMD_ADTC))
> ||
> > +                       (opcode == SD_APP_SD_STATUS &&
> > +                       (mmc_cmd_type(cmd) == MMC_CMD_ADTC))
> ||
> > +                       (opcode == MMC_SEND_EXT_CSD &&
> > +                       (mmc_cmd_type(cmd) == MMC_CMD_ADTC)))
> > +               rawcmd |= (1 << 11);
> > +
> > +       if (cmd->data) {
> > +               struct mmc_data *data = cmd->data;
> > +
> > +               if (mmc_op_multi(opcode)) {
> > +                       if (mmc_card_mmc(host->mmc->card) &&
> mrq->sbc &&
> > +                                       !(mrq->sbc->arg &
> 0xFFFF0000))
> > +                               rawcmd |= 2 << 28; /* AutoCMD23 */
> 
> Hex values and lower-case letters in the hex values, to keep consistency...
> 
> I will stop commenting on this from now on, since I realize it applies
> to the complete code. Please go through the code and fix this.
Ok, will fix it.
> 
> > +               }
> > +
> > +               rawcmd |= ((data->blksz & 0xFFF) << 16);
> > +               if (data->flags & MMC_DATA_WRITE)
> > +                       rawcmd |= (1 << 13);
> > +               if (data->blocks > 1)
> > +                       rawcmd |= (2 << 11);
> > +               else
> > +                       rawcmd |= (1 << 11);
> > +               /* Always use dma mode */
> > +               sdr_clr_bits(host->base + MSDC_CFG, MSDC_CFG_PIO);
> > +
> > +               if (((host->timeout_ns != data->timeout_ns) ||
> > +                               (host->timeout_clks !=
> data->timeout_clks))) {
> > +                       msdc_set_timeout(host, data->timeout_ns,
> > +                                       data->timeout_clks);
> > +               }
> > +
> > +               writel(data->blocks, host->base + SDC_BLK_NUM);
> > +       }
> > +       return rawcmd;
> > +}
> > +
> > +static void msdc_start_data(struct msdc_host *host, struct mmc_request
> *mrq,
> > +                           struct mmc_command *cmd, struct
> mmc_data *data)
> > +{
> > +       bool read;
> > +       unsigned long flags;
> > +       u32 wints;
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       if (!host->data) {
> > +               host->data = data;
> > +       } else {
> > +               dev_err(host->dev, "%s: CMD%d [%d blocks] is active",
> > +                               __func__, host->mrq->cmd->opcode,
> > +                               host->data->blocks);
> > +               dev_err(host->dev, "but new CMD%d [%d blocks]
> started\n",
> > +                       cmd->opcode, data->blocks);
> > +       }
> > +       spin_unlock_irqrestore(&host->lock, flags);
> 
> How about replacing the above error check with a single WARN_ON() instead?
> 
> > +       read = (data->flags & MMC_DATA_READ) != 0;
> 
> Simplify to:
> 
> read = data->flags & MMC_DATA_READ;
OK, will fix it at next revision,
> 
> > +       data->error = 0;
> 
> You shouldn't need to reset that, handle by the mmc core.
> 
> > +       if (data->stop)
> > +               data->stop->error = 0;
> 
> You shouldn't need to reset that, handle by the mmc core.
> 
OK, will fix them at next revision,
> > +
> > +       mod_delayed_work(system_wq, &host->req_timeout,
> DAT_TIMEOUT);
> > +       msdc_dma_setup(host, &host->dma, data);
> > +       wints = msdc_data_ints_mask(host);
> > +       sdr_set_bits(host->base + MSDC_INTEN, wints);
> > +       mb(); /* wait for pending IO to finish */
> > +       sdr_set_field(host->base + MSDC_DMA_CTRL,
> MSDC_DMA_CTRL_START, 1);
> > +       dev_dbg(host->dev, "DMA start\n");
> > +       dev_dbg(host->dev, "%s: cmd=%d DMA data: %d blocks;
> read=%d\n",
> > +                       __func__, cmd->opcode, data->blocks, read);
> > +}
> > +
> > +static int msdc_auto_cmd_done(struct msdc_host *host, int events,
> > +               struct mmc_command *cmd)
> > +{
> > +       u32 *rsp = cmd->resp;
> > +
> > +       rsp[0] = readl(host->base + SDC_ACMD_RESP);
> > +
> > +       if (events & MSDC_INT_ACMDRDY) {
> > +               cmd->error = 0;
> > +       } else {
> > +               msdc_reset_hw(host);
> > +               if (events & MSDC_INT_ACMDCRCERR) {
> > +                       cmd->error = (unsigned int) -EILSEQ;
> > +                       host->error |= REQ_STOP_EIO;
> > +               } else if (events & MSDC_INT_ACMDTMO) {
> > +                       cmd->error = (unsigned int) -ETIMEDOUT;
> > +                       host->error |= REQ_STOP_TMO;
> > +               }
> > +               dev_err(host->dev,
> > +                       "%s: AUTO_CMD%d arg=%08X; rsp %08X;
> cmd_error=%d\n",
> > +                       __func__, cmd->opcode, cmd->arg, rsp[0],
> > +                       (int)cmd->error);
> > +       }
> > +       return (int)(cmd->error);
> 
> This is silly. The struct mmc_command|data->error field should be of
> type int and not unsigned int, which isn't the case currently.
> 
> Host driver shouldn't need to deal with this casting. I have just sent
> a patch to change "error" to an int. Please align to that change, in
> all places where you use the "error" field.
> 
OK, will fix it at next revision,
> > +}
> > +
> > +static void msdc_track_cmd_data(struct msdc_host *host,
> > +                               struct mmc_command *cmd, struct
> mmc_data *data)
> > +{
> > +       if (host->error)
> > +               dev_dbg(host->dev, "%s: cmd=%d arg=%08X;
> host->error=0x%08X\n",
> > +                       __func__, cmd->opcode, cmd->arg,
> host->error);
> > +}
> > +
> > +static void msdc_request_done(struct msdc_host *host, struct
> mmc_request *mrq)
> > +{
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       cancel_delayed_work(&host->req_timeout);
> 
> This looks racy.
> 
> Don't you need a cancel_delayed_work_sync() from somewhere, to be sure
> that work isn't preemted after the "host->mrq" has been reset here? Or
> maybe there is another way!? Of course that can't be done with the
> spin_locks held, but I asume you get my point.
Yes, I get your point, actually, we set a 5s timeout for each request, and, if the work already pending(means that some error happens), we do not wait it,
And the work will set the event to timeout and return,
So, maybe a good solution is that need check the return value of the cancel_delayed_work(), if it is pending, directly return, and do not set the host->mrq to 0 to avoid the race condition,
> 
> > +       host->mrq = NULL;
> > +       spin_unlock_irqrestore(&host->lock, flags);
> > +
> > +       msdc_track_cmd_data(host, mrq->cmd, mrq->data);
> > +       if (mrq->data)
> > +               msdc_unprepare_data(host, mrq);
> > +       mmc_request_done(host->mmc, mrq);
> > +}
> > +
> > +/* returns true if command is fully handled; returns false otherwise */
> > +static bool msdc_cmd_done(struct msdc_host *host, int events,
> > +                         struct mmc_request *mrq, struct
> mmc_command *cmd)
> > +{
> > +       bool done = false;
> > +       bool sbc_error;
> > +       unsigned long flags;
> > +       u32 *rsp = cmd->resp;
> > +
> > +       if (mrq->sbc && cmd == mrq->cmd &&
> > +                       (events & (MSDC_INT_ACMDRDY |
> MSDC_INT_ACMDCRCERR
> > +                                  | MSDC_INT_ACMDTMO)))
> > +               msdc_auto_cmd_done(host, events, mrq->sbc);
> > +
> > +       sbc_error = mrq->sbc && mrq->sbc->error;
> > +
> > +       if (!sbc_error && !(events & (MSDC_INT_CMDRDY
> > +                                       | MSDC_INT_RSPCRCERR
> > +                                       | MSDC_INT_CMDTMO)))
> > +               return done;
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       done = !host->cmd;
> > +       host->cmd = NULL;
> > +       spin_unlock_irqrestore(&host->lock, flags);
> > +
> > +       if (done)
> > +               return true;
> > +
> > +       sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_CMDRDY |
> > +                       MSDC_INTEN_RSPCRCERR |
> MSDC_INTEN_CMDTMO |
> > +                       MSDC_INTEN_ACMDRDY |
> MSDC_INTEN_ACMDCRCERR |
> > +                       MSDC_INTEN_ACMDTMO);
> > +       writel(cmd->arg, host->base + SDC_ARG);
> > +
> > +       switch (host->cmd_rsp) {
> > +       case 0:
> > +               break;
> > +       case 2:
> > +               rsp[0] = readl(host->base + SDC_RESP3);
> > +               rsp[1] = readl(host->base + SDC_RESP2);
> > +               rsp[2] = readl(host->base + SDC_RESP1);
> > +               rsp[3] = readl(host->base + SDC_RESP0);
> > +               break;
> > +       default: /* Response types 1, 3, 4, 5, 6, 7(1b) */
> > +               rsp[0] = readl(host->base + SDC_RESP0);
> > +               break;
> > +       }
> > +       if (!sbc_error && !(events & MSDC_INT_CMDRDY)) {
> > +               msdc_reset_hw(host);
> > +               if (events & MSDC_INT_RSPCRCERR) {
> > +                       cmd->error = (unsigned int) -EILSEQ;
> > +                       host->error |= REQ_CMD_EIO;
> > +               } else if (events & MSDC_INT_CMDTMO) {
> > +                       cmd->error = (unsigned int) -ETIMEDOUT;
> > +                       host->error |= REQ_CMD_TMO;
> > +               }
> > +       }
> > +       if (cmd->error)
> > +               dev_dbg(host->dev,
> > +                               "%s: cmd=%d arg=%08X; rsp %08X;
> cmd_error=%d\n",
> > +                               __func__, cmd->opcode, cmd->arg,
> rsp[0],
> > +                               (int)cmd->error);
> > +
> > +       msdc_cmd_next(host, mrq, cmd);
> > +       done = true;
> > +       return done;
> > +}
> > +
> > +static inline bool msdc_cmd_is_ready(struct msdc_host *host,
> > +               struct mmc_request *mrq, struct mmc_command *cmd)
> > +{
> > +       unsigned long tmo = jiffies + msecs_to_jiffies(20);
> > +
> > +       while ((readl(host->base + SDC_STS) & SDC_STS_CMDBUSY)
> > +                       && time_before(jiffies, tmo))
> > +               continue;
> > +
> > +       if (readl(host->base + SDC_STS) & SDC_STS_CMDBUSY) {
> > +               dev_err(host->dev, "CMD bus busy detected\n");
> > +               host->error |= REQ_CMD_BUSY;
> > +               msdc_cmd_done(host, MSDC_INT_CMDTMO, mrq, cmd);
> > +               return false;
> > +       }
> > +
> > +       if (mmc_resp_type(cmd) == MMC_RSP_R1B || cmd->data) {
> > +               /* R1B or with data, should check SDCBUSY */
> > +               while (readl(host->base + SDC_STS) &
> SDC_STS_SDCBUSY)
> > +                       cpu_relax();
> > +       }
> > +       return true;
> > +}
> > +
> > +static void msdc_start_command(struct msdc_host *host,
> > +               struct mmc_request *mrq, struct mmc_command *cmd)
> > +{
> > +       u32 rawcmd;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       if (host->cmd) {
> > +               dev_err(host->dev, "%s: CMD%d is active, but new
> CMD%d is sent\n",
> > +                               __func__, host->cmd->opcode,
> cmd->opcode);
> > +       } else {
> > +               host->cmd = cmd;
> > +       }
> > +       spin_unlock_irqrestore(&host->lock, flags);
> > +
> 
> How about replacing the above error check with a single WARN_ON() instead?
> 
Sure, it is good, will fix it at next revision,
> > +       if (!msdc_cmd_is_ready(host, mrq, cmd))
> > +               return;
> > +
> > +       if ((readl(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_TXCNT) >>
> 16
> > +               || (readl(host->base + MSDC_FIFOCS) &
> MSDC_FIFOCS_RXCNT)) {
> > +               dev_err(host->dev, "TX/RX FIFO non-empty before start of
> IO. Reset\n");
> > +               msdc_reset_hw(host);
> > +       }
> > +
> > +       cmd->error = 0;
> > +       rawcmd = msdc_cmd_prepare_raw_cmd(host, mrq, cmd);
> > +       mod_delayed_work(system_wq, &host->req_timeout,
> DAT_TIMEOUT);
> > +
> > +       sdr_set_bits(host->base + MSDC_INTEN, MSDC_INTEN_CMDRDY |
> > +                       MSDC_INTEN_RSPCRCERR |
> MSDC_INTEN_CMDTMO |
> > +                       MSDC_INTEN_ACMDRDY |
> MSDC_INTEN_ACMDCRCERR |
> > +                       MSDC_INTEN_ACMDTMO);
> > +       writel(cmd->arg, host->base + SDC_ARG);
> > +       writel(rawcmd, host->base + SDC_CMD);
> > +}
> > +
> > +static void msdc_cmd_next(struct msdc_host *host,
> > +               struct mmc_request *mrq, struct mmc_command *cmd)
> > +{
> > +       if (cmd->error || (mrq->sbc && mrq->sbc->error))
> > +               msdc_request_done(host, mrq);
> > +       else if (cmd == mrq->sbc)
> > +               msdc_start_command(host, mrq, mrq->cmd);
> > +       else if (!cmd->data)
> > +               msdc_request_done(host, mrq);
> > +       else
> > +               msdc_start_data(host, mrq, cmd, cmd->data);
> > +}
> > +
> > +static void msdc_ops_request(struct mmc_host *mmc, struct mmc_request
> *mrq)
> > +{
> > +       unsigned long flags;
> > +       struct msdc_host *host = mmc_priv(mmc);
> > +
> > +       host->error = 0;
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       if (!host->mrq) {
> > +               host->mrq = mrq;
> > +       } else {
> > +               dev_err(host->dev,
> > +                       "%s: req [CMD%d] is active, new req [CMD%d]
> is sent\n",
> > +                       __func__, host->mrq->cmd->opcode,
> mrq->cmd->opcode);
> > +       }
> > +       spin_unlock_irqrestore(&host->lock, flags);
> 
> How about replacing the above error check with a single WARN_ON() instead?
OK, will fix it at next revision.
> 
> > +
> > +       if (mrq->data)
> > +               msdc_prepare_data(host, mrq);
> > +
> > +       /* if SBC is required, we have HW option and SW option.
> > +        * if HW option is enabled, and SBC does not have "special" flags,
> > +        * use HW option,  otherwise use SW option
> > +        */
> > +       if (mrq->sbc && (!mmc_card_mmc(mmc->card) ||
> > +           (mrq->sbc->arg & 0xFFFF0000)))
> > +               msdc_start_command(host, mrq, mrq->sbc);
> > +       else
> > +               msdc_start_command(host, mrq, mrq->cmd);
> > +}
> > +
> > +static void msdc_pre_req(struct mmc_host *mmc, struct mmc_request
> *mrq,
> > +               bool is_first_req)
> > +{
> > +       struct msdc_host *host = mmc_priv(mmc);
> > +       struct mmc_data *data = mrq->data;
> > +
> > +       if (!data)
> > +               return;
> > +
> > +       msdc_prepare_data(host, mrq);
> > +       data->host_cookie |= MSDC_ASYNC_FLAG;
> > +}
> > +
> > +static void msdc_post_req(struct mmc_host *mmc, struct mmc_request
> *mrq,
> > +               int err)
> > +{
> > +       struct msdc_host *host = mmc_priv(mmc);
> > +       struct mmc_data *data;
> > +
> > +       data = mrq->data;
> > +       if (!data)
> > +               return;
> > +       if (data->host_cookie) {
> > +               data->host_cookie &= ~MSDC_ASYNC_FLAG;
> > +               msdc_unprepare_data(host, mrq);
> > +       }
> > +}
> > +
> > +static void msdc_data_xfer_next(struct msdc_host *host,
> > +                               struct mmc_request *mrq, struct
> mmc_data *data)
> > +{
> > +       if (mmc_op_multi(mrq->cmd->opcode) && mrq->stop
> && !mrq->stop->error &&
> > +                       (!data->bytes_xfered || !mrq->sbc))
> > +               msdc_start_command(host, mrq, mrq->stop);
> > +       else
> > +               msdc_request_done(host, mrq);
> > +}
> > +
> > +static bool msdc_data_xfer_done(struct msdc_host *host, u32 events,
> > +                               struct mmc_request *mrq, struct
> mmc_data *data)
> > +{
> > +       struct mmc_command *stop = data->stop;
> > +       unsigned long flags;
> > +       bool done;
> > +       u32 wints;
> > +
> > +       bool check_data = (events &
> > +           (MSDC_INT_XFER_COMPL | MSDC_INT_DATCRCERR |
> MSDC_INT_DATTMO
> > +            | MSDC_INT_DMA_BDCSERR | MSDC_INT_DMA_GPDCSERR
> > +            | MSDC_INT_DMA_PROTECT));
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       done = !host->data;
> > +       if (check_data)
> > +               host->data = NULL;
> > +       spin_unlock_irqrestore(&host->lock, flags);
> > +
> > +       if (done)
> > +               return true;
> > +
> > +       if (check_data || (stop && stop->error)) {
> > +               dev_dbg(host->dev, "DMA status: 0x%8X\n",
> > +                               readl(host->base +
> MSDC_DMA_CFG));
> > +               sdr_set_field(host->base + MSDC_DMA_CTRL,
> MSDC_DMA_CTRL_STOP,
> > +                               1);
> > +               while (readl(host->base + MSDC_DMA_CFG) &
> MSDC_DMA_CFG_STS)
> > +                       ;
> > +               mb(); /* wait for pending IO to finish */
> > +               wints = msdc_data_ints_mask(host);
> > +               sdr_clr_bits(host->base + MSDC_INTEN, wints);
> > +               dev_dbg(host->dev, "DMA stop\n");
> > +
> > +               if ((events & MSDC_INT_XFER_COMPL) && (!stop
> || !stop->error)) {
> > +                       data->bytes_xfered = data->blocks *
> data->blksz;
> > +               } else {
> > +                       dev_err(host->dev, "interrupt events: %x\n",
> events);
> > +                       msdc_reset_hw(host);
> > +                       host->error |= REQ_DAT_ERR;
> > +                       data->bytes_xfered = 0;
> > +
> > +                       if (events & MSDC_INT_DATTMO)
> > +                               data->error = (unsigned int)
> -ETIMEDOUT;
> > +
> > +                       dev_err(host->dev, "%s: cmd=%d; blocks=%d",
> > +                               __func__, mrq->cmd->opcode,
> data->blocks);
> > +                       dev_err(host->dev, "data_error=%d
> xfer_size=%d\n",
> > +                                       (int)data->error,
> data->bytes_xfered);
> > +               }
> > +
> > +               msdc_data_xfer_next(host, mrq, data);
> > +               done = true;
> > +       }
> > +       return done;
> > +}
> > +
> > +static void msdc_set_buswidth(struct msdc_host *host, u32 width)
> > +{
> > +       u32 val = readl(host->base + SDC_CFG);
> > +
> > +       val &= ~SDC_CFG_BUSWIDTH;
> > +
> > +       switch (width) {
> > +       default:
> > +       case MMC_BUS_WIDTH_1:
> > +               val |= (MSDC_BUS_1BITS << 16);
> > +               break;
> > +       case MMC_BUS_WIDTH_4:
> > +               val |= (MSDC_BUS_4BITS << 16);
> > +               break;
> > +       case MMC_BUS_WIDTH_8:
> > +               val |= (MSDC_BUS_8BITS << 16);
> > +               break;
> > +       }
> > +
> > +       writel(val, host->base + SDC_CFG);
> > +       dev_dbg(host->dev, "Bus Width = %d", width);
> > +}
> > +
> > +static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios
> *ios)
> > +{
> > +       struct msdc_host *host = mmc_priv(mmc);
> > +       int min_uv, max_uv;
> > +       int ret = 0;
> > +
> > +       if (!IS_ERR(mmc->supply.vqmmc)) {
> > +
> > +               if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> > +                       min_uv = 3300000;
> > +                       max_uv = 3300000;
> > +               } else if (ios->signal_voltage ==
> MMC_SIGNAL_VOLTAGE_180) {
> > +                       min_uv = 1800000;
> > +                       max_uv = 1800000;
> > +               } else {
> > +                       dev_err(host->dev, "Unsupported signal
> voltage!\n");
> > +                       return -EINVAL;
> > +               }
> > +
> > +               ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv,
> max_uv);
> > +               if (ret) {
> > +                       dev_err(host->dev,
> > +                                       "Regulator set error %d: %d
> - %d\n",
> > +                                       ret, min_uv, max_uv);
> > +               }
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static void msdc_request_timeout(struct work_struct *work)
> > +{
> > +       struct msdc_host *host = container_of(work, struct msdc_host,
> > +                       req_timeout.work);
> > +       unsigned long flags;
> > +       struct mmc_request *mrq;
> > +       struct mmc_command *cmd;
> > +       struct mmc_data *data;
> > +
> > +       spin_lock_irqsave(&host->lock, flags);
> > +       mrq = ACCESS_ONCE(host->mrq);
> > +       cmd = ACCESS_ONCE(host->cmd);
> > +       data = ACCESS_ONCE(host->data);
> 
> Why do you need the ACCESS_ONCE here. Aren't these pointers already
> protected by the spin_lock?
> OK, will remove the ACCESS_ONCE.
> > +       spin_unlock_irqrestore(&host->lock, flags);
> > +
> > +       /* simulate HW timeout status */
> > +       dev_err(host->dev, "%s: aborting cmd/data/mrq\n", __func__);
> > +       if (mrq) {
> > +               dev_err(host->dev, "%s: aborting mrq=%p cmd=%d\n",
> __func__,
> > +                       mrq, mrq->cmd->opcode);
> > +               if (cmd) {
> > +                       dev_err(host->dev, "%s: aborting cmd=%d\n",
> > +                                       __func__, cmd->opcode);
> > +                       msdc_cmd_done(host, MSDC_INT_CMDTMO,
> mrq, cmd);
> > +               } else if (data) {
> > +                       dev_err(host->dev, "%s: abort data: cmd%d; %d
> blocks\n",
> > +                               __func__, mrq->cmd->opcode,
> data->blocks);
> > +                       msdc_data_xfer_done(host,
> MSDC_INT_DATTMO, mrq, data);
> > +               }
> > +       }
> > +}
> > +
> > +static irqreturn_t msdc_irq(int irq, void *dev_id)
> > +{
> > +       struct msdc_host *host = (struct msdc_host *) dev_id;
> > +
> > +       while (true) {
> > +               unsigned long flags;
> > +               struct mmc_request *mrq;
> > +               struct mmc_command *cmd;
> > +               struct mmc_data *data;
> > +               u32 events, event_mask;
> > +
> > +               spin_lock_irqsave(&host->lock, flags);
> > +               events = readl(host->base + MSDC_INT);
> > +               event_mask = readl(host->base + MSDC_INTEN);
> > +               /* clear interrupts */
> > +               writel(events, host->base + MSDC_INT);
> > +
> > +               mrq = ACCESS_ONCE(host->mrq);
> > +               cmd = ACCESS_ONCE(host->cmd);
> > +               data = ACCESS_ONCE(host->data);
> 
> Why do you need the ACCESS_ONCE here. Aren't these pointers already
> protected by the spin_lock?
Will fix it at next revision.
> 
> > +               spin_unlock_irqrestore(&host->lock, flags);
> > +
> > +               if (!(events & event_mask))
> > +                       break;
> > +
> > +               if (!mrq) {
> > +                       dev_err(host->dev,
> > +                               "%s: MRQ=NULL; events=%08X;
> event_mask=%08X\n",
> > +                               __func__, events, event_mask);
> > +                       WARN_ON(1);
> > +                       break;
> > +               }
> > +
> > +               dev_dbg(host->dev, "%s: events=%08X\n", __func__,
> events);
> > +
> > +               if (cmd)
> > +                       msdc_cmd_done(host, events, mrq, cmd);
> > +               else if (data)
> > +                       msdc_data_xfer_done(host, events, mrq,
> data);
> > +       }
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static void msdc_init_hw(struct msdc_host *host)
> > +{
> > +       u32 val;
> > +       /* Configure to MMC/SD mode */
> > +       sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_MODE,
> MSDC_SDMMC);
> > +
> > +       /* Reset */
> > +       msdc_reset_hw(host);
> > +
> > +       /* Disable card detection */
> > +       sdr_clr_bits(host->base + MSDC_PS, MSDC_PS_CDEN);
> > +
> > +       /* Disable and clear all interrupts */
> > +       writel(0, host->base + MSDC_INTEN);
> > +       val = readl(host->base + MSDC_INT);
> > +       writel(val, host->base + MSDC_INT);
> > +
> > +       writel(0, host->base + MSDC_PAD_TUNE);
> > +       writel(0, host->base + MSDC_IOCON);
> > +       sdr_set_field(host->base + MSDC_IOCON, MSDC_IOCON_DDLSEL,
> 1);
> > +       writel(0x403c004f, host->base + MSDC_PATCH_BIT);
> > +       sdr_set_field(host->base + MSDC_PATCH_BIT,
> MSDC_CKGEN_MSDC_DLY_SEL, 1);
> > +       writel(0xffff0089, host->base + MSDC_PATCH_BIT1);
> > +       /* Configure to enable SDIO mode.
> > +          it's must otherwise sdio cmd5 failed */
> > +       sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIO);
> > +
> > +       /* disable detect SDIO device interrupt function */
> > +       sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> > +
> > +       /* Configure to default data timeout */
> > +       sdr_set_field(host->base + SDC_CFG, SDC_CFG_DTOC, 3);
> > +       msdc_set_buswidth(host, MMC_BUS_WIDTH_1);
> > +
> > +       dev_dbg(host->dev, "init hardware done!");
> > +}
> > +
> > +static void msdc_deinit_hw(struct msdc_host *host)
> > +{
> > +       u32 val;
> > +
> > +       /* Disable and clear all interrupts */
> > +       writel(0, host->base + MSDC_INTEN);
> > +
> > +       val = readl(host->base + MSDC_INT);
> > +       writel(val, host->base + MSDC_INT);
> > +
> > +       clk_disable(host->src_clk);
> > +       clk_unprepare(host->src_clk);
> 
> Please use clk_disable_unprepare() instead.
> 
OK, will fix it.
> > +}
> > +
> > +/* init gpd and bd list in msdc_drv_probe */
> > +static void msdc_init_gpd_bd(struct msdc_host *host, struct msdc_dma
> *dma)
> > +{
> > +       struct mt_gpdma_desc *gpd = dma->gpd;
> > +       struct mt_bdma_desc *bd = dma->bd;
> > +       int i;
> > +
> > +       memset(gpd, 0, sizeof(struct mt_gpdma_desc) * MAX_GPD_NUM);
> > +       gpd->next = (u32)dma->gpd_addr + sizeof(struct mt_gpdma_desc);
> > +
> > +       gpd->first_u32 |= GPDMA_DESC_BDP; /* hwo, cs, bd pointer */
> > +       gpd->ptr = (u32)dma->bd_addr; /* physical address */
> > +
> > +       memset(bd, 0, sizeof(struct mt_bdma_desc) *
> MAX_BD_PER_GPD);
> > +
> > +       for (i = 0; i < (MAX_BD_PER_GPD - 1); i++)
> > +               bd[i].next = (u32)dma->bd_addr + sizeof(*bd) * (i + 1);
> > +}
> > +
> > +static int timing_is_uhs(unsigned char timing)
> > +{
> > +       if (timing != MMC_TIMING_LEGACY &&
> > +                       timing != MMC_TIMING_MMC_HS &&
> > +                       timing != MMC_TIMING_SD_HS)
> 
> I don't like these assumtions. There may be other cases which isn't
> covered here that isn't UHS.
> 
Actually, I want set different driving by 3.3V/1.8V signal.
So, will replace it ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180 and rename the function name.
> > +               return 1;
> > +       return 0;
> > +}
> > +
> > +static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > +{
> > +       struct msdc_host *host = mmc_priv(mmc);
> > +       int ret;
> > +       u32 ddr = 0;
> > +
> > +       if (ios->timing == MMC_TIMING_UHS_DDR50)
> > +               ddr = 1;
> 
> I am not sure if this controller supports eMMC cards as well, but if
> that's the case and it also supports DDR you may also want to check
> for MMC_TIMING_MMC_DDR52.
> 
Will add it at next revision.
> > +
> > +       msdc_set_buswidth(host, ios->bus_width);
> > +
> > +       /* Suspend/Resume will do power off/on */
> > +       switch (ios->power_mode) {
> > +       case MMC_POWER_UP:
> > +               msdc_init_hw(host);
> > +               if (!IS_ERR(mmc->supply.vmmc)) {
> > +                       ret = mmc_regulator_set_ocr(mmc,
> mmc->supply.vmmc,
> > +                                       ios->vdd);
> > +                       if (ret) {
> > +                               dev_err(host->dev, "Failed to set
> vmmc power!\n");
> > +                               return;
> > +                       }
> > +               }
> > +               break;
> > +       case MMC_POWER_ON:
> > +               if (!IS_ERR(mmc->supply.vqmmc)) {
> > +                       ret = regulator_enable(mmc->supply.vqmmc);
> 
> The calls to regulator_enable|disable() for the vqmmc will not be
> balanced properly here. You need a local cache variable like
> "is_enabled" to keep track of this.
> 
OK, will fix it at next revision,
> > +                       if (ret)
> > +                               dev_err(host->dev, "Failed to set
> vqmmc power!\n");
> > +               }
> > +               break;
> > +       case MMC_POWER_OFF:
> > +               if (!IS_ERR(mmc->supply.vmmc))
> > +                       mmc_regulator_set_ocr(mmc,
> mmc->supply.vmmc, 0);
> > +
> > +               if (!IS_ERR(mmc->supply.vqmmc))
> > +                       regulator_disable(mmc->supply.vqmmc);
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +
> > +       /* Apply different pinctrl settings for different timing */
> > +       if (timing_is_uhs(ios->timing))
> > +               pinctrl_select_state(host->pinctrl, host->pins_uhs);
> > +       else
> > +               pinctrl_select_state(host->pinctrl, host->pins_default);
> > +
> > +       if (host->mclk != ios->clock || host->ddr != ddr)
> > +               msdc_set_mclk(host, ddr, ios->clock);
> > +}
> > +
> > +static struct mmc_host_ops mt_msdc_ops = {
> > +       .post_req = msdc_post_req,
> > +       .pre_req = msdc_pre_req,
> > +       .request = msdc_ops_request,
> > +       .set_ios = msdc_ops_set_ios,
> > +       .start_signal_voltage_switch = msdc_ops_switch_volt,
> > +};
> > +
> > +static int msdc_drv_probe(struct platform_device *pdev)
> > +{
> > +       struct mmc_host *mmc;
> > +       struct msdc_host *host;
> > +       struct resource *res;
> > +       int ret;
> > +
> > +       if (!pdev->dev.of_node) {
> > +               dev_err(&pdev->dev, "No DT found\n");
> > +               return -EINVAL;
> > +       }
> > +       /* Allocate MMC host for this device */
> > +       mmc = mmc_alloc_host(sizeof(struct msdc_host), &pdev->dev);
> > +       if (!mmc)
> > +               return -ENOMEM;
> > +
> > +       host = mmc_priv(mmc);
> > +       ret = mmc_of_parse(mmc);
> > +       if (ret)
> > +               goto host_free;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       host->base = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(host->base)) {
> > +               ret = PTR_ERR(host->base);
> > +               goto host_free;
> > +       }
> > +
> > +       ret = mmc_regulator_get_supply(mmc);
> > +       if (ret == -EPROBE_DEFER)
> > +               goto host_free;
> > +
> > +       host->src_clk = devm_clk_get(&pdev->dev, "source");
> > +       if (IS_ERR(host->src_clk)) {
> > +               dev_err(&pdev->dev,
> > +                               "Invalid source clock from the device
> tree!\n");
> 
> You can remove this print, it's already handled by the common clk framework.
Ok, will remove it at next revision,
> 
> > +               ret = PTR_ERR(host->src_clk);
> > +               goto host_free;
> > +       }
> > +
> > +       host->irq = platform_get_irq(pdev, 0);
> > +       if (host->irq < 0) {
> > +               ret = -EINVAL;
> > +               goto host_free;
> > +       }
> > +
> > +       host->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +       if (IS_ERR(host->pinctrl)) {
> > +               ret = PTR_ERR(host->pinctrl);
> > +               dev_err(&pdev->dev, "Cannot find pinctrl!\n");
> > +               goto host_free;
> > +       }
> > +
> > +       host->pins_default = pinctrl_lookup_state(host->pinctrl, "default");
> > +       if (IS_ERR(host->pins_default)) {
> > +               ret = PTR_ERR(host->pins_default);
> > +               dev_err(&pdev->dev, "Cannot find pinctrl default!\n");
> > +               goto host_free;
> > +       }
> > +
> > +       host->pins_uhs = pinctrl_lookup_state(host->pinctrl, "state_uhs");
> > +       if (IS_ERR(host->pins_uhs)) {
> > +               ret = PTR_ERR(host->pins_uhs);
> > +               dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> > +               goto host_free;
> > +       }
> > +
> > +       host->dev = &pdev->dev;
> > +       host->mmc = mmc;
> > +       /* Set host parameters to mmc */
> > +       mmc->ops = &mt_msdc_ops;
> > +       mmc->f_min = 260000;
> 
> Doesn't f_min depend on some internal divider algorithm and the clock
> rate? So, then it should maybe be calculated instead of fixed?
> 
Yes, the f_min is the source_clock / (4 * 255);
> > +       mmc->ocr_avail = MMC_VDD_30_31 | MMC_VDD_31_32 |
> MMC_VDD_32_33;
> 
> "ocr_avail" should be fetched from the vmmc regulator, when you invoke
> mmc_regulator_get_supply() above.
Yes, I also want to remove it, but, Not all devices use regulator, our SDIO device which connected on MSDC3, it's power is controlled by gpio.
This because the regulator of the PMU is not enough.
> 
> > +
> > +       mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
> > +       /* MMC core transfer sizes tunable parameters */
> > +       mmc->max_segs = MAX_BD_NUM;
> > +       mmc->max_seg_size = 64 * 1024;
> > +       mmc->max_blk_size = 2048;
> > +       mmc->max_req_size = 512 * 1024;
> > +       mmc->max_blk_count = mmc->max_req_size;
> > +
> > +       host->hclk = clk_get_rate(host->src_clk);
> > +       host->timeout_clks = 3 * 1048576;
> > +       host->dma.gpd = dma_alloc_coherent(&pdev->dev,
> > +                               MAX_GPD_NUM * sizeof(struct
> mt_gpdma_desc),
> > +                               &host->dma.gpd_addr,
> GFP_KERNEL);
> > +       host->dma.bd = dma_alloc_coherent(&pdev->dev,
> > +                               MAX_BD_NUM * sizeof(struct
> mt_bdma_desc),
> > +                               &host->dma.bd_addr, GFP_KERNEL);
> > +       if ((!host->dma.gpd) || (!host->dma.bd)) {
> > +               ret = -ENOMEM;
> > +               goto release_mem;
> > +       }
> > +       msdc_init_gpd_bd(host, &host->dma);
> > +       INIT_DELAYED_WORK(&host->req_timeout,
> msdc_request_timeout);
> > +       spin_lock_init(&host->lock);
> > +
> > +       platform_set_drvdata(pdev, mmc);
> > +       clk_prepare(host->src_clk);
> 
> This should be clk_prepare_enable().
> 
I will remove the clk_prepare here and use clk_prepare_enable() at msdc_ungate_clock();
> > +
> > +       ret = devm_request_irq(&pdev->dev, (unsigned int) host->irq,
> msdc_irq,
> > +               IRQF_TRIGGER_LOW | IRQF_ONESHOT, pdev->name,
> host);
> > +       if (ret)
> > +               goto release;
> > +
> > +       ret = mmc_add_host(mmc);
> > +       if (ret)
> > +               goto release;
> > +
> > +       return 0;
> > +
> > +release:
> > +       platform_set_drvdata(pdev, NULL);
> > +       msdc_deinit_hw(host);
> > +release_mem:
> > +       if (host->dma.gpd)
> > +               dma_free_coherent(&pdev->dev,
> > +                       MAX_GPD_NUM * sizeof(struct
> mt_gpdma_desc),
> > +                       host->dma.gpd, host->dma.gpd_addr);
> > +       if (host->dma.bd)
> > +               dma_free_coherent(&pdev->dev,
> > +                       MAX_BD_NUM * sizeof(struct mt_bdma_desc),
> > +                       host->dma.bd, host->dma.bd_addr);
> > +host_free:
> > +       mmc_free_host(mmc);
> > +
> > +       return ret;
> > +}
> > +
> > +static int msdc_drv_remove(struct platform_device *pdev)
> > +{
> > +       struct mmc_host *mmc;
> > +       struct msdc_host *host;
> > +
> > +       mmc = platform_get_drvdata(pdev);
> > +       host = mmc_priv(mmc);
> > +
> > +       platform_set_drvdata(pdev, NULL);
> > +       mmc_remove_host(host->mmc);
> > +       msdc_deinit_hw(host);
> > +
> > +       dma_free_coherent(&pdev->dev,
> > +                       MAX_GPD_NUM * sizeof(struct
> mt_gpdma_desc),
> > +                       host->dma.gpd, host->dma.gpd_addr);
> > +       dma_free_coherent(&pdev->dev, MAX_BD_NUM * sizeof(struct
> mt_bdma_desc),
> > +                       host->dma.bd, host->dma.bd_addr);
> > +
> > +       mmc_free_host(host->mmc);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id msdc_of_ids[] = {
> > +       {   .compatible = "mediatek,mt8135-mmc", },
> > +       {}
> > +};
> > +
> > +static struct platform_driver mt_msdc_driver = {
> > +       .probe = msdc_drv_probe,
> > +       .remove = msdc_drv_remove,
> > +       .driver = {
> > +               .name = "mtk-msdc",
> > +               .of_match_table = msdc_of_ids,
> > +       },
> > +};
> > +
> > +module_platform_driver(mt_msdc_driver);
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MediaTek SD/MMC Card Driver");
> > --
> > 1.8.1.1.dirty
> >
> 
> Kind regards
> Uffe

************* Email Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: 答复: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver

[Ulf Hansson]

[...]

>> > +
>> > +struct mt_bdma_desc {
>> > +       u32 first_u32;
>> > +#define BDMA_DESC_EOL          (1 << 0)
>> > +#define BDMA_DESC_CHECKSUM     (0xff << 8) /* bit8 ~ bit15 */
>> > +#define BDMA_DESC_BLKPAD       (1 << 17)
>> > +#define BDMA_DESC_DWPAD                (1 << 18)
>> > +       u32 next;
>> > +       u32 ptr;
>> > +       u32 second_u32;
>> > +#define BDMA_DESC_BUFLEN       (0xffff) /* bit0 ~ bit15 */
>> > +};
>> > +
>> > +struct msdc_dma {
>> > +       struct scatterlist *sg; /* I/O scatter list */
>> > +       struct mt_gpdma_desc *gpd;              /* pointer to gpd
>> array */
>> > +       struct mt_bdma_desc *bd;                /* pointer to bd
>> array */
>> > +       dma_addr_t gpd_addr;    /* the physical address of gpd array */
>> > +       dma_addr_t bd_addr;     /* the physical address of bd array */
>> > +};
>>
>> This looks weird from DMA perspective. Can you elaborate on why you
>> can't use the dmaengine API?
>>
> The gpd and bd structure are determined by the MSDC hw, and, the DMA controller is a part of the MSDC hw, different with the chain dma implemented by the kernel.

Hmm. I haven't reviewed the DMA related parts in detail. I will do
that when you have sent the next version.

>> > +
>> > +struct msdc_host {
>> > +       struct device *dev;
>> > +       struct mmc_host *mmc;   /* mmc structure */
>> > +       int cmd_rsp;
>> > +
>> > +       spinlock_t lock;
>> > +       struct mmc_request *mrq;
>> > +       struct mmc_command *cmd;
>> > +       struct mmc_data *data;
>> > +       int error;
>> > +
>> > +       void __iomem *base;             /* host base address */
>> > +
>> > +       struct msdc_dma dma;    /* dma channel */
>> > +
>> > +       u32 timeout_ns;         /* data timeout ns */
>> > +       u32 timeout_clks;       /* data timeout clks */
>> > +
>> > +       struct pinctrl *pinctrl;
>> > +       struct pinctrl_state *pins_default;
>> > +       struct pinctrl_state *pins_uhs;
>> > +       struct delayed_work req_timeout;
>> > +       int irq;                /* host interrupt */
>> > +
>> > +       struct clk *src_clk;    /* msdc source clock */
>> > +       u32 mclk;               /* mmc subsystem clock */
>> > +       u32 hclk;               /* host clock speed */
>> > +       u32 sclk;               /* SD/MS clock speed */
>> > +       bool ddr;
>> > +};
>> > +
>> > +static void sdr_set_bits(void __iomem *reg, u32 bs)
>> > +{
>> > +       u32 val = readl(reg);
>> > +
>> > +       val |= bs;
>> > +       writel(val, reg);
>> > +}
>> > +
>> > +static void sdr_clr_bits(void __iomem *reg, u32 bs)
>> > +{
>> > +       u32 val = readl(reg);
>> > +
>> > +       val &= ~(u32)bs;
>> > +       writel(val, reg);
>> > +}
>> > +
>> > +static void sdr_set_field(void __iomem *reg, u32 field, u32 val)
>> > +{
>> > +       unsigned int tv = readl(reg);
>> > +
>> > +       tv &= ~field;
>> > +       tv |= ((val) << (ffs((unsigned int)field) - 1));
>> > +       writel(tv, reg);
>> > +}
>>
>> A common thought for all the three above functions:
>>
>> Have you considered using a cache variable for those registers that
>> often gets updated? In that way you would have to read the register
>> value every time when you want to write to it. It should improve
>> performance a bit.
>>
> These register can be modified by the MSDC hw, cannot cache it.

Is that true for all registers?

Anyway, let's leave this as is.

>> > +
>> > +static void sdr_get_field(void __iomem *reg, u32 field, u32 *val)
>> > +{
>> > +       unsigned int tv = readl(reg);
>> > +
>> > +       *val = ((tv & field) >> (ffs((unsigned int)field) - 1));
>> > +}
>> > +
>> > +static void msdc_reset_hw(struct msdc_host *host)
>> > +{
>> > +       u32 val;
>> > +
>> > +       sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_RST);
>> > +       while (readl(host->base + MSDC_CFG) & MSDC_CFG_RST)
>> > +               cpu_relax();
>> > +
>> > +       sdr_set_bits(host->base + MSDC_FIFOCS, MSDC_FIFOCS_CLR);
>> > +       while (readl(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_CLR)
>> > +               cpu_relax();
>> > +
>> > +       val = readl(host->base + MSDC_INT);
>> > +       writel(val, host->base + MSDC_INT);
>> > +}
>> > +
>> > +static void msdc_cmd_next(struct msdc_host *host,
>> > +               struct mmc_request *mrq, struct mmc_command *cmd);
>>
>> Please investigate whether you can move around the code to prevent
>> this static declaration of the function.
>>
> It is hard to do it, because the first cmd may be CMD23, and do the next is send the mrq->cmd, so, there is a loop here, at least on method need do static declaration,

Okay.

[...]

>> > +/* clock control primitives */
>> > +static void msdc_set_timeout(struct msdc_host *host, u32 ns, u32 clks)
>> > +{
>> > +       u32 timeout, clk_ns;
>> > +       u32 mode = 0;
>> > +
>> > +       host->timeout_ns = ns;
>> > +       host->timeout_clks = clks;
>> > +       if (host->sclk == 0) {
>> > +               timeout = 0;
>> > +       } else {
>> > +               clk_ns  = 1000000000UL / host->sclk;
>> > +               timeout = (ns + clk_ns - 1) / clk_ns + clks;
>> > +               /* in 1048576 sclk cycle unit */
>> > +               timeout = (timeout + (1 << 20) - 1) >> 20;
>> > +               sdr_get_field(host->base + MSDC_CFG,
>> MSDC_CFG_CKMOD, &mode);
>> > +               /*DDR mode will double the clk cycles for data timeout */
>>
>> How do you know you will be using DDR at this point? Don't you need to
>> check for that?
>>
> The MSDC_CFG_CKMODE can show current mode(DDR or SDR).

Ah, thanks. Got it.

[...]

>> > +static void msdc_request_done(struct msdc_host *host, struct
>> mmc_request *mrq)
>> > +{
>> > +       unsigned long flags;
>> > +
>> > +       spin_lock_irqsave(&host->lock, flags);
>> > +       cancel_delayed_work(&host->req_timeout);
>>
>> This looks racy.
>>
>> Don't you need a cancel_delayed_work_sync() from somewhere, to be sure
>> that work isn't preemted after the "host->mrq" has been reset here? Or
>> maybe there is another way!? Of course that can't be done with the
>> spin_locks held, but I asume you get my point.
> Yes, I get your point, actually, we set a 5s timeout for each request, and, if the work already pending(means that some error happens), we do not wait it,
> And the work will set the event to timeout and return,
> So, maybe a good solution is that need check the return value of the cancel_delayed_work(), if it is pending, directly return, and do not set the host->mrq to 0 to avoid the race condition,

That should work.

[...]

>>
>> "ocr_avail" should be fetched from the vmmc regulator, when you invoke
>> mmc_regulator_get_supply() above.
> Yes, I also want to remove it, but, Not all devices use regulator, our SDIO device which connected on MSDC3, it's power is controlled by gpio.
> This because the regulator of the PMU is not enough.

So could you perhaps use a "gpio regulator" for this GPIO pin? There
is already support to easily describe such in DT.

Or it is a "reset GPIO" you are talking about?

[...]

Kind regards
Uffe

Re: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver

[Sascha Hauer]

Ulf,

On Tue, Mar 31, 2015 at 02:23:06PM +0200, Ulf Hansson wrote:
> On 17 March 2015 at 04:13, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> > +
> > +       msdc_set_buswidth(host, ios->bus_width);
> > +
> > +       /* Suspend/Resume will do power off/on */
> > +       switch (ios->power_mode) {
> > +       case MMC_POWER_UP:
> > +               msdc_init_hw(host);
> > +               if (!IS_ERR(mmc->supply.vmmc)) {
> > +                       ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> > +                                       ios->vdd);
> > +                       if (ret) {
> > +                               dev_err(host->dev, "Failed to set vmmc power!\n");
> > +                               return;
> > +                       }
> > +               }
> > +               break;
> > +       case MMC_POWER_ON:
> > +               if (!IS_ERR(mmc->supply.vqmmc)) {
> > +                       ret = regulator_enable(mmc->supply.vqmmc);
> 
> The calls to regulator_enable|disable() for the vqmmc will not be
> balanced properly here. You need a local cache variable like
> "is_enabled" to keep track of this.

Shouldn't the MMC core provide balanced hooks for this? What about
MMC_POWER_UP, can this be used for enabling regulators?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver

[Ulf Hansson]

On 17 April 2015 at 11:12, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Ulf,
>
> On Tue, Mar 31, 2015 at 02:23:06PM +0200, Ulf Hansson wrote:
>> On 17 March 2015 at 04:13, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>> > +
>> > +       msdc_set_buswidth(host, ios->bus_width);
>> > +
>> > +       /* Suspend/Resume will do power off/on */
>> > +       switch (ios->power_mode) {
>> > +       case MMC_POWER_UP:
>> > +               msdc_init_hw(host);
>> > +               if (!IS_ERR(mmc->supply.vmmc)) {
>> > +                       ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
>> > +                                       ios->vdd);
>> > +                       if (ret) {
>> > +                               dev_err(host->dev, "Failed to set vmmc power!\n");
>> > +                               return;
>> > +                       }
>> > +               }
>> > +               break;
>> > +       case MMC_POWER_ON:
>> > +               if (!IS_ERR(mmc->supply.vqmmc)) {
>> > +                       ret = regulator_enable(mmc->supply.vqmmc);
>>
>> The calls to regulator_enable|disable() for the vqmmc will not be
>> balanced properly here. You need a local cache variable like
>> "is_enabled" to keep track of this.
>
> Shouldn't the MMC core provide balanced hooks for this? What about
> MMC_POWER_UP, can this be used for enabling regulators?

Currently host drivers deals with vqmmc enabling/disabling, setting
voltage, etc - entirely by them selves.

Dough Anderson is working on fixing that:
http://lkml.iu.edu/hypermail/linux/kernel/1503.1/03108.html

MMC_POWER_UP is when vmmc shall be enabled.

MMC_POWER_ON is when vqmmc shall be enabled.

Kind regards
Uffe

Re: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver

[Matthias Brugger]

2015-03-17 4:13 GMT+01:00 Chaotian Jing <chaotian.jing@mediatek.com>:
> Add Mediatek MMC driver code
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/host/Kconfig  |    8 +
>  drivers/mmc/host/Makefile |    1 +
>  drivers/mmc/host/mtk-sd.c | 1412 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1421 insertions(+)
>  create mode 100644 drivers/mmc/host/mtk-sd.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 61ac63a..8b64377 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -768,3 +768,11 @@ config MMC_TOSHIBA_PCI
>         tristate "Toshiba Type A SD/MMC Card Interface Driver"
>         depends on PCI
>         help
> +
> +config MMC_MTK
> +       tristate "MediaTek SD/MMC Card Interface support"
> +       help
> +         This selects the MediaTek(R) Secure digital and Multimedia card Interface.
> +         If you have a machine with a integrated SD/MMC card reader, say Y or M here.
> +         This is needed if support for any SD/SDIO/MMC devices is required.
> +         If unsure, say N.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 6a7cfe0..161b3e4 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_MMC_SDHCI_F_SDH30)       += sdhci_f_sdh30.o
>  obj-$(CONFIG_MMC_SDHCI_SPEAR)  += sdhci-spear.o
>  obj-$(CONFIG_MMC_WBSD)         += wbsd.o
>  obj-$(CONFIG_MMC_AU1X)         += au1xmmc.o
> +obj-$(CONFIG_MMC_MTK)           += mtk-sd.o
>  obj-$(CONFIG_MMC_OMAP)         += omap.o
>  obj-$(CONFIG_MMC_OMAP_HS)      += omap_hsmmc.o
>  obj-$(CONFIG_MMC_ATMELMCI)     += atmel-mci.o
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> new file mode 100644
> index 0000000..86c999b
> --- /dev/null
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -0,0 +1,1412 @@
> +/*
> + * Copyright (c) 2014-2015 MediaTek Inc.
> + * Author: Chaotian.Jing <chaotian.jing@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/ioport.h>
> +#include <linux/irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spinlock.h>
> +
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/core.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/sdio.h>
> +
> +#define MAX_GPD_NUM         (1 + 1)    /* one null gpd */
> +#define MAX_BD_NUM          1024
> +#define MAX_BD_PER_GPD      MAX_BD_NUM

This looks strange.
Why don't you define MAX_GPD_NUM as two?
Why do you define MAX_BD_PER_GPD when it is the same as MAX_BD_NUM?

Cheers,
Matthias

-- 
motzblog.wordpress.com

Re: [PATCH v2 2/5] mmc: mediatek: Add Mediatek MMC driver

[Sascha Hauer]

Hi Chaotian,

On Tue, Mar 17, 2015 at 11:13:52AM +0800, Chaotian Jing wrote:
> Add Mediatek MMC driver code
> 

[...]

> +	host->dev = &pdev->dev;
> +	host->mmc = mmc;
> +	/* Set host parameters to mmc */
> +	mmc->ops = &mt_msdc_ops;
> +	mmc->f_min = 260000;
> +	mmc->ocr_avail = MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33;
> +
> +	mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
> +	/* MMC core transfer sizes tunable parameters */
> +	mmc->max_segs = MAX_BD_NUM;
> +	mmc->max_seg_size = 64 * 1024;

Earlier versions of this driver had 64 * 1024 - 512 here. Using 64k
breaks SD card support here. Reading the partition table still works
fine, but the next transfers fail with messages like:

mtk-msdc 11240000.mmc: msdc_request_timeout: aborting cmd/data/mrq
mtk-msdc 11240000.mmc: msdc_request_timeout: aborting mrq=ffffffc06e586860 cmd=18
mtk-msdc 11240000.mmc: msdc_request_timeout: abort data: cmd18; 136 blocks
mmcblk1: error -110 transferring data, sector 338488, nr 136, cmd response 0x900, card status 0xb00
mtk-msdc 11240000.mmc: set mclk to 0 mmc1: tried to reset card

Can you confirm this?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/5] mmc: dt-bindings: add Mediatek MMC bindings

[Sascha Hauer]

Hi Chaotian,

On Tue, Mar 17, 2015 at 11:13:51AM +0800, Chaotian Jing wrote:
> Document the device-tree binding of Mediatek MMC host
> 
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mmc/mtk-sd.txt | 33 ++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/mtk-sd.txt
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.txt b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> new file mode 100644
> index 0000000..ac43f77
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> @@ -0,0 +1,33 @@
> +* MTK MMC controller
> +
> +The MTK  MSDC can act as a MMC controller
> +to support MMC, SD, and SDIO types of memory cards.
> +
> +This file documents differences between the core properties in mmc.txt
> +and the properties used by the msdc driver.
> +
> +Required properties:
> +- compatible: Should be "mediatek,mt8173-mmc","mediatek,mt8135-mmc".
> +- interrupts: Should contain MSDC interrupt number
> +- clocks: MSDC source clock
> +- clock-names: "source"

When I remove the clk_ignore_unused from the command line SD/MMC stops
working on the MT8173. This is because msdc50_0_h_sel is disabled, when
I enable this clock SD/MMC works again. This clock should probably be
added here and handled appropriatly in the driver.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |