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

From: Ulf Hansson
Date: Tue Mar 31 2015 - 10:46:50 EST

Next message: David Ahern: "[PATCH] perf sched: Make cpu based arrays dynamic"
Previous message: David Ahern: "[PATCH] perf sched: Remove max pid assumption from perf-sched"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 17 March 2015 at 04:13, Chaotian Jing <chaotian.jing@xxxxxxxxxxxx> wrote:
> Add PM support for Mediatek MMC driver
>
> Signed-off-by: Chaotian Jing <chaotian.jing@xxxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: David Ahern: "[PATCH] perf sched: Make cpu based arrays dynamic"
Previous message: David Ahern: "[PATCH] perf sched: Remove max pid assumption from perf-sched"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]