Re: [PATCH v3 6/7] ARM: mediatek: dts: Add emmc support to mt8135

Re: [PATCH v3 6/7] ARM: mediatek: dts: Add emmc support to mt8135

[Sascha Hauer]

On Tue, Apr 28, 2015 at 05:48:44PM +0800, Chaotian Jing wrote:
> From: Yingjoe Chen <yingjoe.chen@mediatek.com>
> 
> Add emmc node for mt8135.dtsi & mt8135-evbp1.dts
> 
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  arch/arm/boot/dts/mt8135-evbp1.dts | 141 +++++++++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/mt8135.dtsi      |  26 +++++++
>  2 files changed, 167 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/mt8135-evbp1.dts b/arch/arm/boot/dts/mt8135-evbp1.dts
> index 357a91f..6d2fee9 100644
> --- a/arch/arm/boot/dts/mt8135-evbp1.dts
> +++ b/arch/arm/boot/dts/mt8135-evbp1.dts
> @@ -24,6 +24,147 @@
>  	};
>  };
>  
> +&mmc0 {
> +	pinctrl-names = "default", "state_uhs";
> +	pinctrl-0 = <&mmc0_pins_default>;
> +	pinctrl-1 = <&mmc0_pins_uhs>;
> +	status = "okay";
> +	bus-width = <8>;
> +	max-frequency = <50000000>;
> +	cap-mmc-highspeed;
> +	vmmc-supply = <&mt6397_vemc_3v3_reg>;
> +	non-removable;
> +	func = "EMMC";

...

> +	func = "SDCARD";

These have been removed in earlier versions, right?

> diff --git a/arch/arm/boot/dts/mt8135.dtsi b/arch/arm/boot/dts/mt8135.dtsi
> index 4d1ddd9..2c5c4da 100644
> --- a/arch/arm/boot/dts/mt8135.dtsi
> +++ b/arch/arm/boot/dts/mt8135.dtsi
> @@ -212,5 +212,31 @@
>  			status = "disabled";
>  		};
>  
> +		mmc0: mmc@11230000 {
> +			compatible = "mediatek,mt8135-mmc";
> +			reg = <0 0x11230000 0 0x108>;

Please use the whole register space for the controller here, not only
the one actually used by the registers.

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 v3 4/7] arm64: dts: mediatek: Add MT8173 MMC dts

[Sascha Hauer]

On Tue, Apr 28, 2015 at 05:48:42PM +0800, Chaotian Jing wrote:
> From: Eddie Huang <eddie.huang@mediatek.com>
> 
> Add node mmc0 and mmc1

While at it you could add mmc2 and mmc3 to the dtsi aswell. No need to patch that
again later.

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 v3 2/7] mmc: mediatek: Add Mediatek MMC driver

[Ulf Hansson]

On 28 April 2015 at 11:48, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> Add Mediatek MMC driver code
> Support eMMC/SD/SDIO
>
> 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 | 1416 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1425 insertions(+)
>  create mode 100644 drivers/mmc/host/mtk-sd.c

[...]

> +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);
> +
> +       msdc_gate_clock(host);
> +
> +       if (!IS_ERR(host->h_clk))
> +               clk_disable_unprepare(host->h_clk);

To make it clear when clocks are managed, I would move all that stuff
outside this function.

That would also follow how msdc_init_hw() is coded.

[...]

> +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 ||
> +               ios->timing == MMC_TIMING_MMC_DDR52)
> +               ddr = 1;
> +
> +       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);

I think you need to move the call to msdc_init_hw() into ->probe().

See more comments in the ->probe() function.

> +               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) && !host->vqmmc_enabled) {
> +                       ret = regulator_enable(mmc->supply.vqmmc);
> +                       if (ret)
> +                               dev_err(host->dev, "Failed to set vqmmc power!\n");
> +                       else
> +                               host->vqmmc_enabled = true;
> +               }
> +               msdc_ungate_clock(host);

I suggest that clocks that is managed through the clk API, should be
enabled during ->probe(). Then leave them enabled until the ->remove()
callback gets invoked.

In the ->set_ios() callback you should care about the internal
registers of your controller to enable/disable bus clocks.
Though, that should be considering of what value the ios->clock has
and not due MMC_POWER_ON|OFF|UP.

See more comments in the ->probe() function.

> +               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) && host->vqmmc_enabled) {
> +                       regulator_disable(mmc->supply.vqmmc);
> +                       host->vqmmc_enabled = false;
> +               }
> +               msdc_gate_clock(host);

Same comment as above and see more comments in the ->probe() function.

