LinuxLists.cc - [PATCH] sdhci: wakeup from runtime PM

2016-03-25 16:05:26

Subject: [PATCH] sdhci: wakeup from runtime PM

Hi,

When not using the SDHCI controller, it is logical to save power by suspending
it. The issue is that SDHCI assumes that the controller is completely disabled.
It means the only way to wake up on a card event is to have a gpio for the card
detection (so two pins for the same signal). A possible workaround is to use
polling but the controller will be resumed/suspended between each attempts.

We have already discussed a long time about this and it seems we don't agree.
In my opinion, even if I can't disable all clocks, I should use runtime PM
to save some power.

I propose two patches, one which is a draft to try to solve it at sdhci level
and one at sdhci-of-at91 level.

Concerning the first one, I don't understand why we need to reject irqs if
runtime_suspended is true. Only SDHCI_INT_CARD_INT irq is enabled so why we
should have other IRQs than this one?

Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
solved my issue only by modifying my driver.

I have not noticed side effects. I have compared a wakeup with a gpio for card
detection and the controller card detect. It seems the two paths are rougly
the same (mmc_detect_change -> mmc_rescan then resume is performed).

Regards

Ludovic

2016-03-25 16:05:42

by Ludovic Desroches

[permalink] [raw]

Subject: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

Signed-off-by: Ludovic Desroches <[email protected]>
---
drivers/mmc/host/sdhci-of-at91.c | 36 ++++++++++--------------------------
drivers/mmc/host/sdhci.c | 12 +++++-------
drivers/mmc/host/sdhci.h | 2 +-
3 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2703aa9..d70bb7a 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -62,12 +62,14 @@ static int sdhci_at91_runtime_suspend(struct device *dev)
struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
int ret;

- ret = sdhci_runtime_suspend_host(host);
+ ret = sdhci_runtime_suspend_host(host, SDHCI_INT_CARD_INT | SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);

clk_disable_unprepare(priv->gck);
- clk_disable_unprepare(priv->hclock);
+ //clk_disable_unprepare(priv->hclock);
clk_disable_unprepare(priv->mainck);

+ printk("sdhci_at91_runtime_suspend\n");
+
return ret;
}

@@ -78,17 +80,18 @@ static int sdhci_at91_runtime_resume(struct device *dev)
struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
int ret;

+ printk("sdhci_at91_runtime_resume\n");
ret = clk_prepare_enable(priv->mainck);
if (ret) {
dev_err(dev, "can't enable mainck\n");
return ret;
}

- ret = clk_prepare_enable(priv->hclock);
- if (ret) {
- dev_err(dev, "can't enable hclock\n");
- return ret;
- }
+ //ret = clk_prepare_enable(priv->hclock);
+ //if (ret) {
+ // dev_err(dev, "can't enable hclock\n");
+ // return ret;
+ //}

ret = clk_prepare_enable(priv->gck);
if (ret) {
@@ -205,25 +208,6 @@ static int sdhci_at91_probe(struct platform_device *pdev)
if (ret)
goto pm_runtime_disable;

- /*
- * When calling sdhci_runtime_suspend_host(), the sdhci layer makes
- * the assumption that all the clocks of the controller are disabled.
- * It means we can't get irq from it when it is runtime suspended.
- * For that reason, it is not planned to wake-up on a card detect irq
- * from the controller.
- * If we want to use runtime PM and to be able to wake-up on card
- * insertion, we have to use a GPIO for the card detection or we can
- * use polling. Be aware that using polling will resume/suspend the
- * controller between each attempt.
- * Disable SDHCI_QUIRK_BROKEN_CARD_DETECTION to be sure nobody tries
- * to enable polling via device tree with broken-cd property.
- */
- if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
- IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc))) {
- host->mmc->caps |= MMC_CAP_NEEDS_POLL;
- host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
- }
-
pm_runtime_put_autosuspend(&pdev->dev);

return 0;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8670f16..619a64a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2467,11 +2467,6 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)

spin_lock(&host->lock);

- if (host->runtime_suspended && !sdhci_sdio_irq_enabled(host)) {
- spin_unlock(&host->lock);
- return IRQ_NONE;
- }
-
intmask = sdhci_readl(host, SDHCI_INT_STATUS);
if (!intmask || intmask == 0xffffffff) {
result = IRQ_NONE;
@@ -2709,7 +2704,7 @@ static void sdhci_runtime_pm_bus_off(struct sdhci_host *host)
pm_runtime_put_noidle(host->mmc->parent);
}

