LinuxLists.cc - [PATCH v6 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos

[permalink] [raw]

Subject: [PATCH v6 3/3] mmc: dw_mmc: Set timeout to max upon resume

The TMOUT register is initted to 0xffffffff at probe time but isn't
initted after suspend/resume. Add an init of this value.

No problems were observed without this (it will also get initted in
__dw_mci_start_request if there is data to send), but it makes the
register dump before and after suspend cleaner.

Signed-off-by: Doug Anderson <[email protected]>
Acked-by: Seungwon Jeon <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
---
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

drivers/mmc/host/dw_mmc.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index f6c7545..6a9a846 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2506,6 +2506,9 @@ int dw_mci_resume(struct dw_mci *host)
/* Restore the old value at FIFOTH register */
mci_writel(host, FIFOTH, host->fifoth_val);

+ /* Put in max timeout */
+ mci_writel(host, TMOUT, 0xFFFFFFFF);
+
mci_writel(host, RINTSTS, 0xFFFFFFFF);
mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
SDMMC_INT_TXDR | SDMMC_INT_RXDR |
--
1.8.3

2013-08-22 16:19:26

[permalink] [raw]

Subject: [PATCH v6 1/3] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT

If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
looping around forever. This has been seen to happen on exynos5420
silicon despite the fact that we haven't enabled any wakeup events due
to a silicon errata. It is safe to do on all exynos variants.

Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v6:
- Took out TODO comment copied from main platform code.

Changes in v5:
- Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon.
- Don't memcpy dev_pm_ops structure, define a new one.

Changes in v4:
- Take Seungwon's suggestion and don't add any dw_mmc-pltfm code.

Changes in v3:
- Add freeze/thaw and poweroff/restore noirq entries.

Changes in v2:
- Use suspend_noirq as per James Hogan.

drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index 866edef..e8c6cc9 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -30,6 +30,7 @@
#define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \
SDMMC_CLKSEL_CCLK_DRIVE(y) | \
SDMMC_CLKSEL_CCLK_DIVIDER(z))
+#define SDMMC_CLKSEL_WAKEUP_INT BIT(11)

#define EXYNOS4210_FIXED_CIU_CLK_DIV 2
#define EXYNOS4412_FIXED_CIU_CLK_DIV 4
@@ -100,6 +101,49 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
return 0;
}

