Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331950 > unrolled thread

[PATCH] mmc: sdhci-of-at91: fix card detect when using runtime PM

Back to article view | Back to linux.kernel

Contents

  [PATCH] mmc: sdhci-of-at91: fix card detect when using runtime PM Ludovic Desroches <ludovic.desroches@atmel.com> - 2016-02-11 14:50 +0100
    Re: [PATCH] mmc: sdhci-of-at91: fix card detect when using runtime PM Ulf Hansson <ulf.hansson@linaro.org> - 2016-02-11 16:20 +0100
      Re: [PATCH] mmc: sdhci-of-at91: fix card detect when using runtime PM Ludovic Desroches <ludovic.desroches@atmel.com> - 2016-02-12 09:40 +0100
        Re: [PATCH] mmc: sdhci-of-at91: fix card detect when using runtime PM Ulf Hansson <ulf.hansson@linaro.org> - 2016-02-12 12:10 +0100
          Re: [PATCH] mmc: sdhci-of-at91: fix card detect when using runtime PM Ludovic Desroches <ludovic.desroches@atmel.com> - 2016-02-12 13:10 +0100
            [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm Ludovic Desroches <ludovic.desroches@atmel.com> - 2016-02-13 11:00 +0100
              Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm Ulf Hansson <ulf.hansson@linaro.org> - 2016-02-16 15:40 +0100
                Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime  pm Ludovic Desroches <ludovic.desroches@atmel.com> - 2016-02-16 16:30 +0100
                  Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime  pm Ludovic Desroches <ludovic.desroches@atmel.com> - 2016-02-17 11:40 +0100

#1331950 — [PATCH] mmc: sdhci-of-at91: fix card detect when using runtime PM

Add quirk broken card detection to enable card detection polling. It is
a short term solution until reworking PM stuff.

If the card detect signal is connected to the sdhci controller and not a
gpio, when runtime PM suspend happens, we have no way to wake up on a card
detect event since these irqs are no more enabled.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
Fixes: f5f17813ae9b ("mmc: sdhci-of-at91: add PM support")
---
 drivers/mmc/host/sdhci-of-at91.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 9cb86fb..efec736 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -45,6 +45,7 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
 
 static const struct sdhci_pltfm_data soc_data_sama5d2 = {
 	.ops = &sdhci_at91_sama5d2_ops,
+	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
 	.quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
 };
 
-- 
2.7.0

[toc] | [next] | [standalone]

#1332135

On 11 February 2016 at 14:48, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> Add quirk broken card detection to enable card detection polling. It is
> a short term solution until reworking PM stuff.
>
> If the card detect signal is connected to the sdhci controller and not a
> gpio, when runtime PM suspend happens, we have no way to wake up on a card
> detect event since these irqs are no more enabled.
>
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> Fixes: f5f17813ae9b ("mmc: sdhci-of-at91: add PM support")
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index 9cb86fb..efec736 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -45,6 +45,7 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
>
>  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
>         .ops = &sdhci_at91_sama5d2_ops,
> +       .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
>         .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>  };
>
> --
> 2.7.0
>

According to the below commit, SDHCI_QUIRK_BROKEN_CARD_DETECTION was
invented because of unreliable card detection mechanism inside the
sdhci controller.
Therefore it required polling to be used, but also to make ->get_cd()
to always return 1 in these cases.

Although, as I understand it that's not the case here. You can still
rely on card detection to work, but as you don't have wakeups you
can't fully make use of card detect, when combined with runtime PM.
I am not sure we should add more users of
SDHCI_QUIRK_BROKEN_CARD_DETECTION, especially since in this case it's
not reflecting the capability of the hardware.

Can't we think of another way?

Kind regards
Uffe


commit 68d1fb7e229c6f95be4fbbe3eb46b24e41184924
Author: Anton Vorontsov <avorontsov@ru.mvista.com>
sdhci: Add support for card-detection polling

This patch adds SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk. When specified,
sdhci driver will set MMC_CAP_NEEDS_POLL MMC host capability, and won't
enable card insert/remove interrupts.

This is needed for hosts with unreliable card detection, such as FSL
eSDHC. The original eSDHC driver was tring to "debounce" card-detection
IRQs by reading present state and disabling particular interrupts. But
with this debouncing scheme I noticed that sometimes we miss card
insertion/removal events.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>

[toc] | [prev] | [next] | [standalone]

