[RFC] improve the SDHCI wakeup support.

[RFC] improve the SDHCI wakeup support.

[Giuseppe CAVALLARO]

Hello.

The following patches, currently built against an ST Kernel
"2.6.32", show how to improve the wakeup support in the SDHCI
device driver.

Note:
I'm going to rework them, for example, against the mmc-next
after performing the review process and, obviously, if you
think that they can be actually useful.

Indeed, on a STB, it can be nice to have the capability to
wake-up the system when a card is inserted ;-).

This work allows the SDHCI to wake up the system
on the following events:
 1) Card Interrupts.
 2) Card Insertion.
 3) Card Removal.

To do that the sdhci has to perform some operations
described in the patch named:
  "mmc_sdhci: improve the wake-up support"

A new parameter has been also added to select which wakeup
event has to be used. At any rate, a device based on
the sdchi, e.g. sdhci-pci, can use a default mode (that
can be modified at run-time as well).
In case of the sdhci-pci the "Card Interrupt event" has not
been modified according to the logic behind the recent commit:
  5f619704d18b93869d045abc49e09cdba109b04b

The define MMC_PM_KEEP_POWER has been used to notify that
a device driver (e.g. sdhci-pltfm) want to wakeup the system.
In any case, the logic for programming the HC register is
embedded in the suspend and it's self contained.

In the sdhci-pltfm, it has been also introduced another fix:
see patch named:
 "mmc: sdhci-pltfm calls the sdhci_alloc_host with pdev->dev"
The driver calls the sdhci_alloc_host passing as device pointer
the parent.
Note: parent name is "platform" and dev name is "sdhci.0".
IMO it makes sense to pass the pdev->dev pointer instead of the
parent. This also helps when invoke the device_set_wakeup_capable etc.

This is a piece of output on our ST platforms:

 bash-3.00# echo 2 > /sys/module/sdhci/parameters/wakeup
 bash-3.00# echo standby > /sys/power/state

 PM: Syncing filesystems ... done.
 Freezing user space processes ... (elapsed 0.00 seconds) done.
 Freezing remaining freezable tasks ... (elapsed 0.00 seconds) done.
 [STM][PM] Analyzing the wakeup devices
 [STM][PM] -> device sdhci.0 can wakeup
 [STM][PM] -> device stm-asc.0 can wakeup

[snip]

As shown above, to modify at run time the wakeup event we can do:
 bash-3.00 echo X > /sys/module/sdhci/parameters/wakeup

where X can be:
 0: no wakeup
 1: Card Interrupts
 2: Card Insertion
 3: Card Removal

Hmm, maybe it's worth having more parameters instead of the
wakeup. I mean, something like this (welcome feedback):
 wake_on_card_int
 wake_on_card_ins
 wake_on_card_rem

Welcome advice and feedback as usual.

Regards
Giuseppe

[RFC] improve the SDHCI wakeup support.

[Giuseppe CAVALLARO]

Hello.

The following patches, currently built against an ST Kernel
"2.6.32", show how to improve the wakeup support in the SDHCI
device driver.

Note:
I'm going to rework them, for example, against the mmc-next
after performing the review process and, obviously, if you
think that they can be actually useful.

Indeed, on a STB, it can be nice to have the capability to
wake-up the system when a card is inserted ;-).

This work allows the SDHCI to wake up the system
on the following events:
 1) Card Interrupts.
 2) Card Insertion.
 3) Card Removal.

To do that the sdhci has to perform some operations
described in the patch named:
  "mmc_sdhci: improve the wake-up support"

A new parameter has been also added to select which wakeup
event has to be used. At any rate, a device based on
the sdchi, e.g. sdhci-pci, can use a default mode (that
can be modified at run-time as well).
In case of the sdhci-pci the "Card Interrupt event" has not
been modified according to the logic behind the recent commit:
  5f619704d18b93869d045abc49e09cdba109b04b

The define MMC_PM_KEEP_POWER has been used to notify that
a device driver (e.g. sdhci-pltfm) want to wakeup the system.
In any case, the logic for programming the HC register is
embedded in the suspend and it's self contained.

To modify at run time the wakeup event we can do:
 $ echo X > /sys/module/sdhci/parameters/wakeup
where X can be:
 0: no wakeup
 1: Card Interrupts
 2: Card Insertion
 3: Card Removal

Hmm, maybe it's worth having more parameters instead of the
wakeup. I mean, something like this (welcome feedback):
 wake_on_card_int
 wake_on_card_ins
 wake_on_card_rem