> +               break;
> +       default:
> +               break;
> +       }
> +
> +       /* Apply different pinctrl settings for different timing */
> +       if (timing_is_uhs(ios))
> +               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)) {
> +               ret = PTR_ERR(host->src_clk);
> +               goto host_free;
> +       }
> +

I am a bit puzzled over the clock management here and in the
->set_ios() callback, as mentioned.

I would suggest to do clk_prepare_enable() for the "source" clock
during probe and leave it on, until the ->remove() callbacks is
invoked.

Moreover, it's a good idea to invoke msdc_init_hw() during ->probe(),
since it makes sure the controller is properly initiated at this
point. You must not rely on the mmc core to invoke your ->set_ios()
callback with MMC_POWER_OFF to deal with that.

This should also enable you to manage both "hclk" and "source" clock
from the runtime PM callbacks, which you add in patch3. Thus it should
enable you to save more power in the runtime PM suspend state.

It would also mean that the "sclk_enabled" member in your host struct
can be removed.

> +       host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> +       if (IS_ERR(host->h_clk)) {
> +               /* host->h_clk is optional, Only for MSDC0/3 at MT8173 */
> +               dev_dbg(&pdev->dev,
> +                               "Invalied hclk from the device tree!\n");
> +       } else {
> +               clk_prepare_enable(host->h_clk);
> +               dev_dbg(&pdev->dev,
> +                               "hclk rate: %ld\n", clk_get_rate(host->h_clk));
> +       }
> +

[...]

Kind regards
Uffe

Re: [PATCH v3 3/7] mmc: mediatek: Add PM support for MMC driver

[Ulf Hansson]

On 28 April 2015 at 11:48, 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 | 58 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 30e688d..79177d9 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -22,6 +22,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spinlock.h>
>
> @@ -215,6 +216,7 @@
>  #define MSDC_ASYNC_FLAG (0x1 << 1)
>  #define MSDC_MMAP_FLAG (0x1 << 2)
>
> +#define MTK_MMC_AUTOSUSPEND_DELAY      500

Most other mmc host drivers use 50ms, any reason to why this driver
wants a different value?

>  #define CMD_TIMEOUT         (HZ/10 * 5)        /* 100ms x5 */
>  #define DAT_TIMEOUT         (HZ    * 5)        /* 1000ms x5 */
>
> @@ -706,6 +708,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 */
> @@ -862,6 +867,8 @@ static void msdc_ops_request(struct mmc_host *mmc, struct mmc_request *mrq)
>                 WARN_ON(host->mrq);
>         spin_unlock_irqrestore(&host->lock, flags);
>
> +       pm_runtime_get_sync(host->dev);
> +
>         if (mrq->data)
>                 msdc_prepare_data(host, mrq);
>
> @@ -999,7 +1006,6 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
>         int ret = 0;
>
>         if (!IS_ERR(mmc->supply.vqmmc)) {
> -

White space.

>                 if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
>                         min_uv = 3300000;
>                         max_uv = 3300000;
> @@ -1018,7 +1024,6 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
>                                         ret, min_uv, max_uv);
>                 }
>         }
> -

White space.

>         return ret;
>  }
>
> @@ -1176,8 +1181,10 @@ 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 ||
> -               ios->timing == MMC_TIMING_MMC_DDR52)
> +                       ios->timing == MMC_TIMING_MMC_DDR52)

White space.