-int sdhci_runtime_suspend_host(struct sdhci_host *host)
+int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs)
{
unsigned long flags;

@@ -2717,7 +2712,10 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
mmc_retune_needed(host->mmc);

spin_lock_irqsave(&host->lock, flags);
- host->ier &= SDHCI_INT_CARD_INT;
+ if (wakeup_irqs)
+ host->ier = wakeup_irqs;
+ else
+ host->ier &= SDHCI_INT_CARD_INT;
sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
spin_unlock_irqrestore(&host->lock, flags);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 3bd2803..b932e15 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -668,7 +668,7 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
extern int sdhci_suspend_host(struct sdhci_host *host);
extern int sdhci_resume_host(struct sdhci_host *host);
extern void sdhci_enable_irq_wakeups(struct sdhci_host *host);
-extern int sdhci_runtime_suspend_host(struct sdhci_host *host);
+extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
extern int sdhci_runtime_resume_host(struct sdhci_host *host);
#endif

--
2.5.0

2016-03-25 16:05:46

by Ludovic Desroches

[permalink] [raw]

Subject: [PATCH] mmc: sdhci-of-at91: allow the use of controller card detect as wake up

At the moment, there are only two ways to wake up from runtime suspend state:
- using a gpio for the card detection which is not a good solution since
you still need the card detect pin of the controller.
- using polling which is also not a good solution since it will
resume/suspend the controller between each attempt.

The sdhci layer makes the assumption that all the clocks are disabled
when suspending the controller. For that reason, irqs are rejected if
runtime_suspended is true.

This patch duplicates the sdhci_runtime_suspend_host() but doesn't set
runtime_suspended and doesn't disable the interface clock if there is a
removable device and no gpio for card detection.

Signed-off-by: Ludovic Desroches <[email protected]>
---
drivers/mmc/host/sdhci-of-at91.c | 71 +++++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 26 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2703aa9..6683f2d 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -35,6 +35,7 @@ struct sdhci_at91_priv {
struct clk *hclock;
struct clk *gck;
struct clk *mainck;
+ bool disable_interface_clk;
};

static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
@@ -55,20 +56,47 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
};

#ifdef CONFIG_PM
+/*
+ * This function is the very same as sdhci_runtime_suspend_host but if the
+ * device is removable and we only have the controller card detect signal then
+ * we won't disable all the clocks. The interface clock will be needed to get
+ * card event interrupt.
+ * When calling sdhci_runtime_suspend_host(), the sdhci layer makes the
+ * assumption that all the clocks of the controller are disabled. If
+ * runtime_suspended is set, irqs will be rejected. That's why it won't be set
+ * if we want to wake up on card event.
+ */
static int sdhci_at91_runtime_suspend(struct device *dev)
{
struct sdhci_host *host = dev_get_drvdata(dev);
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
- int ret;
+ unsigned long flags;
+
+ mmc_retune_timer_stop(host->mmc);
+ mmc_retune_needed(host->mmc);
+
+ spin_lock_irqsave(&host->lock, flags);
+ host->ier &= SDHCI_INT_CARD_INT;
+ if (!priv->disable_interface_clk)
+ host->ier |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
+ sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
+ sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
+ spin_unlock_irqrestore(&host->lock, flags);

- ret = sdhci_runtime_suspend_host(host);
+ synchronize_hardirq(host->irq);
+
+ if (priv->disable_interface_clk) {
+ spin_lock_irqsave(&host->lock, flags);
+ host->runtime_suspended = true;
+ spin_unlock_irqrestore(&host->lock, flags);
+ clk_disable_unprepare(priv->hclock);
+ };

clk_disable_unprepare(priv->gck);
- clk_disable_unprepare(priv->hclock);
clk_disable_unprepare(priv->mainck);

- return ret;
+ return 0;
}

static int sdhci_at91_runtime_resume(struct device *dev)
@@ -84,18 +112,20 @@ static int sdhci_at91_runtime_resume(struct device *dev)
return ret;
}

- ret = clk_prepare_enable(priv->hclock);
- if (ret) {
- dev_err(dev, "can't enable hclock\n");
- return ret;
- }
-
ret = clk_prepare_enable(priv->gck);
if (ret) {
dev_err(dev, "can't enable gck\n");
return ret;
}