Welcome advice and feedback as usual.

Regards
Giuseppe

[PATCH (1/5)] mmc: sdhci-pltfm calls the sdhci_alloc_host with pdev->dev

[Giuseppe CAVALLARO]

ST targets use the sdhci-pltfm driver but there are some
problems when re-insert the driver on our platforms.
Within sdhci-pltfm d.d. the pdata->init invokes own
platform function to claim some resource (based on devres):
see the example code below:

static int mmc_pad_resources(struct sdhci_host *sdhci)
{
    if (!devm_stm_pad_claim(sdhci->mmc->parent, &stx7105_mmc_pad_config,
                            dev_name(sdhci->mmc->parent)))
              return -ENODEV;
     return 0;
}

static struct sdhci_pltfm_data stx7105_mmc_platform_data = {
                .init = mmc_pad_resources,
                .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
};

static struct platform_device stx7105_mmc_device = {
                .name = "sdhci",
                .id = 0,
[snip]
                .dev = {
                      .platform_data = &stx7105_mmc_platform_data,
                }
};

void __init stx7105_configure_mmc(void)
{
 ...
        platform_device_register(&stx7105_mmc_device);
}

As soon as the sdhci-pltfm is removed from the system
the sdhci resources previously allocate are not released
and the probe fails when re-insert the module.

The platform driver calls the sdhci_alloc_host passing as device pointer
the parent.
Note: parent name is "platform" and dev name is "sdhci.0".
IMO it makes sense to pass the pdev->dev pointer instead of the parent.

In the end, this also fixes the problem described above and I can insert/remove
the sdhci-pltfm driver several times without failures.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Reviewed-by: Francesco Virlinzi <francesco.virlinzi@st.com>
---
 drivers/mmc/host/sdhci-pltfm.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 78a8f7a..4e881a5 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -65,11 +65,7 @@ static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Invalid iomem size. You may "
 			"experience problems.\n");
 
-	if (pdev->dev.parent)
-		host = sdhci_alloc_host(pdev->dev.parent, 0);
-	else
-		host = sdhci_alloc_host(&pdev->dev, 0);
-
+	host = sdhci_alloc_host(&pdev->dev, 0);
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
 		goto err;
-- 
1.5.5.6

[PATCH 2/5] mmc: do not clear the host->pm_flags when suspend

[Giuseppe CAVALLARO]

HC driver will be able to use the pm_flags to
undestand if the system can be woken-up by the driver.
So the mmc_suspend_host hasn't to reset this field
in the host structure.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/mmc/core/core.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 569e94d..6d2d6e4 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1276,7 +1276,6 @@ int mmc_suspend_host(struct mmc_host *host)
 			mmc_claim_host(host);
 			mmc_detach_bus(host);
 			mmc_release_host(host);
-			host->pm_flags = 0;
 			err = 0;
 		}
 	}
-- 
1.5.5.6

[PATCH 3/5] mmc_sdhci: improve the wake-up support

[Giuseppe CAVALLARO]

This patch reworks the wake-up support in the sdhci driver.
It is now able to wake-up the system on: Card interrupts,
Card insertion and Card removal events.
This has been tested on ST STB where can make sense to
wake-up the box as soon as a card is inserted.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/mmc/host/sdhci.c |  110 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/mmc/host/sdhci.h |    3 +-
 include/linux/mmc/pm.h   |    9 +++-
 3 files changed, 109 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 69c80c9..8db9757 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -36,7 +36,8 @@
 #define SDHCI_USE_LEDS_CLASS
 #endif
 
-static unsigned int debug_quirks = 0;
+static unsigned int debug_quirks;
+static unsigned int wakeup;
 
 static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *);
 static void sdhci_finish_data(struct sdhci_host *);
@@ -1595,6 +1596,83 @@ out:
 
 #ifdef CONFIG_PM
 