>                 ddr = 1;
>
>         msdc_set_buswidth(host, ios->bus_width);
> @@ -1191,7 +1198,7 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                                         ios->vdd);
>                         if (ret) {
>                                 dev_err(host->dev, "Failed to set vmmc power!\n");
> -                               return;
> +                               goto end;
>                         }
>                 }
>                 break;
> @@ -1227,6 +1234,10 @@ 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);
> +
> +end:
> +       pm_runtime_mark_last_busy(host->dev);
> +       pm_runtime_put_autosuspend(host->dev);
>  }
>
>  static struct mmc_host_ops mt_msdc_ops = {
> @@ -1350,12 +1361,19 @@ static int msdc_drv_probe(struct platform_device *pdev)
>         if (ret)
>                 goto release;
>
> +       pm_runtime_set_active(host->dev);
> +       pm_runtime_set_autosuspend_delay(host->dev, MTK_MMC_AUTOSUSPEND_DELAY);
> +       pm_runtime_use_autosuspend(host->dev);
> +       pm_runtime_enable(host->dev);
>         ret = mmc_add_host(mmc);
> +
>         if (ret)
> -               goto release;
> +               goto end;
>
>         return 0;
>
> +end:
> +       pm_runtime_disable(host->dev);
>  release:
>         platform_set_drvdata(pdev, NULL);
>         msdc_deinit_hw(host);
> @@ -1368,6 +1386,7 @@ release_mem:
>                 dma_free_coherent(&pdev->dev,
>                         MAX_BD_NUM * sizeof(struct mt_bdma_desc),
>                         host->dma.bd, host->dma.bd_addr);
> +

White space.

>  host_free:
>         mmc_free_host(mmc);
>
> @@ -1382,10 +1401,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_disable(host->dev);
> +       pm_runtime_put_noidle(host->dev);
>         dma_free_coherent(&pdev->dev,
>                         sizeof(struct mt_gpdma_desc),
>                         host->dma.gpd, host->dma.gpd_addr);
> @@ -1397,6 +1420,30 @@ static int msdc_drv_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +#ifdef CONFIG_PM
> +static int msdc_runtime_suspend(struct device *dev)
> +{
> +       struct mmc_host *mmc = dev_get_drvdata(dev);
> +       struct msdc_host *host = mmc_priv(mmc);
> +
> +       msdc_gate_clock(host);

According to comments to patch2, I would think we should be able to
handle both the "hclk" and the "source" clock from the runtime PM
callbacks. Let's see.

It should anyway not affect this patch that much. Overall it looks good!

> +       return 0;
> +}
> +
> +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", },
>         {}
> @@ -1408,6 +1455,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

Re: [PATCH v3 2/7] mmc: mediatek: Add Mediatek MMC driver

[Ulf Hansson]

On 6 May 2015 at 08:54, chaotian.jing <chaotian.jing@mediatek.com> wrote:
> Dear Ulf,
>
> Thanks for your review.
> I must do a explain of our MMC host:
> Source clock is source clock of the MMC bus, MMC host has a divider to
> get different bus clock frequency. now the runtime suspend is gating
> this clock.
>
> Hclk is the power domain of the MMC host, if Hclk is gated, the MMC host
> cannot work(all registers readout is zero). and, all registers would be
> reset to default value if Hclk is gated/ungated.
> At MT8173, MSDC0 and MSDC2 has independent Hclk, MSDC1 and MSDC3's Hclk
> was controlled by "Infra module".

Thanks for clarifying!

I don't have enough knowledge about your SoC to understand the detail,
but it seems like we are mixing clocks and power domains. I would
rather keep this separate - if the HW allows it.

I guess the key question I have is the following:
1) Is it hardware wise possible to gate the hclk, but without gating
the power domain?
2) At what level is the reference counting done for each device in the
power domain? In HW or in sofftware?

>
> And, our MMC host has ability to control the gate/ungate of bus clock
> automatically, in MSDC_CFG bit 1, if this bit is set to 0, then "bus
> clock is gated to 0 if no command or data is transmitted".
> So, if the runtime PM do not control the Source clock, Hclk, then the
> runtime PM is needless.
>
> if runtime PM do gate/ungate Hclk, then need do save/restore the
> registers meanwhile.

Yes agree, that's a common thing to deal with from runtime PM callbacks.

>
> So, how about your suggestion ?
> do we still need runtime PM ?

Yes, I definitely think you need it!

Kind regards
Uffe

Re: [PATCH v3 2/7] mmc: mediatek: Add Mediatek MMC driver

[Ulf Hansson]

On 7 May 2015 at 03:42, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> Dear Ulf,
>
> Thanks!
> Below is my comment:
>
> On Wed, 2015-05-06 at 18:31 +0200, Ulf Hansson wrote:
>> On 6 May 2015 at 08:54, chaotian.jing <chaotian.jing@mediatek.com> wrote:
>> > Dear Ulf,
>> >
>> > Thanks for your review.
>> > I must do a explain of our MMC host:
>> > Source clock is source clock of the MMC bus, MMC host has a divider to
>> > get different bus clock frequency. now the runtime suspend is gating
>> > this clock.
>> >
>> > Hclk is the power domain of the MMC host, if Hclk is gated, the MMC host
>> > cannot work(all registers readout is zero). and, all registers would be
>> > reset to default value if Hclk is gated/ungated.
>> > At MT8173, MSDC0 and MSDC2 has independent Hclk, MSDC1 and MSDC3's Hclk
>> > was controlled by "Infra module".
> Sorry for mistake, MSDC0 and MSDC3 has independent Hclk, MSDC1 and
> MSDC2's Hclk was controlled by "Infra module". the Infra module is a
> "power saving module", when the system go to sleep, Infra module will be
> set and MSDC1 & MSDC2's Hclk will be gated automatically.
>>
>> Thanks for clarifying!
>>
>> I don't have enough knowledge about your SoC to understand the detail,
>> but it seems like we are mixing clocks and power domains. I would
>> rather keep this separate - if the HW allows it.
>>
>> I guess the key question I have is the following:
>> 1) Is it hardware wise possible to gate the hclk, but without gating
>> the power domain?
>> 2) At what level is the reference counting done for each device in the
>> power domain? In HW or in sofftware?
>>
>
> Actually, Our MMC host do not have power domain, all the control of the
> host is the Hclk.