#1332585

On Thu, Feb 11, 2016 at 04:10:54PM +0100, Ulf Hansson wrote:
> On 11 February 2016 at 14:48, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > Add quirk broken card detection to enable card detection polling. It is
> > a short term solution until reworking PM stuff.
> >
> > If the card detect signal is connected to the sdhci controller and not a
> > gpio, when runtime PM suspend happens, we have no way to wake up on a card
> > detect event since these irqs are no more enabled.
> >
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > Fixes: f5f17813ae9b ("mmc: sdhci-of-at91: add PM support")
> > ---
> >  drivers/mmc/host/sdhci-of-at91.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> > index 9cb86fb..efec736 100644
> > --- a/drivers/mmc/host/sdhci-of-at91.c
> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > @@ -45,6 +45,7 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
> >
> >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> >         .ops = &sdhci_at91_sama5d2_ops,
> > +       .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
> >         .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
> >  };
> >
> > --
> > 2.7.0
> >
> 
> According to the below commit, SDHCI_QUIRK_BROKEN_CARD_DETECTION was
> invented because of unreliable card detection mechanism inside the
> sdhci controller.
> Therefore it required polling to be used, but also to make ->get_cd()
> to always return 1 in these cases.
> 
> Although, as I understand it that's not the case here. You can still
> rely on card detection to work, but as you don't have wakeups you
> can't fully make use of card detect, when combined with runtime PM.
> I am not sure we should add more users of
> SDHCI_QUIRK_BROKEN_CARD_DETECTION, especially since in this case it's
> not reflecting the capability of the hardware.
> 
> Can't we think of another way?

Sorry but I am not sure to understand. In the previous thread, you told
me to use MMC_CAP_NEEDS_POLL which is set if we have
SDHCI_QUIRK_BROKEN_CARD_DETECTION. I was not confortable to do this
because as you say it is not reflecting the capability of the hardware.

Do you mean that I can simply add MMC_CAP_NEEDS_POLL after sdhci_add_host()?

Regards

Ludovic

> 
> Kind regards
> Uffe
> 
> 
> commit 68d1fb7e229c6f95be4fbbe3eb46b24e41184924
> Author: Anton Vorontsov <avorontsov@ru.mvista.com>
> sdhci: Add support for card-detection polling
> 
> This patch adds SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk. When specified,
> sdhci driver will set MMC_CAP_NEEDS_POLL MMC host capability, and won't
> enable card insert/remove interrupts.
> 
> This is needed for hosts with unreliable card detection, such as FSL
> eSDHC. The original eSDHC driver was tring to "debounce" card-detection
> IRQs by reading present state and disabling particular interrupts. But
> with this debouncing scheme I noticed that sometimes we miss card
> insertion/removal events.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>

[toc] | [prev] | [next] | [standalone]

#1332675

>>
>> According to the below commit, SDHCI_QUIRK_BROKEN_CARD_DETECTION was
>> invented because of unreliable card detection mechanism inside the
>> sdhci controller.
>> Therefore it required polling to be used, but also to make ->get_cd()
>> to always return 1 in these cases.
>>
>> Although, as I understand it that's not the case here. You can still
>> rely on card detection to work, but as you don't have wakeups you
>> can't fully make use of card detect, when combined with runtime PM.
>> I am not sure we should add more users of
>> SDHCI_QUIRK_BROKEN_CARD_DETECTION, especially since in this case it's
>> not reflecting the capability of the hardware.
>>
>> Can't we think of another way?
>
> Sorry but I am not sure to understand. In the previous thread, you told
> me to use MMC_CAP_NEEDS_POLL which is set if we have
> SDHCI_QUIRK_BROKEN_CARD_DETECTION. I was not confortable to do this
> because as you say it is not reflecting the capability of the hardware.
>
> Do you mean that I can simply add MMC_CAP_NEEDS_POLL after sdhci_add_host()?

Yes, something like that, but...

Within this context, I realize that the DT binding "broken-cd" has two
different meanings, while comparing the generic MMC bindings towards
SDHCI's. That's bad.

In the SDHCI case it means, enable MMC_CAP_NEEDS_POLL *and* make
->get_cd() to always return 1 (via adding
SDHCI_QUIRK_BROKEN_CARD_DETECTION).

In the generic MMC case, it means only to enable MMC_CAP_NEEDS_POLL,
which is exactly what you want.