+static void sdhci_can_wakeup(struct sdhci_host *host)
+{
+	struct device *dev = host->mmc->parent;
+
+	/* SDHCI is capable to wakeup the system */
+	device_set_wakeup_capable(dev, 1);
+
+	/* Enable wakeup */
+	device_set_wakeup_enable(dev, 1);
+	enable_irq_wake(host->irq);
+}
+
+/*
+ * According to the SD SPEC to wake up the system
+ * the HC has to be programmed as below:
+ * - SD Bus power: ON
+ * - SD clock: Stop
+ * - Internal clock: Oscillate
+ * - Data transfer witdh: 0
+ *
+ * While suspending we have to:
+ * 1) Set BUS width to 1
+ * 2) Mask the Signal Interrupt register
+ * 3) Clean the Signal Interrupt register
+ * 4) Set the enable bits of each factor to 1 in the Wakeup
+ *    control register
+ * 5) Set the bits of Normal Interrupt Status Enable register
+ *    to use wakeup.
+ */
+static void sdhci_set_wakeup(struct sdhci_host *host)
+{
+	u16 clk;
+	u8 pwr;
+	mmc_pm_flag_t wakeflag = 0;
+
+	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	clk &= ~SDHCI_CLOCK_CARD_EN;
+	clk |= SDHCI_CLOCK_INT_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+	pwr = sdhci_readb(host, SDHCI_POWER_CONTROL);
+	pwr |= SDHCI_POWER_ON;
+	sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
+
+	sdhci_mask_irqs(host, SDHCI_INT_NORMAL_MASK |
+			SDHCI_INT_ERROR_MASK);
+	sdhci_writel(host, 0, SDHCI_INT_STATUS);
+	sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
+	sdhci_writel(host, SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT |
+		     SDHCI_INT_CARD_INT, SDHCI_INT_ENABLE);
+
+	/*
+	 * If a wake up event is set by user, then force the pm_flags
+	 * to use it (independently of the previous setting made in the d.d.
+	 */
+	if (wakeup == 1)
+		host->mmc->pm_flags |= MMC_PM_WAKE_SDIO_IRQ;
+	else if (wakeup == 2)
+		host->mmc->pm_flags |= MMC_PM_WAKE_SDIO_CARD_INS;
+	else if (wakeup == 3)
+		host->mmc->pm_flags |= MMC_PM_WAKE_SDIO_CARD_REM;
+
+	/*
+	 * Program the Wakeup Control register according to the event selected.
+	 * If there is no event (SDHCI_WAKE_UP_CONTROL = 0x0) so the device
+	 * won't be able to wake up the system.
+	 */
+	if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
+		wakeflag |= SDHCI_WAKE_ON_INT;
+	if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_CARD_INS)
+		wakeflag |= SDHCI_WAKE_ON_INSERT;
+	if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_CARD_REM)
+		wakeflag |= SDHCI_WAKE_ON_REMOVE;
+
+	sdhci_enable_irq_wakeups(host, wakeflag);
+}
+
 int sdhci_suspend_host(struct sdhci_host *host, pm_message_t state)
 {
 	int ret;
@@ -1605,7 +1683,10 @@ int sdhci_suspend_host(struct sdhci_host *host, pm_message_t state)
 	if (ret)
 		return ret;
 
-	free_irq(host->irq, host);
+	if (host->mmc->pm_flags & MMC_PM_KEEP_POWER)
+		sdhci_set_wakeup(host);
+	else
+		free_irq(host->irq, host);
 
 	return 0;
 }
@@ -1621,12 +1702,13 @@ int sdhci_resume_host(struct sdhci_host *host)
 			host->ops->enable_dma(host);
 	}
 
-	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
-			  mmc_hostname(host->mmc), host);
-	if (ret)
-		return ret;
-
-	sdhci_init(host, (host->mmc->pm_flags & MMC_PM_KEEP_POWER));
+	if (!(host->mmc->pm_flags & MMC_PM_KEEP_POWER)) {
+		ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
+				  mmc_hostname(host->mmc), host);
+		if (ret)
+			return ret;
+	}
+	sdhci_init(host, host->mmc->pm_flags);
 	mmiowb();
 
 	ret = mmc_resume_host(host->mmc);
@@ -1637,16 +1719,16 @@ int sdhci_resume_host(struct sdhci_host *host)
 
 EXPORT_SYMBOL_GPL(sdhci_resume_host);
 
-void sdhci_enable_irq_wakeups(struct sdhci_host *host)
+void sdhci_enable_irq_wakeups(struct sdhci_host *host, unsigned int mode)
 {
 	u8 val;
+
 	val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
-	val |= SDHCI_WAKE_ON_INT;
+	val |= mode;
 	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
 }
 
 EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