+ if (priv->disable_interface_clk) {
+ ret = clk_prepare_enable(priv->hclock);
+ if (ret) {
+ dev_err(dev, "can't enable hclock\n");
+ return ret;
+ }
+ }
+
return sdhci_runtime_resume_host(host);
}
#endif /* CONFIG_PM */
@@ -206,23 +236,12 @@ static int sdhci_at91_probe(struct platform_device *pdev)
goto pm_runtime_disable;

/*
- * When calling sdhci_runtime_suspend_host(), the sdhci layer makes
- * the assumption that all the clocks of the controller are disabled.
- * It means we can't get irq from it when it is runtime suspended.
- * For that reason, it is not planned to wake-up on a card detect irq
- * from the controller.
- * If we want to use runtime PM and to be able to wake-up on card
- * insertion, we have to use a GPIO for the card detection or we can
- * use polling. Be aware that using polling will resume/suspend the
- * controller between each attempt.
- * Disable SDHCI_QUIRK_BROKEN_CARD_DETECTION to be sure nobody tries
- * to enable polling via device tree with broken-cd property.
+ * If the device is non removable or if there is a gpio for the card
+ * detection, all the clocks can be disabled in runtime suspend state.
*/
- if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
- IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc))) {
- host->mmc->caps |= MMC_CAP_NEEDS_POLL;
- host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
- }
+ priv->disable_interface_clk = (host->mmc->caps & MMC_CAP_NONREMOVABLE)
+ || !IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc));
+

pm_runtime_put_autosuspend(&pdev->dev);

--
2.5.0

2016-03-25 16:47:21

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

