Re: [PATCH V4 01/15] mmc: host: Add facility to support re-tuning

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 9443 bytes --]

On Thu, 2 Apr 2015 16:00:52 +0200 Ulf Hansson <ulf.hansson@linaro.org> wrote:

> [...]
> 
> >>>>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Our SDIO wifi device has such a state and it can only come out of it
> >>>>>>>> upon
> >>>>>>>> CMD52 write or CMD14 (ExitSleep).
> >>>>>>>
> >>>>>>>
> >>>>>>> So how will the mmc core know about these states? I guess we will
> >>>>>>> require SDIO func drivers to deal with enable/disable or hold/release
> >>>>>>> of re-tuning then?
> >>>>>>
> >>>>>>
> >>>>>> This is actually why in the past we tried to only kick off retuning
> >>>>>> before a
> >>>>>> request that requires use of data line(s) so mmc core (or sdhci) would not
> >>>>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
> >>>>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
> >>>>>
> >>>>> That's an interesting idea. It would eliminate the need for SDIO func
> >>>>> drivers to care about holding/releasing re-tuning.
> >>>>>
> >>>>> Would be nice to hear about Adrian's thoughts around this as well.
> >>>>
> >>>> I don't see how it would work because re-tuning is needed after the host
> >>>> controller runtime resumes. i.e. once the SDIO wifi card stops being active
> >>>> the host controller will runtime suspend.
> >>>
> >>> Why do we always need to re-tune from this phase?
> >>>
> >>> What Arend points out is that we could "delay" the re-tune until we
> >>> know there is a DATA request. Wouldn't that work for SDHCI as well?
> >>
> >> You beat me to it, but that is indeed what I meant.
> >
> > The CMD line needs tuning too, so delaying tuning on every command will
> > cause errors. It is better to hold tuning for the specific command used to
> > wake-up.
> 
> Hmm.
> 
> This touches the discussion where Neil Brown also was involved, around
> how to handle "idle operations" for SDIO.

Does it?  I'm not at all sure that I follow all the issues with re-tuning.

It *seems* that there are two sorts of events:
 1/ those that indicate that a retune will be needed.  This includes
    power cycling of some device, and time passing.
 2/ those that need to have retuning done (if needed) before they are
    performed.

Did I over-simplify there?

If I didn't, then do we just need a "retune needed" flag which type-1 events
can set asynchronously, and which type-2 events check before they are
performed.
The code I looked at seems to contain those ideas, but it seems more
complicated so I must have missed something.

For my purposes, "idle" it exactly "not mmc_claimed for a while".

I cannot quite see where "idle" fits in to retuning.... unless maybe you want
to retune proactively *before* a type-2 event comes along.  Is that the issue?

> 
> How does SDIO func drivers detects "request inactivity", which
> triggers them to send its device to "sleep mode"? That answer should
> be runtime PM.