Perhaps you wonder why I think it's a good good idea to use DT to
decide if MMC_CAP_NEEDS_POLL should be enabled?
It allows flexibility for future platforms. For example, there may be
platforms adding GPIO card detect support or even cards that's
non-removable.

I realize that the fix to solve this regression would then mean that
sdhci-of-at91 need to clear SDHCI_QUIRK_BROKEN_CARD_DETECTION after
parsing the shdci DTB, but then the DTB for your platform also needs
an update as the "broken-cd" options needs to be set.

Do you think this can work?

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1332694

On Fri, Feb 12, 2016 at 12:01:39PM +0100, Ulf Hansson wrote:
> >>
> >> According to the below commit, SDHCI_QUIRK_BROKEN_CARD_DETECTION was
> >> invented because of unreliable card detection mechanism inside the
> >> sdhci controller.
> >> Therefore it required polling to be used, but also to make ->get_cd()
> >> to always return 1 in these cases.
> >>
> >> Although, as I understand it that's not the case here. You can still
> >> rely on card detection to work, but as you don't have wakeups you
> >> can't fully make use of card detect, when combined with runtime PM.
> >> I am not sure we should add more users of
> >> SDHCI_QUIRK_BROKEN_CARD_DETECTION, especially since in this case it's
> >> not reflecting the capability of the hardware.
> >>
> >> Can't we think of another way?
> >
> > Sorry but I am not sure to understand. In the previous thread, you told
> > me to use MMC_CAP_NEEDS_POLL which is set if we have
> > SDHCI_QUIRK_BROKEN_CARD_DETECTION. I was not confortable to do this
> > because as you say it is not reflecting the capability of the hardware.
> >
> > Do you mean that I can simply add MMC_CAP_NEEDS_POLL after sdhci_add_host()?
> 
> Yes, something like that, but...
> 
> Within this context, I realize that the DT binding "broken-cd" has two
> different meanings, while comparing the generic MMC bindings towards
> SDHCI's. That's bad.
> 
> In the SDHCI case it means, enable MMC_CAP_NEEDS_POLL *and* make
> ->get_cd() to always return 1 (via adding
> SDHCI_QUIRK_BROKEN_CARD_DETECTION).
> 
> In the generic MMC case, it means only to enable MMC_CAP_NEEDS_POLL,
> which is exactly what you want.
> 
> Perhaps you wonder why I think it's a good good idea to use DT to
> decide if MMC_CAP_NEEDS_POLL should be enabled?
> It allows flexibility for future platforms. For example, there may be
> platforms adding GPIO card detect support or even cards that's
> non-removable.

I agree.

> 
> I realize that the fix to solve this regression would then mean that
> sdhci-of-at91 need to clear SDHCI_QUIRK_BROKEN_CARD_DETECTION after
> parsing the shdci DTB, but then the DTB for your platform also needs
> an update as the "broken-cd" options needs to be set.
> 
> Do you think this can work?
> 

It should but as SDHCI_QUIRK_BROKEN_CARD_DETECTION, broken-cd is not
reflecting the capability of the hardware.

I was thinking about checking non-removable and using mmc_gpio_get_cd()
in my runtime_suspend callback. If I have a non removable device or a
gpio for card detection then I can disable all clocks and call
sdhci_runtime_suspend_host(). If not, I keep enabled the clock for the
'interface', disable the other one and that's all. The controller won't
be set as runtime suspended but I would save some power. Does it sounds
good?


Regards

Ludovic

[toc] | [prev] | [next] | [standalone]

#1333292 — [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

When suspending the sdhci host, the only hardware event that could wake
up the host is a sdio irq if they are enabled. If we want to wakeup on
card detect events, a gpio as to be used.
If we don't want to use a gpio but the card detect pio of the controller
then we need to keep enabled the clock of the controller interface to
get the interrupt and to not set the host in a suspended state to have the
interrupt handled.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 46 ++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index efec736..2159c6e 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -18,6 +18,7 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/mmc/host.h>
+#include <linux/mmc/slot-gpio.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
 
 static const struct sdhci_pltfm_data soc_data_sama5d2 = {
 	.ops = &sdhci_at91_sama5d2_ops,
-	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
 	.quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
 };
 
@@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
 };
 
 #ifdef CONFIG_PM
