This allows sdhci to detect its own activity and to autosuspend
when not used

inspired from  mmci: handle clock frequency 0 properly
From Linus Walleij <linus.walleij@stericsson.com>
author of mmc aggressive clock gating fw.

The idea of using mmc clock gating fw in order to power gate the
sdhci is simple (hope no too simplistic):
Whenever the mmc fw tells we dont need the MMC clock, we dont need 
the sdhci power as well.

This does not mean that the child (card) is
suspended. In case of a Wifi SDIO card, the card will be suspended
and resumed according to the ifconfig up/down status.
Even if the Wifi interface is up, user might not use the network.
Sdhci can be powered off during those period. It is up to the HW
implementation to implement smart enough power gating to still
support enough always-on circuitry allowing to detect sdio 
interrupts.

This patch goes on top of Yunpeng's patch available on patchwork:
378052 [v2,2/3] mmc: enable runtime PM support of sdhci host controller

CC: Chris Ball <cjb@laptop.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Alan Cox <alan@linux.intel.com>
CC: Takashi Iwai <tiwai@suse.de>
CC: Maxim Levitsky <maximlevitsky@gmail.com>
CC: Linus Walleij <linus.walleij@stericsson.com>
CC: Ohad Ben-Cohen <ohad@wizery.com>
CC: Yunpeng Gao <yunpeng.gao@intel.com>

Signed-off-by: Pierre Tardy <tardyp@gmail.com>
---
 drivers/mmc/host/sdhci.c  |   20 ++++++++++++++++++++
 include/linux/mmc/sdhci.h |    1 +
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a298fb0..a648330 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1161,6 +1161,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sdhci_host *host;
 	unsigned long flags;
+        unsigned int lastclock;
 	u8 ctrl;
 
 	host = mmc_priv(mmc);
@@ -1171,6 +1172,24 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		goto out;
 
 ...
[ message continues ]

Just have a question why not let sdio card driver call pm_runtime_get/put instead of host
controller driver itself?
This patch may suspend host controller without cooperation of sdio card driver. But suspending
host controller will change controller's registers and then impact sdio card. I think it's
safer to suspend host controller according to sdio card driver's notification following runtime
PM framework. 

Another question is why to call pm_runtime_get/put when ios-clock changes? Is it based on
Linus Walleij's aggressive clock gating framework patch? Linus' patch doesn't gate SDIO cards.

Thanks,
--

[ message continues ]

Because sdio card maintain its own power via runtime_pm, and sdhci
wants to manage its power more independantly, and go suspended more
often than the sdio card. A wifi sdio card would be active when
background scanning for network. During that time, the sdio bus is
completly inactive (for full-mac wifi device), and sdhci can go
suspended during that time.
runtime_suspend of sdhci should *not* gate the sdio card. It should
only gate the sdhci.
An sdio bus inactive does not mean that the sdio card is inactive.
(think wifi Idle, bluetooth idle, ethernet idle)

We need to suspend the sdhci as soon as the sdio bus is inactive,
which is what clock gating framework is trying to detect. It does not
do usage counting via runtime_pm because it is generic enough to allow
clock gating *and* power gating.

Regards,
Pierre
--

[ message continues ]

If the wifi sdio card enters into some low power state which will not use the sdio bus, then the wifi driver can notify the sdhci host controller driver to power down the host controller only by calling the pm_runtime_put() and pm_suspend_ignore_children(). That is, it can be handled well in the current runtime pm framework. So, why we have to move to the 'aggressive clock gating framework'?

Also, I noticed that in the current 'aggressive clock gating framework' patch, the clock gating of SDIO card has been disabled (please reference code and comments of function mmc_host_may_gate_card() in that patch). That means, if discard the runtime PM framework and move to the aggressive clock gating framework, then all the SDIO cards (including the wifi sdio card) will fail to notify its host controller to enter into any low power mode.

Thanks.

Regards,
Yunpeng

--

[ message continues ]

The aggressive clock gating makes more sense since the different
drivers will know better how to handle the gating. ios with f=0 can
be interpreted differently. Else every driver has to register
runtime PM hooks for this, which is less elegant.

A better question is why the clock gating does not use runtime
PM abstractions.

The hysteresis (timeout after inactivity, in the MMC spec >= 8 MCLK
cycles) can possibly be handled by refactoring the runtime PM

We discussed different approaches to this, from marking an
SDIO slot as suspendable by using platform data, or whitelisting
the SDIO cards that can handle clock gating.

It was decided to keep SDIO cards out of it until we have this
infrastructure in place. So now you will have the opportunity
to fix this!

Not all SDIO cards will work properly if you try to gate them
so we need a mechanism to selectively do this.
Any suggestions?

Yours,
Linus Walleij
--

[ message continues ]

Thanks for the response. I just curious that is this the only reason to change the framework? To my understanding, seems it's not a very strong reason :-)