"should be" :-)
I just had a look at libertas/if_sdio because that is  the driver that I use.
It has an "auto_deepsleep_timer" which works a lot like runtime_pm
autosuspend.   However it doesn't appear that auto_deepsleep mode is ever
enabled :-( so there isn't much point converting it to use runtime_pm.


> 
> So, from mmc core perspective we should be able to get notifications
> through runtime PM callbacks (from mmc or sdio bus) to understand
> whether we need to hold of re-tune.

I'm having trouble getting to grips with this idea of "holding off re-tune".
Is it not sufficient to only allow a re-tune to happen when the host is
claimed by the retuner?
In generally if you want to cause something to "hold off", you use a lock of
some sort.  Can we not just create a lock (if no present lock is sufficient)
- which may just be a flag - and take it whenever re-tune is not allowed?


> 
> I know, it's a bit of a vague idea so I will give it some more
> thinking during the Easter holidays.

To give you something more concrete to work with, below is how I want to do
idle detection.  I'm defining "idle" as "mmc_host hasn't been claimed for
100ms".  I do it by enabling runtime-pm on the mmc_host.
I have a follow-on patch which adds an idle handler for the 4-bit->1-bit
transition.

One issue that I haven't quite resolved to my own satisfaction is whether the
mmc host devices (parents of the mmc_host class device) should have
ignore_children set.  Several already do, but this is currently completely
pointless as the child (the mmc_host) doesn't do power management, so there
is nothing to ignore.

I think I'd like to remove all the pm_suspend_ignore_children() calls from
drivers/mmc/host/*.c and from mmc_alloc_host()  in this patch.  Then whenever
the mmc_host was pm_runtime active - i.e. when the mmc_host was claimed - the
host device would be held active.  Then we could revert

mmc: core: Enable runtime PM management of host devices

as it would no longer be needed.

Happy Easter!

NeilBrown




From: NeilBrown <neil@brown.name>
Date: Tue, 24 Mar 2015 17:18:15 +1100
Subject: [PATCH] mmc/core: add pm_runtime tracking to mmc_host devices.

Enable runtime pm for mmc_host devices, and take a
reference while the host is claimed.

Use an autosuspend timeout so that the device isn't
put to sleep until we have been idle for a while.

Set the parent to ignore children, so the PM status of
the host does not affect the controller at all.

This functionality will be used in a future patch to allow
commands to be sent to the device when the host is idle.

Signed-off-by: NeilBrown <neil@brown.name>

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index f6faa18edf7b..39e2c781e382 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -901,6 +901,7 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
 
 	might_sleep();
 
+	pm_runtime_get_sync(mmc_classdev(host));
 	add_wait_queue(&host->wq, &wait);
 	spin_lock_irqsave(&host->lock, flags);
 	while (1) {
@@ -924,6 +925,8 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
 	spin_unlock_irqrestore(&host->lock, flags);
 	remove_wait_queue(&host->wq, &wait);
 
+	if (stop)
+		pm_runtime_put(mmc_classdev(host));
 	if (pm)
 		pm_runtime_get_sync(mmc_dev(host));
 
@@ -956,6 +959,8 @@ void mmc_release_host(struct mmc_host *host)
 		pm_runtime_mark_last_busy(mmc_dev(host));
 		pm_runtime_put_autosuspend(mmc_dev(host));
 	}
+	pm_runtime_mark_last_busy(mmc_classdev(host));
+	pm_runtime_put_autosuspend(mmc_classdev(host));
 }
 EXPORT_SYMBOL(mmc_release_host);
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 8be0df758e68..86692b427301 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -22,6 +22,7 @@
 #include <linux/leds.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
@@ -46,9 +47,39 @@ static void mmc_host_classdev_release(struct device *dev)
 	kfree(host);
 }
 
+static int mmc_host_runtime_suspend(struct device *dev)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+
+	BUG_ON(host->claimed);
+	host->claimed = 1;
+	/* idle handling happens here */
+
+	host->claimed = 0;
+	return 0;
+}
+
+static int mmc_host_runtime_resume(struct device *dev)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+
+	BUG_ON(host->claimed);
+	host->claimed = 1;
+	/* undo any idle handling here */
+
+	host->claimed = 0;
+	return 0;
+}
+
+static struct dev_pm_ops mmc_host_dev_pm_ops = {
+	.runtime_suspend = mmc_host_runtime_suspend,
+	.runtime_resume = mmc_host_runtime_resume,
+};
+
 static struct class mmc_host_class = {
 	.name		= "mmc_host",
 	.dev_release	= mmc_host_classdev_release,
+	.pm		= &mmc_host_dev_pm_ops,
 };
 
 int mmc_register_host_class(void)
@@ -490,6 +521,11 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 	host->class_dev.parent = dev;
 	host->class_dev.class = &mmc_host_class;
 	device_initialize(&host->class_dev);
+	if (dev)
+		/*
+		 * The device can sleep even when host is claimed.
+		 */
+		pm_suspend_ignore_children(dev, true);
 
 	if (mmc_gpio_alloc(host)) {
 		put_device(&host->class_dev);
@@ -532,15 +568,25 @@ EXPORT_SYMBOL(mmc_alloc_host);
 int mmc_add_host(struct mmc_host *host)
 {
 	int err;
+	struct device *dev = mmc_classdev(host);
 
 	WARN_ON((host->caps & MMC_CAP_SDIO_IRQ) &&
 		!host->ops->enable_sdio_irq);
 
-	err = device_add(&host->class_dev);
+	err = device_add(dev);
 	if (err)
 		return err;
+	pm_runtime_enable(dev);
+	/*
+	 * The host should be able to suspend while the attached card
+	 * stays awake.
+	 */
+	pm_suspend_ignore_children(dev, true);
+	pm_runtime_get_sync(dev);
+	pm_runtime_set_autosuspend_delay(dev, 100);
+	pm_runtime_use_autosuspend(dev);
 
-	led_trigger_register_simple(dev_name(&host->class_dev), &host->led);
+	led_trigger_register_simple(dev_name(dev), &host->led);
 
 #ifdef CONFIG_DEBUG_FS
 	mmc_add_host_debugfs(host);
@@ -550,6 +596,9 @@ int mmc_add_host(struct mmc_host *host)
 	mmc_start_host(host);
 	register_pm_notifier(&host->pm_notify);
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 }
 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]