+#ifdef CONFIG_PM_SLEEP
+static int dw_mci_exynos_suspend(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+
+ return dw_mci_suspend(host);
+}
+
+static int dw_mci_exynos_resume(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+
+ return dw_mci_resume(host);
+}
+
+/**
+ * dw_mci_exynos_resume_noirq - Exynos-specific resume code
+ *
+ * On exynos5420 there is a silicon errata that will sometimes leave the
+ * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate
+ * that it fired and we can clear it by writing a 1 back. Clear it to prevent
+ * interrupts from going off constantly.
+ *
+ * We run this code on all exynos variants because it doesn't hurt.
+ */
+
+static int dw_mci_exynos_resume_noirq(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+ u32 clksel;
+
+ clksel = mci_readl(host, CLKSEL);
+ if (clksel & SDMMC_CLKSEL_WAKEUP_INT)
+ mci_writel(host, CLKSEL, clksel);
+
+ return 0;
+}
+#else
+#define dw_mci_exynos_suspend NULL
+#define dw_mci_exynos_resume NULL
+#define dw_mci_exynos_resume_noirq NULL
+#endif /* CONFIG_PM_SLEEP */
+
static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
{
/*
@@ -187,13 +231,20 @@ static int dw_mci_exynos_probe(struct platform_device *pdev)
return dw_mci_pltfm_register(pdev, drv_data);
}

+const struct dev_pm_ops dw_mci_exynos_pmops = {
+ SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume)
+ .resume_noirq = dw_mci_exynos_resume_noirq,
+ .thaw_noirq = dw_mci_exynos_resume_noirq,
+ .restore_noirq = dw_mci_exynos_resume_noirq,
+};
+
static struct platform_driver dw_mci_exynos_pltfm_driver = {
.probe = dw_mci_exynos_probe,
.remove = __exit_p(dw_mci_pltfm_remove),
.driver = {
.name = "dwmmc_exynos",
.of_match_table = dw_mci_exynos_match,
- .pm = &dw_mci_pltfm_pmops,
+ .pm = &dw_mci_exynos_pmops,
},
};

--
1.8.3

2013-08-22 16:49:25

[permalink] [raw]

Subject: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

2013-08-23 12:56:25

[permalink] [raw]

Subject: Re: [PATCH v6 3/3] mmc: dw_mmc: Set timeout to max upon resume

Acked-by: Jaehoon Chung <[email protected]>

On 08/23/2013 01:19 AM, Doug Anderson wrote:
> The TMOUT register is initted to 0xffffffff at probe time but isn't
> initted after suspend/resume. Add an init of this value.
>
> No problems were observed without this (it will also get initted in
> __dw_mci_start_request if there is data to send), but it makes the
> register dump before and after suspend cleaner.
>
> Signed-off-by: Doug Anderson <[email protected]>
> Acked-by: Seungwon Jeon <[email protected]>
> Reviewed-by: Tomasz Figa <[email protected]>
> ---
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> drivers/mmc/host/dw_mmc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index f6c7545..6a9a846 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2506,6 +2506,9 @@ int dw_mci_resume(struct dw_mci *host)
> /* Restore the old value at FIFOTH register */
> mci_writel(host, FIFOTH, host->fifoth_val);
>
> + /* Put in max timeout */
> + mci_writel(host, TMOUT, 0xFFFFFFFF);
> +
> mci_writel(host, RINTSTS, 0xFFFFFFFF);
> mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
> SDMMC_INT_TXDR | SDMMC_INT_RXDR |
>

2013-08-23 13:21:48

[permalink] [raw]

Subject: Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

Hi Doug,

If the clock-gating is enabled, then maybe it's continuously printed the kernel message for Bus_speed.

Best Regards,
Jaehoon Chung

On 08/23/2013 01:19 AM, Doug Anderson wrote:
> Previously the dw_mmc driver would ignore any requests to disable the
> card's clock. This doesn't seem like a good thing in general, but had
> one extra bad side effect in the following situtation:
> * mmc core would set clk to 400kHz at boot time while initting
> * mmc core would set clk to 0 since no card, but it would be ignored.
> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0
> but dw_mmc thinks that they're 400kHz (it ignored the set to 0).
> * insert card
> * mmc core would set clk to 400kHz which would be considered a no-op.
>
> Note that if there is no card in the slot and we do a suspend/resume
> cycle, we _do_ still end up with differences in a dw_mmc register
> dump, but the differences are clock related and we've got the clock
> disabled both before and after, so this should be OK.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> Changes in v6:
> - Replaces previous pathes that ensured saving/restoring clocks.
>
> drivers/mmc/host/dw_mmc.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index ee5f167..f6c7545 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> u32 div;
> u32 clk_en_a;
>
> - if (slot->clock != host->current_speed || force_clkinit) {
> + if (slot->clock == 0) {
> + mci_writel(host, CLKENA, 0);
> + mci_send_cmd(slot,
> + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
> + } else if (slot->clock != host->current_speed || force_clkinit) {
> div = host->bus_hz / slot->clock;
> if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
> /*
> @@ -675,9 +679,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> /* inform CIU */
> mci_send_cmd(slot,
> SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
> -
> - host->current_speed = slot->clock;
> }
> + host->current_speed = slot->clock;
>
> /* Set the current slot bus width */
> mci_writel(host, CTYPE, (slot->ctype << slot->id));
> @@ -807,13 +810,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
> mci_writel(slot->host, UHS_REG, regs);
>
> - if (ios->clock) {
> - /*
> - * Use mirror of ios->clock to prevent race with mmc
> - * core ios update when finding the minimum.
> - */
> - slot->clock = ios->clock;
> - }
> + /*
> + * Use mirror of ios->clock to prevent race with mmc
> + * core ios update when finding the minimum.
> + */
> + slot->clock = ios->clock;
>
> if (drv_data && drv_data->set_ios)
> drv_data->set_ios(slot->host, ios);
>

2013-08-23 20:40:55

[permalink] [raw]

Subject: Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

Jaehoon,

On Fri, Aug 23, 2013 at 6:21 AM, Jaehoon Chung <[email protected]> wrote:
> Hi Doug,
>
> If the clock-gating is enabled, then maybe it's continuously printed the kernel message for Bus_speed.

Can you explain? I don't think dw_mmc has support for clock gating
right now. ...or are there some patches that I'm not aware of? I
could believe that if you've got some non-upstream clock gating
patches that these would need to be modified to handle it... ...but
unless those are slated to land upstream it seems like I can't take
them into account, can I?

Thanks!

-Doug

2013-08-26 01:31:08

[permalink] [raw]

Subject: Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

Hi Doug,

On 08/24/2013 05:40 AM, Doug Anderson wrote:
> Jaehoon,
>
> On Fri, Aug 23, 2013 at 6:21 AM, Jaehoon Chung <[email protected]> wrote:
>> Hi Doug,
>>
>> If the clock-gating is enabled, then maybe it's continuously printed the kernel message for Bus_speed.
>
> Can you explain? I don't think dw_mmc has support for clock gating
> right now. ...or are there some patches that I'm not aware of? I
> could believe that if you've got some non-upstream clock gating
> patches that these would need to be modified to handle it... ...but
> unless those are slated to land upstream it seems like I can't take
> them into account, can I?
If i enabled the CONFIG_MMC_CLK_GATE, the i have found the below message whenever some operation is run.
I will test more with your patch.

[ 6.335000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.345000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.355000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.365000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.375000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.480000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.490000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.500000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.530000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.535000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)
[ 6.545000] mmc_host mmc1: Bus speed (slot 0) = 100000000Hz (slot req 52000000Hz, actual 50000000HZ div = 1)

Best Regards,
Jaehoon Chung
>
> Thanks!
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2013-08-26 03:38:43

[permalink] [raw]

Subject: Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

Jaehoon,

On Sun, Aug 25, 2013 at 6:31 PM, Jaehoon Chung <[email protected]> wrote:
> Hi Doug,
>
> On 08/24/2013 05:40 AM, Doug Anderson wrote:
>> Jaehoon,
>>
>> On Fri, Aug 23, 2013 at 6:21 AM, Jaehoon Chung <[email protected]> wrote:
>>> Hi Doug,
>>>
>>> If the clock-gating is enabled, then maybe it's continuously printed the kernel message for Bus_speed.
>>
>> Can you explain? I don't think dw_mmc has support for clock gating
>> right now. ...or are there some patches that I'm not aware of? I
>> could believe that if you've got some non-upstream clock gating
>> patches that these would need to be modified to handle it... ...but
>> unless those are slated to land upstream it seems like I can't take
>> them into account, can I?
> If i enabled the CONFIG_MMC_CLK_GATE, the i have found the below message whenever some operation is run.
> I will test more with your patch.

Ah, sorry! I wasn't aware of that config option. I was thinking of
automatic clock gating based on something like the common clock
framework. When there are no more users of a gate clock it will get
turned off. To have that work dw_mmc would need to release its biu /
ciu clocks at some point.

If I had to guess, I'd speculate that perhaps we should just change
the printout to a dev_debug(), though I do find that printout
incredibly useful. If I had to guess I'd say that the mmc core is
switching between a clock of 0 and a full speed clock constantly. If
that's true then it means that dw_mmc used to treat that like a no-op.
Now it actually gates the clock. If you comment out the printout, do
things still work? Does your power consumption go down?

Let me know if you find anything. Otherwise I can try to reproduce this week.

-Doug

2013-08-26 04:34:31

[permalink] [raw]

Subject: RE: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

On Fri, August 23, 2013, Doug Anderson wrote:
> Previously the dw_mmc driver would ignore any requests to disable the
> card's clock. This doesn't seem like a good thing in general, but had
> one extra bad side effect in the following situtation:
> * mmc core would set clk to 400kHz at boot time while initting
> * mmc core would set clk to 0 since no card, but it would be ignored.
> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0
> but dw_mmc thinks that they're 400kHz (it ignored the set to 0).
> * insert card
> * mmc core would set clk to 400kHz which would be considered a no-op.
>
> Note that if there is no card in the slot and we do a suspend/resume
> cycle, we _do_ still end up with differences in a dw_mmc register
> dump, but the differences are clock related and we've got the clock
> disabled both before and after, so this should be OK.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> Changes in v6:
> - Replaces previous pathes that ensured saving/restoring clocks.
>
> drivers/mmc/host/dw_mmc.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index ee5f167..f6c7545 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> u32 div;
> u32 clk_en_a;
>
> - if (slot->clock != host->current_speed || force_clkinit) {
> + if (slot->clock == 0) {
> + mci_writel(host, CLKENA, 0);
> + mci_send_cmd(slot,
> + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
Basically dw_mmc driver uses host's low power mode(auto clock gating)
So, how about keeping origin code rather than programming clock setting to '0'?

> + } else if (slot->clock != host->current_speed || force_clkinit) {
And then, if condition('slot->clock is not zero') is added in order to allow to set clock,
print messages which Jaehoon pointed would be solved.

Thanks,
Seungwon Jeon

> div = host->bus_hz / slot->clock;
> if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
> /*
> @@ -675,9 +679,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> /* inform CIU */
> mci_send_cmd(slot,
> SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
> -
> - host->current_speed = slot->clock;
> }
> + host->current_speed = slot->clock;
>
> /* Set the current slot bus width */
> mci_writel(host, CTYPE, (slot->ctype << slot->id));
> @@ -807,13 +810,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
> mci_writel(slot->host, UHS_REG, regs);
>
> - if (ios->clock) {
> - /*
> - * Use mirror of ios->clock to prevent race with mmc
> - * core ios update when finding the minimum.
> - */
> - slot->clock = ios->clock;
> - }
> + /*
> + * Use mirror of ios->clock to prevent race with mmc
> + * core ios update when finding the minimum.
> + */
> + slot->clock = ios->clock;
>
> if (drv_data && drv_data->set_ios)
> drv_data->set_ios(slot->host, ios);
> --
> 1.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-08-26 09:06:09

[permalink] [raw]

Subject: Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

On 08/26/2013 01:34 PM, Seungwon Jeon wrote:
> On Fri, August 23, 2013, Doug Anderson wrote:
>> Previously the dw_mmc driver would ignore any requests to disable the
>> card's clock. This doesn't seem like a good thing in general, but had
>> one extra bad side effect in the following situtation:
>> * mmc core would set clk to 400kHz at boot time while initting
>> * mmc core would set clk to 0 since no card, but it would be ignored.
>> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0
>> but dw_mmc thinks that they're 400kHz (it ignored the set to 0).
>> * insert card
>> * mmc core would set clk to 400kHz which would be considered a no-op.
>>
>> Note that if there is no card in the slot and we do a suspend/resume
>> cycle, we _do_ still end up with differences in a dw_mmc register
>> dump, but the differences are clock related and we've got the clock
>> disabled both before and after, so this should be OK.
>>
>> Signed-off-by: Doug Anderson <[email protected]>
>> ---
>> Changes in v6:
>> - Replaces previous pathes that ensured saving/restoring clocks.
>>
>> drivers/mmc/host/dw_mmc.c | 21 +++++++++++----------
>> 1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index ee5f167..f6c7545 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>> u32 div;
>> u32 clk_en_a;
>>
>> - if (slot->clock != host->current_speed || force_clkinit) {
>> + if (slot->clock == 0) {
>> + mci_writel(host, CLKENA, 0);
>> + mci_send_cmd(slot,
>> + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
> Basically dw_mmc driver uses host's low power mode(auto clock gating)
> So, how about keeping origin code rather than programming clock setting to '0'?
Right, Dw-mmc controller can use the Low-power mode.
This mode is functionality like clock-gating. Well, i didn't know what benefit we get, if set to 0.

Best Regards,
Jaehoon Chung
>
>> + } else if (slot->clock != host->current_speed || force_clkinit) {
> And then, if condition('slot->clock is not zero') is added in order to allow to set clock,
> print messages which Jaehoon pointed would be solved.
>
> Thanks,
> Seungwon Jeon
>
>> div = host->bus_hz / slot->clock;
>> if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
>> /*
>> @@ -675,9 +679,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>> /* inform CIU */
>> mci_send_cmd(slot,
>> SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
>> -
>> - host->current_speed = slot->clock;
>> }
>> + host->current_speed = slot->clock;
>>
>> /* Set the current slot bus width */
>> mci_writel(host, CTYPE, (slot->ctype << slot->id));
>> @@ -807,13 +810,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>
>> mci_writel(slot->host, UHS_REG, regs);
>>
>> - if (ios->clock) {
>> - /*
>> - * Use mirror of ios->clock to prevent race with mmc
>> - * core ios update when finding the minimum.
>> - */
>> - slot->clock = ios->clock;
>> - }
>> + /*
>> + * Use mirror of ios->clock to prevent race with mmc
>> + * core ios update when finding the minimum.
>> + */
>> + slot->clock = ios->clock;
>>
>> if (drv_data && drv_data->set_ios)
>> drv_data->set_ios(slot->host, ios);
>> --
>> 1.8.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2013-08-26 16:05:52

[permalink] [raw]

Subject: Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

Jaehoon / Seungwon,

On Mon, Aug 26, 2013 at 2:06 AM, Jaehoon Chung <[email protected]> wrote:
> On 08/26/2013 01:34 PM, Seungwon Jeon wrote:
>> On Fri, August 23, 2013, Doug Anderson wrote:
>>> Previously the dw_mmc driver would ignore any requests to disable the
>>> card's clock. This doesn't seem like a good thing in general, but had
>>> one extra bad side effect in the following situtation:
>>> * mmc core would set clk to 400kHz at boot time while initting
>>> * mmc core would set clk to 0 since no card, but it would be ignored.
>>> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0
>>> but dw_mmc thinks that they're 400kHz (it ignored the set to 0).
>>> * insert card
>>> * mmc core would set clk to 400kHz which would be considered a no-op.
>>>
>>> Note that if there is no card in the slot and we do a suspend/resume
>>> cycle, we _do_ still end up with differences in a dw_mmc register
>>> dump, but the differences are clock related and we've got the clock
>>> disabled both before and after, so this should be OK.
>>>
>>> Signed-off-by: Doug Anderson <[email protected]>
>>> ---
>>> Changes in v6:
>>> - Replaces previous pathes that ensured saving/restoring clocks.
>>>
>>> drivers/mmc/host/dw_mmc.c | 21 +++++++++++----------
>>> 1 file changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index ee5f167..f6c7545 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>> u32 div;
>>> u32 clk_en_a;
>>>
>>> - if (slot->clock != host->current_speed || force_clkinit) {
>>> + if (slot->clock == 0) {
>>> + mci_writel(host, CLKENA, 0);
>>> + mci_send_cmd(slot,
>>> + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
>> Basically dw_mmc driver uses host's low power mode(auto clock gating)
>> So, how about keeping origin code rather than programming clock setting to '0'?
> Right, Dw-mmc controller can use the Low-power mode.
> This mode is functionality like clock-gating. Well, i didn't know what benefit we get, if set to 0.

Ah, right. ...so it's unlikely that we'd save any power because we're
already gating the clock.

I'd really still rather honor the MMC subsystem's request. It
shouldn't _hurt_ to turn the clock off when the subsystem requests it,
right? One reason to honor the mmc core is that it will make things
cleaner if/when we support a voltage change operation. The MMC core
has the logic for the voltage change, and part of that involves
turning off the clock. We'll already need a bunch of special case
code in dw_mmc for voltage change, but it would be nice to avoid one
extra bit.

Another option would be to add a core MMC quirk to disable MMC_CLKGATE
on a per-driver basis. In the "single zImage" world that seems like
the right thing to do, since the MMC_CLKGATE description seems to
indicate that enabling that won't really work on all drivers anyway.

-Doug

2013-08-29 07:05:01

[permalink] [raw]

Subject: RE: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

On Tuesday, August 27, 2013, Doug Anderson wrote:
> Jaehoon / Seungwon,
>
> On Mon, Aug 26, 2013 at 2:06 AM, Jaehoon Chung <[email protected]> wrote:
> > On 08/26/2013 01:34 PM, Seungwon Jeon wrote:
> >> On Fri, August 23, 2013, Doug Anderson wrote:
> >>> Previously the dw_mmc driver would ignore any requests to disable the
> >>> card's clock. This doesn't seem like a good thing in general, but had
> >>> one extra bad side effect in the following situtation:
> >>> * mmc core would set clk to 400kHz at boot time while initting
> >>> * mmc core would set clk to 0 since no card, but it would be ignored.
> >>> * suspend to ram and resume; clocks in the dw_mmc IP block are now 0
> >>> but dw_mmc thinks that they're 400kHz (it ignored the set to 0).
> >>> * insert card
> >>> * mmc core would set clk to 400kHz which would be considered a no-op.
> >>>
> >>> Note that if there is no card in the slot and we do a suspend/resume
> >>> cycle, we _do_ still end up with differences in a dw_mmc register
> >>> dump, but the differences are clock related and we've got the clock
> >>> disabled both before and after, so this should be OK.
> >>>
> >>> Signed-off-by: Doug Anderson <[email protected]>
> >>> ---
> >>> Changes in v6:
> >>> - Replaces previous pathes that ensured saving/restoring clocks.
> >>>
> >>> drivers/mmc/host/dw_mmc.c | 21 +++++++++++----------
> >>> 1 file changed, 11 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >>> index ee5f167..f6c7545 100644
> >>> --- a/drivers/mmc/host/dw_mmc.c
> >>> +++ b/drivers/mmc/host/dw_mmc.c
> >>> @@ -635,7 +635,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> >>> u32 div;
> >>> u32 clk_en_a;
> >>>
> >>> - if (slot->clock != host->current_speed || force_clkinit) {
> >>> + if (slot->clock == 0) {
> >>> + mci_writel(host, CLKENA, 0);
> >>> + mci_send_cmd(slot,
> >>> + SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
> >> Basically dw_mmc driver uses host's low power mode(auto clock gating)
> >> So, how about keeping origin code rather than programming clock setting to '0'?
> > Right, Dw-mmc controller can use the Low-power mode.
> > This mode is functionality like clock-gating. Well, i didn't know what benefit we get, if set to 0.
>
> Ah, right. ...so it's unlikely that we'd save any power because we're
> already gating the clock.
>
> I'd really still rather honor the MMC subsystem's request. It
> shouldn't _hurt_ to turn the clock off when the subsystem requests it,
Even though turning off by clock programming doesn't hurt,
it is costly behavior when considering low power mode of host's own support.
Just now, how about focusing on the problem clock isn't updated properly after suspend/resume?

> right? One reason to honor the mmc core is that it will make things
> cleaner if/when we support a voltage change operation. The MMC core
> has the logic for the voltage change, and part of that involves
> turning off the clock. We'll already need a bunch of special case
> code in dw_mmc for voltage change, but it would be nice to avoid one
> extra bit.
Turning off clock during voltage switching would be another procedure.
I guess it could be discussed later.
I want to fix some minor change to prevent frequent message that Jaehoon pointed.

Thanks,
Seungwon Jeon

>
> Another option would be to add a core MMC quirk to disable MMC_CLKGATE
> on a per-driver basis. In the "single zImage" world that seems like
> the right thing to do, since the MMC_CLKGATE description seems to
> indicate that enabling that won't really work on all drivers anyway.
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-08-29 16:34:17

[permalink] [raw]

Subject: Re: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

Seungwon,

On Thu, Aug 29, 2013 at 12:04 AM, Seungwon Jeon <[email protected]> wrote:
>> I'd really still rather honor the MMC subsystem's request. It
>> shouldn't _hurt_ to turn the clock off when the subsystem requests it,
> Even though turning off by clock programming doesn't hurt,
> it is costly behavior when considering low power mode of host's own support.

It is costly? We are talking about these two commands, right?

mci_writel(host, CLKENA, 0);
mci_send_cmd(slot,
SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);

Do you have a reason to believe that these are more costly than all of
the rest of the code that's executed when the user defines
CONFIG_MMC_CLKGATE? You're still proposing doing all of the updates
of the clock when slot->clock is non-zero, right? ...so at best
skipping this code will be 33% faster since the re-enable code
disables and then reenables the clock. If it's the
"SDMMC_CMD_PRV_DAT_WAIT" that you're worried about then skipping this
code will only be 25% faster since there are already three calls with
SDMMC_CMD_PRV_DAT_WAIT in the enable code.

> Just now, how about focusing on the problem clock isn't updated properly after suspend/resume?

I tried to do that in the original patches, but you pointed out
(correctly) that we should do the correct fix rather than a hackier
fix. IMHO the most correct fix is to honor the MMC core's request to
turn the clock off. Partially honoring the MMC core (as you suggest)
is certainly less hacky that my original proposal but I still think
turning the clock off is better.

>> right? One reason to honor the mmc core is that it will make things
>> cleaner if/when we support a voltage change operation. The MMC core
>> has the logic for the voltage change, and part of that involves
>> turning off the clock. We'll already need a bunch of special case
>> code in dw_mmc for voltage change, but it would be nice to avoid one
>> extra bit.
> Turning off clock during voltage switching would be another procedure.
> I guess it could be discussed later.

Agreed that we're not trying to get voltage switching done here, but
forward thinking is nice. If there's no reason _not_ to turn the
clock off and it will help us later, let's do it. Also, we've already
agreed that MMC_CLKGATE isn't so useful for dw_mmc, so trying to do
something awkward to make MMC_CLKGATE slightly faster doesn't seem
worth it.

> I want to fix some minor change to prevent frequent message that Jaehoon pointed.

As far as I can tell, the frequent messages and whether or not to
actually turn the clock off are unrelated. I will send up a patch
that fixes the frequent messages by caching the last value printed and
only printing if it changed. I have verified that this works and that
the system still functions OK (can boot to prompt) with
CONFIG_MMC_CLKGATE.

Note: re-reading over some of the previous messages, it sounds like
you're proposing using the patch from your email directly, AKA:

http://article.gmane.org/gmane.linux.kernel/1542482

Did you test that patch? Did it work for you? It doesn't actually
compile cleanly for me (you removed the "force_clkinit" param in the
function but not the callers). That's easy to fix, but implies that
this patch was just a proposal and not a tested solution.

...but aside from not compiling cleanly, I don't think it will work
for the same reasons that the original code didn't work. Specifically
it doesn't address the core problem that we need to update
host->current_speed when the clock is 0. Otherwise we won't re-init
and we run into the original problem, right? To be certain I took
your patch and applied it, then fixed the callers of
dw_mci_setup_bus() not to pass a second parameter. I did a
suspend/resume with no card in and then plugged a card in. It didn't
work.

As I said above, new patch coming shortly. As always: feel free to
point out any glaring mistakes I made in the above. ;) Note that I
will be out of communication for the next week or so and buried
beneath email for a few days after that, so my response might be a
little delayed.

-Doug

2013-08-29 16:39:22

[permalink] [raw]

Subject: [PATCH v7 0/3] mmc: dw_mmc: fixes for suspend/resume on exynos

This series of patches addresses some suspend/resume problems with
dw_mmc on exynos platforms, espeically exynos5420. This patchset was
tested on the current ToT Chromeos 3.8 tree (which has lots of
backports from 3.10/3.11) and on ToT Linux (v3.11-rc6). I have
confirmed basic booting and that SD cards work across suspend/resume
(both if they are plugged in and if they are not plugged in).

I have received confirmation from Samsung that the problem solved for
exynos5420 is a silicon errata and that this is a good fix.

Changes in v7:
- Avoid printing the same clock over and over again w/ MMC_CLKGATE.

Changes in v6:
- Took out TODO comment copied from main platform code.
- Replaces previous pathes that ensured saving/restoring clocks.

Changes in v5:
- Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon.
- Don't memcpy dev_pm_ops structure, define a new one.

Changes in v4:
- Take Seungwon's suggestion and don't add any dw_mmc-pltfm code.

Changes in v3:
- Add freeze/thaw and poweroff/restore noirq entries.

Changes in v2:
- Use suspend_noirq as per James Hogan.

Doug Anderson (3):
mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)
mmc: dw_mmc: Set timeout to max upon resume

drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++-
drivers/mmc/host/dw_mmc.c | 39 ++++++++++++++++++-----------
2 files changed, 77 insertions(+), 15 deletions(-)

--
1.8.4

2013-08-29 16:39:26

[permalink] [raw]

Subject: [PATCH v7 3/3] mmc: dw_mmc: Set timeout to max upon resume

The TMOUT register is initted to 0xffffffff at probe time but isn't
initted after suspend/resume. Add an init of this value.

No problems were observed without this (it will also get initted in
__dw_mci_start_request if there is data to send), but it makes the
register dump before and after suspend cleaner.

Signed-off-by: Doug Anderson <[email protected]>
Acked-by: Seungwon Jeon <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
---
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

drivers/mmc/host/dw_mmc.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1584705..f8d891a 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2513,6 +2513,9 @@ int dw_mci_resume(struct dw_mci *host)
/* Restore the old value at FIFOTH register */
mci_writel(host, FIFOTH, host->fifoth_val);

+ /* Put in max timeout */
+ mci_writel(host, TMOUT, 0xFFFFFFFF);
+
mci_writel(host, RINTSTS, 0xFFFFFFFF);
mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
SDMMC_INT_TXDR | SDMMC_INT_RXDR |
--
1.8.4

2013-08-29 16:39:23

[permalink] [raw]

Subject: [PATCH v7 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

Previously the dw_mmc driver would ignore any requests to disable the
card's clock. This doesn't seem like a good thing in general, but had
one extra bad side effect in the following situtation:
* mmc core would set clk to 400kHz at boot time while initting
* mmc core would set clk to 0 since no card, but it would be ignored.
* suspend to ram and resume; clocks in the dw_mmc IP block are now 0
but dw_mmc thinks that they're 400kHz (it ignored the set to 0).
* insert card
* mmc core would set clk to 400kHz which would be considered a no-op.

Note that if there is no card in the slot and we do a suspend/resume
cycle, we _do_ still end up with differences in a dw_mmc register
dump, but the differences are clock related and we've got the clock
disabled both before and after, so this should be OK.

Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v7:
- Avoid printing the same clock over and over again w/ MMC_CLKGATE.

Changes in v6:
- Replaces previous pathes that ensured saving/restoring clocks.

drivers/mmc/host/dw_mmc.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index ee5f167..1584705 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -88,6 +88,8 @@ struct idmac_desc {
* @queue_node: List node for placing this node in the @queue list of
* &struct dw_mci.
* @clock: Clock rate configured by set_ios(). Protected by host->lock.
+ * @last_printed_clock: The last clock we printed. Keeping track of this helps
+ * us to avoid spamming the console with CONFIG_MMC_CLKGATE.
* @flags: Random state bits associated with the slot.
* @id: Number of this slot.
* @last_detect_state: Most recently observed card detect state.
@@ -105,6 +107,7 @@ struct dw_mci_slot {
struct list_head queue_node;

unsigned int clock;
+ unsigned int last_printed_clock;
unsigned long flags;
#define DW_MMC_CARD_PRESENT 0
#define DW_MMC_CARD_NEED_INIT 1
@@ -635,7 +638,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
u32 div;
u32 clk_en_a;

- if (slot->clock != host->current_speed || force_clkinit) {
+ if (slot->clock == 0) {
+ mci_writel(host, CLKENA, 0);
+ mci_send_cmd(slot,
+ SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
+ } else if (slot->clock != host->current_speed || force_clkinit) {
div = host->bus_hz / slot->clock;
if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
/*
@@ -646,10 +653,14 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)

div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;

- dev_info(&slot->mmc->class_dev,
- "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ"
- " div = %d)\n", slot->id, host->bus_hz, slot->clock,
- div ? ((host->bus_hz / div) >> 1) : host->bus_hz, div);
+ if (slot->clock != slot->last_printed_clock || force_clkinit) {
+ dev_info(&slot->mmc->class_dev,
+ "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
+ slot->id, host->bus_hz, slot->clock,
+ div ? ((host->bus_hz / div) >> 1) :
+ host->bus_hz, div);
+ slot->last_printed_clock = slot->clock;
+ }

/* disable clock */
mci_writel(host, CLKENA, 0);
@@ -675,9 +686,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
/* inform CIU */
mci_send_cmd(slot,
SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
-
- host->current_speed = slot->clock;
}
+ host->current_speed = slot->clock;

/* Set the current slot bus width */
mci_writel(host, CTYPE, (slot->ctype << slot->id));
@@ -807,13 +817,11 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)

mci_writel(slot->host, UHS_REG, regs);

- if (ios->clock) {
- /*
- * Use mirror of ios->clock to prevent race with mmc
- * core ios update when finding the minimum.
- */
- slot->clock = ios->clock;
- }
+ /*
+ * Use mirror of ios->clock to prevent race with mmc
+ * core ios update when finding the minimum.
+ */
+ slot->clock = ios->clock;

if (drv_data && drv_data->set_ios)
drv_data->set_ios(slot->host, ios);
--
1.8.4

2013-08-29 16:45:28

[permalink] [raw]

Subject: [PATCH v7 1/3] mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT

If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up
looping around forever. This has been seen to happen on exynos5420
silicon despite the fact that we haven't enabled any wakeup events due
to a silicon errata. It is safe to do on all exynos variants.

Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v7: None
Changes in v6:
- Took out TODO comment copied from main platform code.

Changes in v5:
- Cleaned up dw_mci_exynos_resume_noirq() comment as per Seungwon.
- Don't memcpy dev_pm_ops structure, define a new one.

Changes in v4:
- Take Seungwon's suggestion and don't add any dw_mmc-pltfm code.

Changes in v3:
- Add freeze/thaw and poweroff/restore noirq entries.

Changes in v2:
- Use suspend_noirq as per James Hogan.

drivers/mmc/host/dw_mmc-exynos.c | 53 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index 866edef..e8c6cc9 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -30,6 +30,7 @@
#define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \
SDMMC_CLKSEL_CCLK_DRIVE(y) | \
SDMMC_CLKSEL_CCLK_DIVIDER(z))
+#define SDMMC_CLKSEL_WAKEUP_INT BIT(11)

#define EXYNOS4210_FIXED_CIU_CLK_DIV 2
#define EXYNOS4412_FIXED_CIU_CLK_DIV 4
@@ -100,6 +101,49 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host)
return 0;
}

+#ifdef CONFIG_PM_SLEEP
+static int dw_mci_exynos_suspend(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+
+ return dw_mci_suspend(host);
+}
+
+static int dw_mci_exynos_resume(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+
+ return dw_mci_resume(host);
+}
+
+/**
+ * dw_mci_exynos_resume_noirq - Exynos-specific resume code
+ *
+ * On exynos5420 there is a silicon errata that will sometimes leave the
+ * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate
+ * that it fired and we can clear it by writing a 1 back. Clear it to prevent
+ * interrupts from going off constantly.
+ *
+ * We run this code on all exynos variants because it doesn't hurt.
+ */
+
+static int dw_mci_exynos_resume_noirq(struct device *dev)
+{
+ struct dw_mci *host = dev_get_drvdata(dev);
+ u32 clksel;
+
+ clksel = mci_readl(host, CLKSEL);
+ if (clksel & SDMMC_CLKSEL_WAKEUP_INT)
+ mci_writel(host, CLKSEL, clksel);
+
+ return 0;
+}
+#else
+#define dw_mci_exynos_suspend NULL
+#define dw_mci_exynos_resume NULL
+#define dw_mci_exynos_resume_noirq NULL
+#endif /* CONFIG_PM_SLEEP */
+
static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
{
/*
@@ -187,13 +231,20 @@ static int dw_mci_exynos_probe(struct platform_device *pdev)
return dw_mci_pltfm_register(pdev, drv_data);
}

+const struct dev_pm_ops dw_mci_exynos_pmops = {
+ SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume)
+ .resume_noirq = dw_mci_exynos_resume_noirq,
+ .thaw_noirq = dw_mci_exynos_resume_noirq,
+ .restore_noirq = dw_mci_exynos_resume_noirq,
+};
+
static struct platform_driver dw_mci_exynos_pltfm_driver = {
.probe = dw_mci_exynos_probe,
.remove = __exit_p(dw_mci_pltfm_remove),
.driver = {
.name = "dwmmc_exynos",
.of_match_table = dw_mci_exynos_match,
- .pm = &dw_mci_pltfm_pmops,
+ .pm = &dw_mci_exynos_pmops,
},
};

--
1.8.4

2013-08-30 03:55:18

[permalink] [raw]

Subject: RE: [PATCH v6 2/3] mmc: dw_mmc: Honor requests to set the clock to 0 (turn off clock)

On Fri, August 30, 2013, Doug Anderson wrote:
> Seungwon,
>
> On Thu, Aug 29, 2013 at 12:04 AM, Seungwon Jeon <[email protected]> wrote:
> >> I'd really still rather honor the MMC subsystem's request. It
> >> shouldn't _hurt_ to turn the clock off when the subsystem requests it,
> > Even though turning off by clock programming doesn't hurt,
> > it is costly behavior when considering low power mode of host's own support.
>
> It is costly? We are talking about these two commands, right?
>
> mci_writel(host, CLKENA, 0);
> mci_send_cmd(slot,
> SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
I mean that because host supports auto clock gating, we don't need to
clock programming with setting '0'. If MMC_CLKGATE is enabled, clock programming
will be executed with between '0' clock and working clock frequently. Actually the result is same.
Of course, if host didn't support this feature, we would have considered that manually to save the power consumption.

>
> Do you have a reason to believe that these are more costly than all of
> the rest of the code that's executed when the user defines
> CONFIG_MMC_CLKGATE? You're still proposing doing all of the updates
> of the clock when slot->clock is non-zero, right? ...so at best
No, origin condition should be remained;
Required clock should be different with current clock.

> skipping this code will be 33% faster since the re-enable code
> disables and then reenables the clock. If it's the
> "SDMMC_CMD_PRV_DAT_WAIT" that you're worried about then skipping this
> code will only be 25% faster since there are already three calls with
> SDMMC_CMD_PRV_DAT_WAIT in the enable code.
>
>
> > Just now, how about focusing on the problem clock isn't updated properly after suspend/resume?
>
> I tried to do that in the original patches, but you pointed out
> (correctly) that we should do the correct fix rather than a hackier
> fix. IMHO the most correct fix is to honor the MMC core's request to
> turn the clock off. Partially honoring the MMC core (as you suggest)
> is certainly less hacky that my original proposal but I still think
> turning the clock off is better.
>
>
> >> right? One reason to honor the mmc core is that it will make things
> >> cleaner if/when we support a voltage change operation. The MMC core
> >> has the logic for the voltage change, and part of that involves
> >> turning off the clock. We'll already need a bunch of special case
> >> code in dw_mmc for voltage change, but it would be nice to avoid one
> >> extra bit.
> > Turning off clock during voltage switching would be another procedure.
> > I guess it could be discussed later.
>
> Agreed that we're not trying to get voltage switching done here, but
> forward thinking is nice. If there's no reason _not_ to turn the
> clock off and it will help us later, let's do it. Also, we've already
> agreed that MMC_CLKGATE isn't so useful for dw_mmc, so trying to do
> something awkward to make MMC_CLKGATE slightly faster doesn't seem
> worth it.
>
>
> > I want to fix some minor change to prevent frequent message that Jaehoon pointed.
>
> As far as I can tell, the frequent messages and whether or not to
> actually turn the clock off are unrelated. I will send up a patch
> that fixes the frequent messages by caching the last value printed and
> only printing if it changed. I have verified that this works and that
> the system still functions OK (can boot to prompt) with
> CONFIG_MMC_CLKGATE.
>
>
> Note: re-reading over some of the previous messages, it sounds like
> you're proposing using the patch from your email directly, AKA:
>
> http://article.gmane.org/gmane.linux.kernel/1542482
>
> Did you test that patch? Did it work for you? It doesn't actually
> compile cleanly for me (you removed the "force_clkinit" param in the
> function but not the callers). That's easy to fix, but implies that
> this patch was just a proposal and not a tested solution.
>
> ...but aside from not compiling cleanly, I don't think it will work
> for the same reasons that the original code didn't work. Specifically
> it doesn't address the core problem that we need to update
> host->current_speed when the clock is 0. Otherwise we won't re-init
> and we run into the original problem, right? To be certain I took
> your patch and applied it, then fixed the callers of
> dw_mci_setup_bus() not to pass a second parameter. I did a
> suspend/resume with no card in and then plugged a card in. It didn't
> work.

Some change proposed from me are mixed with both current existing part and your new patch.
It's not whole code to replace your patch. But if it makes you confused, sorry about that.
Important thing I intended is that if required clock(slot->clock) is '0', not try to update clock.
with considering automatic clock gating. Please check first comment from me on v6.

Thanks,
Seungwon Jeon
>
>
> As I said above, new patch coming shortly. As always: feel free to
> point out any glaring mistakes I made in the above. ;) Note that I
> will be out of communication for the next week or so and buried
> beneath email for a few days after that, so my response might be a
> little delayed.
>
>
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-08-30 11:31:50