-
 #endif /* CONFIG_PM */
 
 /*****************************************************************************\
@@ -1928,6 +2010,10 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	sdhci_enable_card_detection(host);
 
+#ifdef CONFIG_PM
+	if (host->mmc->pm_flags & MMC_PM_KEEP_POWER)
+		sdhci_can_wakeup(host);
+#endif
 	return 0;
 
 #ifdef SDHCI_USE_LEDS_CLASS
@@ -2021,9 +2107,11 @@ module_init(sdhci_drv_init);
 module_exit(sdhci_drv_exit);
 
 module_param(debug_quirks, uint, 0444);
+module_param(wakeup, uint, S_IRUGO | S_IWUSR);
 
 MODULE_AUTHOR("Pierre Ossman <pierre@ossman.eu>");
 MODULE_DESCRIPTION("Secure Digital Host Controller Interface core driver");
 MODULE_LICENSE("GPL");
 
 MODULE_PARM_DESC(debug_quirks, "Force certain quirks.");
+MODULE_PARM_DESC(wakeup, "Wakeup:0:disable,1:Card Int,2:Card Ins,3: Card Rem.");
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index f939371..cd70d00 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -312,7 +312,8 @@ extern void sdhci_remove_host(struct sdhci_host *host, int dead);
 #ifdef CONFIG_PM
 extern int sdhci_suspend_host(struct sdhci_host *host, pm_message_t state);
 extern int sdhci_resume_host(struct sdhci_host *host);
-extern void sdhci_enable_irq_wakeups(struct sdhci_host *host);
+extern void sdhci_enable_irq_wakeups(struct sdhci_host *host,
+				     unsigned int mode);
 #endif
 
 #endif /* __SDHCI_HW_H */
diff --git a/include/linux/mmc/pm.h b/include/linux/mmc/pm.h
index d37aac4..31df7f2 100644
--- a/include/linux/mmc/pm.h
+++ b/include/linux/mmc/pm.h
@@ -24,7 +24,14 @@
 
 typedef unsigned int mmc_pm_flag_t;
 
-#define MMC_PM_KEEP_POWER	(1 << 0)	/* preserve card power during suspend */
+/* Preserve card power during suspend. It is also used for notifying
+ * that the device can wakeup the system. This can be done, for example,
+ * by either card interrupt enent or card insertion event or card
+ * removal event.
+ */
+#define MMC_PM_KEEP_POWER	(1 << 0)
 #define MMC_PM_WAKE_SDIO_IRQ	(1 << 1)	/* wake up host system on SDIO IRQ assertion */
+#define MMC_PM_WAKE_SDIO_CARD_INS	(1 << 2) /* wake up on card insertion */
+#define MMC_PM_WAKE_SDIO_CARD_REM	(1 << 3) /* wake up on card removal  */
 
 #endif
-- 
1.5.5.6

[PATCH 4/5] mmc: sdhci-pci invokes the sdhci_enable_irq_wakeups with SDHCI_WAKE_ON_INT

[Giuseppe CAVALLARO]

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/mmc/host/sdhci-pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 59f3de5..8634fb1 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -524,7 +524,7 @@ static int sdhci_pci_suspend (struct pci_dev *pdev, pm_message_t state)
 
 		slot_pm_flags = slot->host->mmc->pm_flags;
 		if (slot_pm_flags & MMC_PM_WAKE_SDIO_IRQ)
-			sdhci_enable_irq_wakeups(slot->host);
+			sdhci_enable_irq_wakeups(slot->host, SDHCI_WAKE_ON_INT);
 
 		pm_flags |= slot_pm_flags;
 	}
-- 
1.5.5.6

[PATCH 5/5] mmc: sdhci-pltfm can wake up.

[Giuseppe CAVALLARO]

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/mmc/host/sdhci-pltfm.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 4e881a5..6ea32e1 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -100,6 +100,8 @@ static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
 			goto err_plat_init;
 	}
 
+	host->mmc->pm_flags = MMC_PM_KEEP_POWER;
+
 	ret = sdhci_add_host(host);
 	if (ret)
 		goto err_add_host;
-- 
1.5.5.6

Re: [PATCH 2/5] mmc: do not clear the host->pm_flags when suspend

[Ohad Ben-Cohen]

Hi Giuseppe,

On Wed, Nov 10, 2010 at 5:28 PM, Giuseppe CAVALLARO
<peppe.cavallaro@st.com> wrote:
> HC driver will be able to use the pm_flags to
> undestand if the system can be woken-up by the driver.
> So the mmc_suspend_host hasn't to reset this field
> in the host structure.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/mmc/core/core.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 569e94d..6d2d6e4 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1276,7 +1276,6 @@ int mmc_suspend_host(struct mmc_host *host)
>                        mmc_claim_host(host);
>                        mmc_detach_bus(host);
>                        mmc_release_host(host);
> -                       host->pm_flags = 0;

This is elapsed because the card is removed at this point (e.g. SDIO
returned -ENOSYS).