Take the example of sd/mmc card -
by using the aggressive clock gating framework, it means the host controller will gate (clock gating or power gating) itself if not receiving requests for 8 clocks even if the request queue of mmc block driver is not empty at that time. So the host controller has to be gated / ungated repeatedly until the current request queue of mmc block driver becomes empty. I don't think this is elegant since most of the gating / ungating operations are not necessary.

Instead, if we do it in the mmc block driver by just call the pm_runtime_put() once the current request queue is empty and call pm_runtime_get() once any new request comes, then the host controller can be gated/ungated appropriately.

Thanks.

Regards,
Yunpeng
--

[ message continues ]

I'm also concerned by thrashing. Please see my original patch. I am
using pm_runtime_put_autosuspend() so that suspend timeout can be
tweaked in user space.

Please also note that clock gating only platforms must also be
concerned by thrashing, as clock_disable() call might endup with PLL
My original patch does not include any ignore_child() call. So that
children can decide wether they want to prevent suspension of their
parent.That will be the case of a wifi card who uses runtime_pm to
manage its own power, and still wants its sdio bus to suspend
automatically when not used (wifi idle)


Regards,
Pierre
--

[ message continues ]

You are right..##??!! HW vendors that does not support standards...

AFAIK (was working on a sdio HW IP team before), the sdio clock is not
supposed to be taken as is for a system clock derivative especially
because it can stop between request.
I would personally suggest white list from drivers. With a call to
mmc_host_allow_gate_card(struct mmc_card *card, bool allow)

see following patch..

Regards,
-- 
Pierre
--

[ message continues ]

Some sdio card are not following sdio standard, and does not work
when the sdio bus's clock is gated

To keep functionnality for all legacy driver, we turn this quirk on
for every sdio card.
Drivers needs to disable the quirk manually when someone verified that their
supported card works with clock gating.

Signed-off-by: Pierre Tardy <tardyp@gmail.com>
---
 drivers/mmc/core/host.c  |    5 +----
 drivers/mmc/core/sdio.c  |    6 ++++++
 include/linux/mmc/card.h |    1 +
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 92e3370..54cc461 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -160,10 +160,7 @@ static bool mmc_host_may_gate_card(struct mmc_card *card)
 	 * gate the clock, because there is somebody out there that may still
 	 * be using it.
 	 */
-	if (mmc_card_sdio(card))
-		return false;
-
-	return true;
+	return !(card->quirks & MMC_QUIRK_BROKEN_CLK_GATING);
 }
 
 /**
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 82f4b90..6df1ead 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -785,6 +785,12 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 
 	mmc_release_host(host);
 
+        /*
+         * see comments in mmc_host_may_gate_card()
+         * this can be overidden by function drivers if they know that
+         * their sdio card works with clock gating
+         */
+        card->quirks |= MMC_QUIRK_BROKEN_CLK_GATING;
 	/*
 	 * First add the card to the driver model...
 	 */
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 8ce0827..5071eb1 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -121,6 +121,7 @@ struct mmc_card {
 						/* for byte mode */
 #define MMC_QUIRK_NONSTD_SDIO	(1<<2)		/* non-standard SDIO card attached */
 						/* (missing CIA registers) */
+#define MMC_QUIRK_BROKEN_CLK_GATING (1<<3)	/* clock gating the sdio bus will make card fail */
 
 ...
[ message continues ]

why not just not compile the the code without clock gating defined ?

and if code cannot run with it clock gating defined do a 
#error ?

Also from the diff I think you patch changes the behavior of SD/eMMC cards which may not be what you want .



--

[ message continues ]

The problem here is not related to the drivers that wont compile with
clock gating.
sdio clock gating, is that you gate the sdio bus clock when you dont need it.

Because some vendors sdio card IP is bad, some card are not working
properly when you do this (Their interrupt logic might need a clock to
trigger interrupt, or they are derivating their own internal clock
from the sdio bus clock). Those card are wrong thus I'm using a quirk
to deal with them.

The problem is that we still dont know how many card are wrong and
what are they. So that's why I'm making it default for all sdio card
at the moment.
When we'll have more experience on this, we can switch the behaviour,
The quirk is only set for sdio cards, which is, I think, equivalent to
the previous implementation.

-       if (mmc_card_sdio(card))
-               return false;
-
-       return true;
+       return !(card->quirks & MMC_QUIRK_BROKEN_CLK_GATING);

@@ -785,6 +785,12 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
+        card->quirks |= MMC_QUIRK_BROKEN_CLK_GATING;


This code is made to propose a solution for allowing drivers owner
that *knows* that their card work with CG, to disable the quirk in
their probe function.

Pierre
--

[ message continues ]

Hi Pierre,
I have some concerns about this RFC.
I agree with you that even if the Wifi interface is up, user might not use the network and host controller should be power gated. That also is the purport of runtime PM framework! But I think Wifi card driver can know whether user is using the network after ifconfig up, right? So that Wifi driver can tell host controller when should be power gated as well. That means you can use runtime PM framework to control Wifi card's parent to do power gating. Then why not use the it?

--

[ message continues ]