+static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
+{
+	u32 caps = host->mmc->caps;
+
+	return (caps & MMC_CAP_NONREMOVABLE) ||
+	       (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
+}
+
 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 = pltfm_host->priv;
-	int ret;
+	int ret = 0;
 
-	ret = sdhci_runtime_suspend_host(host);
+	/*
+	 * If we call sdhci_runtime_suspend(), we could wakeup only if sdio
+	 * irqs are enabled. If we want to wakeup on a card detect event there
+	 * are two options:
+	 * - do not call sdhci_runtime_suspend() but save power by disabling
+	 *   all clocks excepting the one for the controller interface.
+	 * - call sdhci_runtime_suspend(), save maximum power by disabling
+	 *   all clocks but use a gpio for the card detect signal to have a way
+	 *   to wakeup.
+	 */
+	if (sdhci_at91_use_sdhci_runtime(host)) {
+		ret = sdhci_runtime_suspend_host(host);
+		clk_disable_unprepare(priv->hclock);
+	}
 
 	clk_disable_unprepare(priv->gck);
-	clk_disable_unprepare(priv->hclock);
 	clk_disable_unprepare(priv->mainck);
 
 	return ret;
@@ -84,19 +104,23 @@ 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;
 	}
 
-	return sdhci_runtime_resume_host(host);
+	if (sdhci_at91_use_sdhci_runtime(host)) {
+		ret = clk_prepare_enable(priv->hclock);
+		if (ret) {
+			dev_err(dev, "can't enable hclock\n");
+			return ret;
+		}
+
+		ret = sdhci_runtime_resume_host(host);
+	}
+
+	return ret;
 }
 #endif /* CONFIG_PM */
 
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1335466 — Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

On 13 February 2016 at 10:56, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> When suspending the sdhci host, the only hardware event that could wake
> up the host is a sdio irq if they are enabled. If we want to wakeup on
> card detect events, a gpio as to be used.
> If we don't want to use a gpio but the card detect pio of the controller
> then we need to keep enabled the clock of the controller interface to
> get the interrupt and to not set the host in a suspended state to have the
> interrupt handled.
>
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 46 ++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index efec736..2159c6e 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/mmc/host.h>
> +#include <linux/mmc/slot-gpio.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
>
>  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
>         .ops = &sdhci_at91_sama5d2_ops,
> -       .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,

You probably have some leftovers from earlier local changes, as this
isn't going to apply to my next branch.

>         .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>  };
>
> @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>
>  #ifdef CONFIG_PM
> +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> +{
> +       u32 caps = host->mmc->caps;
> +
> +       return (caps & MMC_CAP_NONREMOVABLE) ||
> +              (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
> +}
> +
>  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 = pltfm_host->priv;
> -       int ret;
> +       int ret = 0;
>
> -       ret = sdhci_runtime_suspend_host(host);
> +       /*
> +        * If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> +        * irqs are enabled. If we want to wakeup on a card detect event there
> +        * are two options:
> +        * - do not call sdhci_runtime_suspend() but save power by disabling
> +        *   all clocks excepting the one for the controller interface.
> +        * - call sdhci_runtime_suspend(), save maximum power by disabling
> +        *   all clocks but use a gpio for the card detect signal to have a way
> +        *   to wakeup.
> +        */
> +       if (sdhci_at91_use_sdhci_runtime(host)) {

I am not sure this approach is safe, particularly for cases when
sdhci_runtime_suspend() isn't invoked.

As I understand it, in those cases potentially the sdhci's IRQ handler
may be invoked to serve even other IRQs than a card detect IRQ. Doing
that while being runtime suspended doesn't seem like a good idea. Will
it even work?

> +               ret = sdhci_runtime_suspend_host(host);
> +               clk_disable_unprepare(priv->hclock);
> +       }
>
>         clk_disable_unprepare(priv->gck);
> -       clk_disable_unprepare(priv->hclock);
>         clk_disable_unprepare(priv->mainck);
>
>         return ret;
> @@ -84,19 +104,23 @@ 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;
>         }
>
> -       return sdhci_runtime_resume_host(host);
> +       if (sdhci_at91_use_sdhci_runtime(host)) {
> +               ret = clk_prepare_enable(priv->hclock);
> +               if (ret) {
> +                       dev_err(dev, "can't enable hclock\n");
> +                       return ret;
> +               }
> +
> +               ret = sdhci_runtime_resume_host(host);
> +       }
> +
> +       return ret;
>  }
>  #endif /* CONFIG_PM */
>
> --
> 2.7.0
>

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1335525 — Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
> On 13 February 2016 at 10:56, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > When suspending the sdhci host, the only hardware event that could wake
> > up the host is a sdio irq if they are enabled. If we want to wakeup on
> > card detect events, a gpio as to be used.
> > If we don't want to use a gpio but the card detect pio of the controller
> > then we need to keep enabled the clock of the controller interface to
> > get the interrupt and to not set the host in a suspended state to have the
> > interrupt handled.
> >
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > ---
> >  drivers/mmc/host/sdhci-of-at91.c | 46 ++++++++++++++++++++++++++++++----------
> >  1 file changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> > index efec736..2159c6e 100644
> > --- a/drivers/mmc/host/sdhci-of-at91.c
> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/mmc/host.h>
> > +#include <linux/mmc/slot-gpio.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
> >
> >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> >         .ops = &sdhci_at91_sama5d2_ops,
> > -       .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
> 
> You probably have some leftovers from earlier local changes, as this
> isn't going to apply to my next branch.
> 