Why would you want to use flags, set by a function driver of an SDIO
card, that was just removed ?

>                        err = 0;
>                }
>        }
> --
> 1.5.5.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/5] mmc: do not clear the host->pm_flags when suspend

[Peppe CAVALLARO]

Hi Ohad,

On 11/10/2010 9:28 PM, Ohad Ben-Cohen wrote:
>
> Hi Giuseppe,
>
> On Wed, Nov 10, 2010 at 5:28 PM, Giuseppe CAVALLARO
> <peppe.cavallaro@st.com> wrote:
> > HC driver will be able to use the pm_flags to
> > undestand if the system can be woken-up by the driver.
> > So the mmc_suspend_host hasn't to reset this field
> > in the host structure.
> >
> > Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> > ---
> >  drivers/mmc/core/core.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 569e94d..6d2d6e4 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1276,7 +1276,6 @@ int mmc_suspend_host(struct mmc_host *host)
> >                        mmc_claim_host(host);
> >                        mmc_detach_bus(host);
> >                        mmc_release_host(host);
> > -                       host->pm_flags = 0;
>
> This is elapsed because the card is removed at this point (e.g. SDIO
> returned -ENOSYS).
>
> Why would you want to use flags, set by a function driver of an SDIO
> card, that was just removed ?
>
The define MMC_PM_KEEP_POWER has been used to notify that
a device driver (e.g. sdhci-pltfm) want to wakeup the system.
It is set in the pm_flags. When the sdhci suspend is invoked, it can
call the mmc_suspend_host without free the interrupt if
MMC_PM_KEEP_POWER is set.
Then the sdhci_set_wakeup programs the HC to be able to wake up the system.

When the resume is invoked the pm_flags is used to avoid to request the irq.
For this reason I removed the inst "host->pm_flags = 0;" from the
mmc_suspend_host
function. Maybe I could not remove it but add a check if the driver
wants to wakeup
the system and the MMC_PM_KEEP_POWER is set. What do you think?

Thanks
Regards,
Peppe
>
>
> >                        err = 0;
> >                }
> >        }
> > --
> > 1.5.5.6
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
>

Re: [PATCH 2/5] mmc: do not clear the host->pm_flags when suspend

[Ohad Ben-Cohen]

Hi Peppe,

On Thu, Nov 11, 2010 at 9:24 AM, Peppe CAVALLARO <peppe.cavallaro@st.com> wrote:
>> > -                       host->pm_flags = 0;
>>
>> This is elapsed because the card is removed at this point (e.g. SDIO
>> returned -ENOSYS).
>>
>> Why would you want to use flags, set by a function driver of an SDIO
>> card, that was just removed ?
>>
> The define MMC_PM_KEEP_POWER has been used to notify that
> a device driver (e.g. sdhci-pltfm) want to wakeup the system.

MMC_PM_KEEP_POWER indicates that a card wishes to preserve its power
during system suspend; SDIO function drivers set it, if desired, and
if the host supports it.

If you permanently set it in sdhci-pltfm, you completely change the
interface. Drivers will be very surprised with such a default power-on
behavior.

> It is set in the pm_flags. When the sdhci suspend is invoked, it can
> call the mmc_suspend_host without free the interrupt if
> MMC_PM_KEEP_POWER is set.
> Then the sdhci_set_wakeup programs the HC to be able to wake up the system.

MMC_PM_KEEP_POWER does not necessarily mean the user wants the system
to wake up on any SDIO events (e.g. it might be desired to keep a card
powered in a low power mode, without any wake-up expectancies, just to
be able to bring it up very quickly when the system resumes).

> When the resume is invoked the pm_flags is used to avoid to request the irq.
> For this reason I removed the inst "host->pm_flags = 0;" from the
> mmc_suspend_host
> function. Maybe I could not remove it but add a check if the driver
> wants to wakeup

Who is this driver ? Host controller ? I don't think it should decide
whether the system will wake up or not on these events. Host
controller should merely provide capabilities (by means of pm_caps),
and let the upper layers choose.

> the system and the MMC_PM_KEEP_POWER is set. What do you think?

Beyond MMC_PM_WAKE_SDIO_IRQ, which is already established, you seem to
add two additional wake up events: card insertion and card removal.

The insertion event, at least (still need to think about the removal
one), have little to do with the SDIO function driver itself (e.g., if
the system is suspended, and the user inserts a card, obviously there
is no driver loaded for it yet that could have set pm_flags).

These events seem to be more system-wide and not driver-specific: does
the user want the host to wake up on card insertion or not ?