Hi Ludovic,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url: https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base: https://git.linaro.org/people/ulf.hansson/mmc next
config: x86_64-allyesdebian (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/mmc/host/sdhci-pci-core.c: In function 'sdhci_pci_runtime_suspend':
>> drivers/mmc/host/sdhci-pci-core.c:1497:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
ret = sdhci_runtime_suspend_host(slot->host);
^
In file included from drivers/mmc/host/sdhci-pci-core.c:31:0:
drivers/mmc/host/sdhci.h:671:12: note: declared here
extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
^

vim +/sdhci_runtime_suspend_host +1497 drivers/mmc/host/sdhci-pci-core.c

66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 1491
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 1492 for (i = 0; i < chip->num_slots; i++) {
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 1493 slot = chip->slots[i];
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 1494 if (!slot)
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 1495 continue;
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 1496
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 @1497 ret = sdhci_runtime_suspend_host(slot->host);
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 1498
b678b91f drivers/mmc/host/sdhci-pci.c Axel Lin 2011-12-03 1499 if (ret)
b678b91f drivers/mmc/host/sdhci-pci.c Axel Lin 2011-12-03 1500 goto err_pci_runtime_suspend;

:::::: The code at line 1497 was first introduced by commit
:::::: 66fd8ad5100b5003046aa744a4f12fa31bb831f9 mmc: sdhci-pci: add runtime pm support

:::::: TO: Adrian Hunter <[email protected]>
:::::: CC: Chris Ball <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (2.37 kB)
.config.gz (35.78 kB)
Download all attachments

2016-03-25 16:51:19

Hi Ludovic,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url: https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base: https://git.linaro.org/people/ulf.hansson/mmc next
config: i386-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All errors (new ones prefixed by >>):

drivers/mmc/host/sdhci-pxav3.c: In function 'sdhci_pxav3_runtime_suspend':
>> drivers/mmc/host/sdhci-pxav3.c:539:8: error: too few arguments to function 'sdhci_runtime_suspend_host'
ret = sdhci_runtime_suspend_host(host);
^
In file included from drivers/mmc/host/sdhci-pxav3.c:39:0:
drivers/mmc/host/sdhci.h:671:12: note: declared here
extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
^

vim +/sdhci_runtime_suspend_host +539 drivers/mmc/host/sdhci-pxav3.c

bb691ae4 Kevin Liu 2013-02-01 533 {
bb691ae4 Kevin Liu 2013-02-01 534 struct sdhci_host *host = dev_get_drvdata(dev);
bb691ae4 Kevin Liu 2013-02-01 535 struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
f599da40 Jisheng Zhang 2016-02-16 536 struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
3bb10f60 Jisheng Zhang 2015-01-23 537 int ret;
bb691ae4 Kevin Liu 2013-02-01 538
3bb10f60 Jisheng Zhang 2015-01-23 @539 ret = sdhci_runtime_suspend_host(host);
3bb10f60 Jisheng Zhang 2015-01-23 540 if (ret)
3bb10f60 Jisheng Zhang 2015-01-23 541 return ret;
bb691ae4 Kevin Liu 2013-02-01 542

:::::: The code at line 539 was first introduced by commit
:::::: 3bb10f60933e84abfe2be69f60b3486f9b96348b mmc: sdhci-pxav3: fix race between runtime pm and irq

:::::: TO: Jisheng Zhang <[email protected]>
:::::: CC: Ulf Hansson <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (2.12 kB)
.config.gz (52.21 kB)
Download all attachments

2016-04-05 12:44:54

by Adrian Hunter

[permalink] [raw]

Subject: Re: [PATCH] sdhci: wakeup from runtime PM

On 25/03/16 18:05, Ludovic Desroches wrote:
> Hi,
>
> When not using the SDHCI controller, it is logical to save power by suspending
> it. The issue is that SDHCI assumes that the controller is completely disabled.
> It means the only way to wake up on a card event is to have a gpio for the card
> detection (so two pins for the same signal). A possible workaround is to use
> polling but the controller will be resumed/suspended between each attempts.
>
> We have already discussed a long time about this and it seems we don't agree.
> In my opinion, even if I can't disable all clocks, I should use runtime PM
> to save some power.
>
> I propose two patches, one which is a draft to try to solve it at sdhci level
> and one at sdhci-of-at91 level.
>
> Concerning the first one, I don't understand why we need to reject irqs if
> runtime_suspended is true.

The interrupt handler might be called because the interrupt is shared i.e.
the interrupt is for a different device. In that case the host controller
might be off and the registers inaccessible. In that case we cannot even
look at the interrupt register to determine if we were expecting the interrupt.

> Only SDHCI_INT_CARD_INT irq is enabled so why we
> should have other IRQs than this one?

In the case of SDIO Card interrupt, it is delivered via the host controller,
so we have to assume the registers are accessible if we are
runtime-suspended with the SDIO IRQ enabled.

>
> Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
> SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
> solved my issue only by modifying my driver.

I don't mind allowing card detect interrupts while runtime suspended, but we
need a flag so that:
- runtime suspend leaves the insert/remove interrupts enabled
- irq handler knows it can access registers
- irq thread handler knows to runtime resume before doing anything else

But it seems like you need to persuade Ulf first.

2016-04-07 09:11:12

by Ulf Hansson

[permalink] [raw]

Subject: Re: [PATCH] sdhci: wakeup from runtime PM

On 5 April 2016 at 14:40, Adrian Hunter <[email protected]> wrote:
> On 25/03/16 18:05, Ludovic Desroches wrote:
>> Hi,
>>
>> When not using the SDHCI controller, it is logical to save power by suspending
>> it. The issue is that SDHCI assumes that the controller is completely disabled.
>> It means the only way to wake up on a card event is to have a gpio for the card
>> detection (so two pins for the same signal). A possible workaround is to use
>> polling but the controller will be resumed/suspended between each attempts.
>>
>> We have already discussed a long time about this and it seems we don't agree.
>> In my opinion, even if I can't disable all clocks, I should use runtime PM
>> to save some power.
>>
>> I propose two patches, one which is a draft to try to solve it at sdhci level
>> and one at sdhci-of-at91 level.
>>
>> Concerning the first one, I don't understand why we need to reject irqs if
>> runtime_suspended is true.
>
> The interrupt handler might be called because the interrupt is shared i.e.
> the interrupt is for a different device. In that case the host controller
> might be off and the registers inaccessible. In that case we cannot even
> look at the interrupt register to determine if we were expecting the interrupt.
>
>> Only SDHCI_INT_CARD_INT irq is enabled so why we
>> should have other IRQs than this one?
>
> In the case of SDIO Card interrupt, it is delivered via the host controller,
> so we have to assume the registers are accessible if we are
> runtime-suspended with the SDIO IRQ enabled.
>
>>
>> Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
>> SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
>> solved my issue only by modifying my driver.
>
> I don't mind allowing card detect interrupts while runtime suspended, but we
> need a flag so that:
> - runtime suspend leaves the insert/remove interrupts enabled
> - irq handler knows it can access registers

To me, this seem like the wrong way of how to configure wake-ups for
these kind of devices.

I don't think the regular IRQs shall be enabled and the driver
shouldn't assume the registers are accessible without first runtime
resuming the device.

> - irq thread handler knows to runtime resume before doing anything else
>
> But it seems like you need to persuade Ulf first.

For a more thorough explanation to why I don't like, please have a
look at my comment for another related thread on the mmc list.
http://www.spinics.net/lists/linux-mmc/msg36132.html

Kind regards
Uffe

2016-04-07 15:13:07

by Ludovic Desroches

[permalink] [raw]

Subject: Re: [PATCH] sdhci: wakeup from runtime PM

On Thu, Apr 07, 2016 at 11:11:08AM +0200, Ulf Hansson wrote:
> On 5 April 2016 at 14:40, Adrian Hunter <[email protected]> wrote:
> > On 25/03/16 18:05, Ludovic Desroches wrote:
> >> Hi,
> >>
> >> When not using the SDHCI controller, it is logical to save power by suspending
> >> it. The issue is that SDHCI assumes that the controller is completely disabled.
> >> It means the only way to wake up on a card event is to have a gpio for the card
> >> detection (so two pins for the same signal). A possible workaround is to use
> >> polling but the controller will be resumed/suspended between each attempts.
> >>
> >> We have already discussed a long time about this and it seems we don't agree.
> >> In my opinion, even if I can't disable all clocks, I should use runtime PM
> >> to save some power.
> >>
> >> I propose two patches, one which is a draft to try to solve it at sdhci level
> >> and one at sdhci-of-at91 level.
> >>
> >> Concerning the first one, I don't understand why we need to reject irqs if
> >> runtime_suspended is true.
> >
> > The interrupt handler might be called because the interrupt is shared i.e.
> > the interrupt is for a different device. In that case the host controller
> > might be off and the registers inaccessible. In that case we cannot even
> > look at the interrupt register to determine if we were expecting the interrupt.

Ok, I have missed we can get a shared irq.

> >
> >> Only SDHCI_INT_CARD_INT irq is enabled so why we
> >> should have other IRQs than this one?
> >
> > In the case of SDIO Card interrupt, it is delivered via the host controller,
> > so we have to assume the registers are accessible if we are
> > runtime-suspended with the SDIO IRQ enabled.
> >
> >>
> >> Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
> >> SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
> >> solved my issue only by modifying my driver.
> >
> > I don't mind allowing card detect interrupts while runtime suspended, but we
> > need a flag so that:
> > - runtime suspend leaves the insert/remove interrupts enabled
> > - irq handler knows it can access registers
>
> To me, this seem like the wrong way of how to configure wake-ups for
> these kind of devices.

We are definitely not in agreement on this point. It means I have to
rely on polling for card detection. I did it but I had in mind it would
be a temporary workaround while waiting to find a better one.

>
> I don't think the regular IRQs shall be enabled and the driver
> shouldn't assume the registers are accessible without first runtime
> resuming the device.
>
> > - irq thread handler knows to runtime resume before doing anything else
> >
> > But it seems like you need to persuade Ulf first.
>

It will be a difficult task :)

> For a more thorough explanation to why I don't like, please have a
> look at my comment for another related thread on the mmc list.
> http://www.spinics.net/lists/linux-mmc/msg36132.html
>

To facilitate discussion, I copy a part of your explanation here:

> My interpretation of how runtime PM should be deployed, it's s that a
> device shall not process data in the runtime suspend state. To do
> that, it first needs to be runtime resumed. I think this conforms to
> what the runtime PM documentation also recommends in section 2, Device
> Runtime PM Callbacks.

So simply invoking the resume callback when the card detect irq is
received should be in accordance with the PM documentation.

> Moreover I think it's an SoC specific detail whether a driver *can*
> access a device's registers in the runtime suspend state (thus it
> shouldn't do it). Especially, devices may reside in a PM domain, which
> could enter a retention state, because all its devices are runtime
> suspended. Typically for an ARM SoC is that devices looses their
> register's context in such retention state.

Maybe it's a SoC specific detail but that doesn't mean that this detail
should not be taken into account. The PM documentation mentions 'the PM
core regards the device as suspended, which need not mean that it has been
put into a low power state'. It sounds like we can access to the
registers, there is no requirement to disable all the clocks.

I am not arguing for SD card detection wake-up itself but for saving power
when I don't use my SDHCI controller. I boot Linux without a SD card,
the controller is runtime suspended, it seems obvious that the controller
should be resumed when I insert a card. I really want to use runtime PM
because the controller should be most of the time suspended excepting if the
rootfs is on the SD card.

Am I the only one to find that the constraint is too big to use runtime PM?

Regards

Ludovic