Yes, it is based on the first patch of this thread, it was only to
discuss about it.

> >         .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
> >  };
> >
> > @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
> >  };
> >
> >  #ifdef CONFIG_PM
> > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> > +{
> > +       u32 caps = host->mmc->caps;
> > +
> > +       return (caps & MMC_CAP_NONREMOVABLE) ||
> > +              (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
> > +}
> > +
> >  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 = pltfm_host->priv;
> > -       int ret;
> > +       int ret = 0;
> >
> > -       ret = sdhci_runtime_suspend_host(host);
> > +       /*
> > +        * If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> > +        * irqs are enabled. If we want to wakeup on a card detect event there
> > +        * are two options:
> > +        * - do not call sdhci_runtime_suspend() but save power by disabling
> > +        *   all clocks excepting the one for the controller interface.
> > +        * - call sdhci_runtime_suspend(), save maximum power by disabling
> > +        *   all clocks but use a gpio for the card detect signal to have a way
> > +        *   to wakeup.
> > +        */
> > +       if (sdhci_at91_use_sdhci_runtime(host)) {
> 
> I am not sure this approach is safe, particularly for cases when
> sdhci_runtime_suspend() isn't invoked.
> 
> As I understand it, in those cases potentially the sdhci's IRQ handler
> may be invoked to serve even other IRQs than a card detect IRQ. Doing
> that while being runtime suspended doesn't seem like a good idea. Will
> it even work?
> 

Yes it is the idea. It will allow to save some power. I don't see how I can use
runtime PM if I need to invoke sdhci_runtime_suspend_host() without modifying
sdhci layer to handle card detect irq.

I was also afraid to have some side effects but it works, at least for the
card detect case.

> > +               ret = sdhci_runtime_suspend_host(host);
> > +               clk_disable_unprepare(priv->hclock);
> > +       }
> >
> >         clk_disable_unprepare(priv->gck);
> > -       clk_disable_unprepare(priv->hclock);
> >         clk_disable_unprepare(priv->mainck);
> >
> >         return ret;
> > @@ -84,19 +104,23 @@ 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;
> >         }
> >
> > -       return sdhci_runtime_resume_host(host);
> > +       if (sdhci_at91_use_sdhci_runtime(host)) {
> > +               ret = clk_prepare_enable(priv->hclock);
> > +               if (ret) {
> > +                       dev_err(dev, "can't enable hclock\n");
> > +                       return ret;
> > +               }
> > +
> > +               ret = sdhci_runtime_resume_host(host);
> > +       }
> > +
> > +       return ret;
> >  }
> >  #endif /* CONFIG_PM */

Regards

Ludovic

[toc] | [prev] | [next] | [standalone]

#1336228 — Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