One idea that comes to mind is to use the PM core's wakeup sysfs
device attribute to control this; this way the user will be able to
directly enable/disable these wakeup events.

It might still make sense to use pm_flags and pm_caps here, and then
the user will manipulate the sysfs entries of the mmc host class_dev
rather than manipulating the host controller device directly. Not sure
how big an advantage is that, though.



>
> Thanks
> Regards,
> Peppe
>>
>>
>> >                        err = 0;
>> >                }
>> >        }
>> > --
>> > 1.5.5.6
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>>
>

Re: [PATCH 2/5] mmc: do not clear the host->pm_flags when suspend

[Peppe CAVALLARO]

Hi Ohad,

On 11/12/2010 12:45 AM, Ohad Ben-Cohen wrote:
>
> MMC_PM_KEEP_POWER indicates that a card wishes to preserve its power
> during system suspend; SDIO function drivers set it, if desired, and
> if the host supports it.
>
> If you permanently set it in sdhci-pltfm, you completely change the
> interface. Drivers will be very surprised with such a default power-on
> behavior.
>

Yes.

> > It is set in the pm_flags. When the sdhci suspend is invoked, it can
> > call the mmc_suspend_host without free the interrupt if
> > MMC_PM_KEEP_POWER is set.
> > Then the sdhci_set_wakeup programs the HC to be able to wake up the
> system.
>
> MMC_PM_KEEP_POWER does not necessarily mean the user wants the system
> to wake up on any SDIO events (e.g. it might be desired to keep a card
> powered in a low power mode, without any wake-up expectancies, just to
> be able to bring it up very quickly when the system resumes).
>

This is also true. I wanted to use this macro to indicate that the
wakeup is possible.
I mean, if the driver preserves the power could be wakeup capable.
>
> > When the resume is invoked the pm_flags is used to avoid to request
> the irq.
> > For this reason I removed the inst "host->pm_flags = 0;" from the
> > mmc_suspend_host
> > function. Maybe I could not remove it but add a check if the driver
> > wants to wakeup
>
> Who is this driver ? Host controller ? I don't think it should decide
> whether the system will wake up or not on these events. Host
> controller should merely provide capabilities (by means of pm_caps),
> and let the upper layers choose.
>
IIUC, the driver should set its capabilities in pm_caps and I could use
the device_can_wakeup
in suspend and resume;

I mean, in the suspend, for example, I should use:
  if (!(device_may_wake_up(host->mmc->parent)))
           ...
instead of
  if (!(host->mmc->pm_flags & MMC_PM_KEEP_POWER)) {
   ...
>
> > the system and the MMC_PM_KEEP_POWER is set. What do you think?
>
> Beyond MMC_PM_WAKE_SDIO_IRQ, which is already established, you seem to
> add two additional wake up events: card insertion and card removal.
>
> The insertion event, at least (still need to think about the removal
> one), have little to do with the SDIO function driver itself (e.g., if
> the system is suspended, and the user inserts a card, obviously there
> is no driver loaded for it yet that could have set pm_flags).
>

Thanks! Now I understand why the pm_flags is clear in the mmc suspend.
Please see the following test scenario.
I suspend the STB, then I want to to remove the card (and the box is
always in power down).
In the end, I want to reinsert the card and wake up the system ;-).
In this case, the pm_flags is cleared (when I enter in the sdhci resume).
If I suspend the box again, I won't be able to get any info about the
wakeup event has to
be triggered. Am I missing anything?

So I've not clear if I should use the pm_caps instead of pm_flags.

> These events seem to be more system-wide and not driver-specific: does
> the user want the host to wake up on card insertion or not ?
>

Yes user does.

I wanted to actually use:

MMC_PM_WAKE_SDIO_IRQ
  for programming the HC to wakeup on card interrupt
MMC_PM_WAKE_SDIO_CARD_INS
  for programming the HC to wakeup on card insertion
MMC_PM_WAKE_SDIO_CARD_REM
  for programming the HC to wakeup on card removal


> One idea that comes to mind is to use the PM core's wakeup sysfs
> device attribute to control this; this way the user will be able to
> directly enable/disable these wakeup events.
>
AFAIK, I can only enable/disable wakeup by using:
     /sys/devices/platform/sdhci.0/power/wakeup

I need to pass which events has to wakeup the HC (card int, insert, rem).
Indeed, I wanted to enable/disable them at user level by using the new
option:

 bash-3.00 echo X > /sys/module/sdhci/parameters/wakeup

