On Thu, 2 Apr 2015 16:00:52 +0200 Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxx>
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:
pgpitmnxxG7B5.pgp
Description: OpenPGP digital signature