Okay, but I am not sure that fully answered my question.

You said that hclk will be gated when the power domain gets gated. And
the power domain (infra module, right?) will be gated at system PM
sleep. And when that happens the mmc controller loses register
context.

But, again, can hclk be gated without gating the power domain?

No matter what, it seems like a good idea to gate the power domain
when all clients of it are idle. Yes we would need to manage register
save/restore context, but on the other hand allow you to save more
power, right?

Kind regards
Uffe

Re: [PATCH v3 2/7] mmc: mediatek: Add Mediatek MMC driver

[Ulf Hansson]

On 11 May 2015 at 11:18, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> On Fri, 2015-05-08 at 14:12 +0200, Ulf Hansson wrote:
>> On 7 May 2015 at 03:42, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>> > Dear Ulf,
>> >
>> > Thanks!
>> > Below is my comment:
>> >
>> > On Wed, 2015-05-06 at 18:31 +0200, Ulf Hansson wrote:
>> >> On 6 May 2015 at 08:54, chaotian.jing <chaotian.jing@mediatek.com> wrote:
>> >> > Dear Ulf,
>> >> >
>> >> > Thanks for your review.
>> >> > I must do a explain of our MMC host:
>> >> > Source clock is source clock of the MMC bus, MMC host has a divider to
>> >> > get different bus clock frequency. now the runtime suspend is gating
>> >> > this clock.
>> >> >
>> >> > Hclk is the power domain of the MMC host, if Hclk is gated, the MMC host
>> >> > cannot work(all registers readout is zero). and, all registers would be
>> >> > reset to default value if Hclk is gated/ungated.
>> >> > At MT8173, MSDC0 and MSDC2 has independent Hclk, MSDC1 and MSDC3's Hclk
>> >> > was controlled by "Infra module".
>> > Sorry for mistake, MSDC0 and MSDC3 has independent Hclk, MSDC1 and
>> > MSDC2's Hclk was controlled by "Infra module". the Infra module is a
>> > "power saving module", when the system go to sleep, Infra module will be
>> > set and MSDC1 & MSDC2's Hclk will be gated automatically.
>> >>
>> >> Thanks for clarifying!
>> >>
>> >> I don't have enough knowledge about your SoC to understand the detail,
>> >> but it seems like we are mixing clocks and power domains. I would
>> >> rather keep this separate - if the HW allows it.
>> >>
>> >> I guess the key question I have is the following:
>> >> 1) Is it hardware wise possible to gate the hclk, but without gating
>> >> the power domain?
>> >> 2) At what level is the reference counting done for each device in the
>> >> power domain? In HW or in sofftware?
>> >>
>> >
>> > Actually, Our MMC host do not have power domain, all the control of the
>> > host is the Hclk.
>>
>> Okay, but I am not sure that fully answered my question.
>>
>> You said that hclk will be gated when the power domain gets gated. And
>> the power domain (infra module, right?) will be gated at system PM
>> sleep. And when that happens the mmc controller loses register
>> context.
>>
>> But, again, can hclk be gated without gating the power domain?
>>
>> No matter what, it seems like a good idea to gate the power domain
>> when all clients of it are idle. Yes we would need to manage register
>> save/restore context, but on the other hand allow you to save more
>> power, right?
>>
> For MSDC0 & MSDC3, it has independent HCLK, do not have power domain.
> For MSDC1 & MSDC2, its HCLK was controlled by Infra module(This is a big
> power domain for many modules).
> So, for msdc0 & msdc3, we can control its Hclk, for msdc1 & msdc2, we
> cannot control it.

Okay, thanks for the clarification!

>
> Yes, I will do gate/ungate HCLK & source clock in PM.
> In addition, Since the gate/ungate HCLK is done by runtime PM, and all
> register operation should work at HCLK is on, so the msdc_init_hw() only
> can be called at set_ios(), because in probe(), the HCLK may have not
> been enabled.

I suggest you to enable HCLK in ->probe() and thus you may also call
msdc_init_hw() from there.

This should simplify for you, when adding the runtime PM support.

Kind regards
Uffe