where X could be:
 0: no wakeup
 1: Card Interrupts
 2: Card Insertion
 3: Card Removal

Maybe it's worth having more parameters instead of the
wakeup. I mean, something like this (welcome feedback):
 wake_on_card_int
 wake_on_card_ins
 wake_on_card_re

What do you think?

> It might still make sense to use pm_flags and pm_caps here, and then
> the user will manipulate the sysfs entries of the mmc host class_dev
> rather than manipulating the host controller device directly. Not sure
> how big an advantage is that, though.
>

I think you are right.

Welcome other feedback so I'll rework these patch and align them to
mmc-next as Wolfram suggested.

Many thanks for your feedback and details.

Regards
Peppe

Re: [PATCH 2/5] mmc: do not clear the host->pm_flags when suspend

[Nicolas Pitre]

On Wed, 10 Nov 2010, Giuseppe CAVALLARO wrote:

> HC driver will be able to use the pm_flags to
> undestand if the system can be woken-up by the driver.
> So the mmc_suspend_host hasn't to reset this field
> in the host structure.
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/mmc/core/core.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 569e94d..6d2d6e4 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1276,7 +1276,6 @@ int mmc_suspend_host(struct mmc_host *host)
>  			mmc_claim_host(host);
>  			mmc_detach_bus(host);
>  			mmc_release_host(host);
> -			host->pm_flags = 0;
>  			err = 0;
>  		}
>  	}

This is wrong.  The host->pm_flags are set by SDIO function drivers to 
indicate they want the host controller to keep card power on when the 
host system is going to sleep.  In this case, the card has been removed, 
and therefore any flag is invalid and should be cleared.


Nicolas

Re: [PATCH (1/5)] mmc: sdhci-pltfm calls the sdhci_alloc_host with pdev->dev

[Wolfram Sang]

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

Hi,

thanks for the work in general, just...

> -	if (pdev->dev.parent)
> -		host = sdhci_alloc_host(pdev->dev.parent, 0);
> -	else
> -		host = sdhci_alloc_host(&pdev->dev, 0);
> -
> +	host = sdhci_alloc_host(&pdev->dev, 0);
>  	if (IS_ERR(host)) {
>  		ret = PTR_ERR(host);
>  		goto err;

NACK. This part looks different in current mainline (and for a reason).
Removing the dev.parent-branch will break some PCI-based solutions.

I think you should first rebase the series to mmc-next and then ask for
review. It is too confusing for reviewers otherwise. At least, I will
stop here.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH (1/5)] mmc: sdhci-pltfm calls the sdhci_alloc_host with pdev->dev

[Peppe CAVALLARO]

Hi Wolfram