On Tue, Feb 16, 2016 at 04:22:04PM +0100, Ludovic Desroches wrote:
> On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
> > On 13 February 2016 at 10:56, Ludovic Desroches
> > <ludovic.desroches@atmel.com> wrote:
> > > When suspending the sdhci host, the only hardware event that could wake
> > > up the host is a sdio irq if they are enabled. If we want to wakeup on
> > > card detect events, a gpio as to be used.
> > > If we don't want to use a gpio but the card detect pio of the controller
> > > then we need to keep enabled the clock of the controller interface to
> > > get the interrupt and to not set the host in a suspended state to have the
> > > interrupt handled.
> > >
> > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > > ---
> > >  drivers/mmc/host/sdhci-of-at91.c | 46 ++++++++++++++++++++++++++++++----------
> > >  1 file changed, 35 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> > > index efec736..2159c6e 100644
> > > --- a/drivers/mmc/host/sdhci-of-at91.c
> > > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/err.h>
> > >  #include <linux/io.h>
> > >  #include <linux/mmc/host.h>
> > > +#include <linux/mmc/slot-gpio.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_device.h>
> > > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
> > >
> > >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> > >         .ops = &sdhci_at91_sama5d2_ops,
> > > -       .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
> > 
> > You probably have some leftovers from earlier local changes, as this
> > isn't going to apply to my next branch.
> > 
> 
> Yes, it is based on the first patch of this thread, it was only to
> discuss about it.
> 
> > >         .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
> > >  };
> > >
> > > @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
> > >  };
> > >
> > >  #ifdef CONFIG_PM
> > > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> > > +{
> > > +       u32 caps = host->mmc->caps;
> > > +
> > > +       return (caps & MMC_CAP_NONREMOVABLE) ||
> > > +              (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));


I am wondering if I should take account of sdio irq enabled or not here.

I have a sdio device which drives me crazy because of power management.
The driver of this device is in staging, it is wilc1000. It seems that I
am stuck because the sdio irq are not received. If I don't disable the
clock of the controller (hclock), I should receive the sdio IRQ as I
receive card detect ones, isn't it?

It doesn't work, it seems I have also to not disabled mainck and gck
which are clocks needed to generate the clock sent to the sdio device.
If none of the clocks have to be disabled, where it has to be managed?

Do I have to anticipate this use case in the driver of my sdhci
controller or does it have to be managed in the sdio device driver? They
are using sdio_claim/release_host to suspend or resume the host but
maybe they use it in a bad way.

> > > +}
> > > +
> > >  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 = pltfm_host->priv;
> > > -       int ret;
> > > +       int ret = 0;
> > >
> > > -       ret = sdhci_runtime_suspend_host(host);
> > > +       /*
> > > +        * If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> > > +        * irqs are enabled. If we want to wakeup on a card detect event there
> > > +        * are two options:
> > > +        * - do not call sdhci_runtime_suspend() but save power by disabling
> > > +        *   all clocks excepting the one for the controller interface.
> > > +        * - call sdhci_runtime_suspend(), save maximum power by disabling
> > > +        *   all clocks but use a gpio for the card detect signal to have a way
> > > +        *   to wakeup.
> > > +        */
> > > +       if (sdhci_at91_use_sdhci_runtime(host)) {
> > 
> > I am not sure this approach is safe, particularly for cases when
> > sdhci_runtime_suspend() isn't invoked.
> > 
> > As I understand it, in those cases potentially the sdhci's IRQ handler
> > may be invoked to serve even other IRQs than a card detect IRQ. Doing
> > that while being runtime suspended doesn't seem like a good idea. Will
> > it even work?
> > 
> 
> Yes it is the idea. It will allow to save some power. I don't see how I can use
> runtime PM if I need to invoke sdhci_runtime_suspend_host() without modifying
> sdhci layer to handle card detect irq.
> 
> I was also afraid to have some side effects but it works, at least for the
> card detect case.
> 
> > > +               ret = sdhci_runtime_suspend_host(host);
> > > +               clk_disable_unprepare(priv->hclock);
> > > +       }
> > >
> > >         clk_disable_unprepare(priv->gck);
> > > -       clk_disable_unprepare(priv->hclock);
> > >         clk_disable_unprepare(priv->mainck);
> > >
> > >         return ret;
> > > @@ -84,19 +104,23 @@ 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;
> > >         }
> > >
> > > -       return sdhci_runtime_resume_host(host);
> > > +       if (sdhci_at91_use_sdhci_runtime(host)) {
> > > +               ret = clk_prepare_enable(priv->hclock);
> > > +               if (ret) {
> > > +                       dev_err(dev, "can't enable hclock\n");
> > > +                       return ret;
> > > +               }
> > > +
> > > +               ret = sdhci_runtime_resume_host(host);
> > > +       }
> > > +
> > > +       return ret;
> > >  }
> > >  #endif /* CONFIG_PM */
> 
> Regards
> 
> Ludovic

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web