On 11/10/2010 4:43 PM, Wolfram Sang wrote:
> Hi,
>
> thanks for the work in general, just...
>
>> -	if (pdev->dev.parent)
>> -		host = sdhci_alloc_host(pdev->dev.parent, 0);
>> -	else
>> -		host = sdhci_alloc_host(&pdev->dev, 0);
>> -
>> +	host = sdhci_alloc_host(&pdev->dev, 0);
>>  	if (IS_ERR(host)) {
>>  		ret = PTR_ERR(host);
>>  		goto err;
> NACK. This part looks different in current mainline (and for a reason).
Yes, as I had written in the first email I built these patches against
our Kernel 2.6.32.
I wanted to align them to the mmc-next after clarifying some doubts I had.
For example the wakeup option used for selecting at runtime the wakeup mode.

> Removing the dev.parent-branch will break some PCI-based solutions.
Hmm, I suspected this :-(. Unfortunately I need to not pass the parent
for the problem described in the patch.
How to proceed? Do I have to re-introduce the sdhci-stm driver?
> I think you should first rebase the series to mmc-next and then ask for
> review. It is too confusing for reviewers otherwise. At least, I will
> stop here.
At any rate, no problem to re-base the patches to the mmc-next.I'll do
it soon!

Regards
Peppe
> Kind regards,
>
>    Wolfram
>

Re: [PATCH (1/5)] mmc: sdhci-pltfm calls the sdhci_alloc_host with pdev->dev

[Wolfram Sang]

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

> > Removing the dev.parent-branch will break some PCI-based solutions.
> Hmm, I suspected this :-(. Unfortunately I need to not pass the parent
> for the problem described in the patch.
> How to proceed? Do I have to re-introduce the sdhci-stm driver?

The current mainline code does not work for you?

> > I think you should first rebase the series to mmc-next and then ask for
> > review. It is too confusing for reviewers otherwise. At least, I will
> > stop here.
> At any rate, no problem to re-base the patches to the mmc-next.I'll do
> it soon!

Great!

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH (1/5)] mmc: sdhci-pltfm calls the sdhci_alloc_host with pdev->dev

[Peppe CAVALLARO]

Hi Wolfram,

On 11/11/2010 8:48 AM, Wolfram Sang wrote:

>>> Removing the dev.parent-branch will break some PCI-based solutions.
>> Hmm, I suspected this :-(. Unfortunately I need to not pass the parent
>> for the problem described in the patch.
>> How to proceed? Do I have to re-introduce the sdhci-stm driver?
> The current mainline code does not work for you?\
your commit 4b711cb13843f5082e82970dd1e8031383134a65
resolves the issue I'm facing.

>>> I think you should first rebase the series to mmc-next and then ask for
>>> review. It is too confusing for reviewers otherwise. At least, I will
>>> stop here.
>> At any rate, no problem to re-base the patches to the mmc-next.I'll do
>> it soon!
> Great!
I'll resend all without this patch.

Many thanks!
Peppe

> Kind regards,
>
>    Wolfram
>

Re: [RFC] improve the SDHCI wakeup support.

[Peppe CAVALLARO]

Hello

On 11/10/2010 4:28 PM, Giuseppe CAVALLARO wrote:
>
> Hello.
>
> The following patches, currently built against an ST Kernel
> "2.6.32", show how to improve the wakeup support in the SDHCI
> device driver.
>
> Note:
> I'm going to rework them, for example, against the mmc-next
> after performing the review process and, obviously, if you
> think that they can be actually useful.
>
> Indeed, on a STB, it can be nice to have the capability to
> wake-up the system when a card is inserted ;-).
>
> This work allows the SDHCI to wake up the system
> on the following events:
>  1) Card Interrupts.
>  2) Card Insertion.
>  3) Card Removal.
>
> To do that the sdhci has to perform some operations
> described in the patch named:
>   "mmc_sdhci: improve the wake-up support"
>
> A new parameter has been also added to select which wakeup
> event has to be used. At any rate, a device based on
> the sdchi, e.g. sdhci-pci, can use a default mode (that
> can be modified at run-time as well).
> In case of the sdhci-pci the "Card Interrupt event" has not
> been modified according to the logic behind the recent commit:
>   5f619704d18b93869d045abc49e09cdba109b04b
>
> The define MMC_PM_KEEP_POWER has been used to notify that
> a device driver (e.g. sdhci-pltfm) want to wakeup the system.
> In any case, the logic for programming the HC register is
> embedded in the suspend and it's self contained.
>
> In the sdhci-pltfm, it has been also introduced another fix:
> see patch named:
>  "mmc: sdhci-pltfm calls the sdhci_alloc_host with pdev->dev"
> The driver calls the sdhci_alloc_host passing as device pointer
> the parent.
> Note: parent name is "platform" and dev name is "sdhci.0".
> IMO it makes sense to pass the pdev->dev pointer instead of the
> parent. This also helps when invoke the device_set_wakeup_capable etc.
>
> This is a piece of output on our ST platforms:
>
>  bash-3.00# echo 2 > /sys/module/sdhci/parameters/wakeup
>  bash-3.00# echo standby > /sys/power/state
>
>  PM: Syncing filesystems ... done.
>  Freezing user space processes ... (elapsed 0.00 seconds) done.
>  Freezing remaining freezable tasks ... (elapsed 0.00 seconds) done.
>  [STM][PM] Analyzing the wakeup devices
>  [STM][PM] -> device sdhci.0 can wakeup
>  [STM][PM] -> device stm-asc.0 can wakeup
>
> [snip]
>
> As shown above, to modify at run time the wakeup event we can do:
>  bash-3.00 echo X > /sys/module/sdhci/parameters/wakeup
>
> where X can be:
>  0: no wakeup
>  1: Card Interrupts
>  2: Card Insertion
>  3: Card Removal
>
> Hmm, maybe it's worth having more parameters instead of the
> wakeup. I mean, something like this (welcome feedback):
>  wake_on_card_int
>  wake_on_card_ins
>  wake_on_card_rem
>
> Welcome advice and feedback as usual.
>

I've reworked the patches, I sent the mailing list last week, now built
against the mmc-next Git repository.
I've removed some piece of the code commented by yourself (in this first
thread of emails) and re-generated a single path (marked as V2).

Many thanks for your advice and welcome feedback on the new patch.

Regards
Peppe