[PATCH 0/3] sdhci-dove GPIO support for card detection

[PATCH 0/3] sdhci-dove GPIO support for card detection

[Russell King - ARM Linux]

The following three patches allow sdhci-dove to use a GPIO for card
presence detection.  This is a three stage process:

1. Clean up error paths by using devm_clk_get().
2. Use the two-stage sdhci initialization.
3. Implement the GPIO card detection support.

A very similar patch has been tested by Sebastian on the cubox; since
that version, I've changed the error printing slightly and dropped the
non-DT support for this feature as mainline doesn't have non-DT cubox
support.

 drivers/mmc/host/sdhci-dove.c |  108 +++++++++++++++++++++++++++++++++--------
 1 files changed, 88 insertions(+), 20 deletions(-)

[PATCH 1/3] MMC: sdhci-dove: use devm_clk_get()

[Russell King]

Use devm_clk_get() rather than clk_get() to make cleanup paths more simple.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci-dove.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c
index 8fd50a2..4233fd9 100644
--- a/drivers/mmc/host/sdhci-dove.c
+++ b/drivers/mmc/host/sdhci-dove.c
@@ -92,7 +92,7 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	priv->clk = clk_get(&pdev->dev, NULL);
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (!IS_ERR(priv->clk))
 		clk_prepare_enable(priv->clk);
 
@@ -107,10 +107,8 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
 	return 0;
 
 sdhci_dove_register_fail:
-	if (!IS_ERR(priv->clk)) {
+	if (!IS_ERR(priv->clk))
 		clk_disable_unprepare(priv->clk);
-		clk_put(priv->clk);
-	}
 	return ret;
 }
 
@@ -122,10 +120,9 @@ static int __devexit sdhci_dove_remove(struct platform_device *pdev)
 
 	sdhci_pltfm_unregister(pdev);
 
-	if (!IS_ERR(priv->clk)) {
+	if (!IS_ERR(priv->clk))
 		clk_disable_unprepare(priv->clk);
-		clk_put(priv->clk);
-	}
+
 	return 0;
 }
 
-- 
1.7.4.4

[PATCH 2/3] MMC: sdhci-dove: use two-stage initialization for sdhci-pltfm

[Russell King]

We need to use the two-stage initialization for sdhci-pltfm if we're
going to do anything extra at initialization time.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci-dove.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c
index 4233fd9..4af64f3 100644
--- a/drivers/mmc/host/sdhci-dove.c
+++ b/drivers/mmc/host/sdhci-dove.c
@@ -93,22 +93,32 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
 	}
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
-	if (!IS_ERR(priv->clk))
-		clk_prepare_enable(priv->clk);
 
-	ret = sdhci_pltfm_register(pdev, &sdhci_dove_pdata);
-	if (ret)
-		goto sdhci_dove_register_fail;
+	host = sdhci_pltfm_init(pdev, &sdhci_dove_pdata);
+	if (IS_ERR(host)) {
+		ret = PTR_ERR(host);
+		goto err_sdhci_pltfm_init;
+	}
 
-	host = platform_get_drvdata(pdev);
 	pltfm_host = sdhci_priv(host);
 	pltfm_host->priv = priv;
 
+	if (!IS_ERR(priv->clk))
+		clk_prepare_enable(priv->clk);
+
+	sdhci_get_of_property(pdev);
+
+	ret = sdhci_add_host(host);
+	if (ret)
+		goto err_sdhci_add;
+
 	return 0;
 
-sdhci_dove_register_fail:
+err_sdhci_add:
 	if (!IS_ERR(priv->clk))
 		clk_disable_unprepare(priv->clk);
+	sdhci_pltfm_free(pdev);
+err_sdhci_pltfm_init:
 	return ret;
 }
 
-- 
1.7.4.4

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Russell King]

This commit taken from Rabeeh's Cubox kernel and re-worked for DT;
Sebastian Hasselbrath is believed to be the original author.

Some Cuboxes require a GPIO for card detection; this implements the
optional GPIO support for card detection.  This GPIO is logic 0 for
card inserted.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci-dove.c |   73 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c
index 4af64f3..e621448 100644
--- a/drivers/mmc/host/sdhci-dove.c
+++ b/drivers/mmc/host/sdhci-dove.c
@@ -19,20 +19,30 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
-#include <linux/err.h>
-#include <linux/io.h>
 #include <linux/clk.h>
 #include <linux/err.h>
-#include <linux/module.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
 #include <linux/mmc/host.h>
+#include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 
 #include "sdhci-pltfm.h"
 
 struct sdhci_dove_priv {
 	struct clk *clk;
+	int gpio_cd;
 };
 
+static irqreturn_t sdhci_dove_carddetect_irq(int irq, void *data)
+{
+	struct sdhci_host *host = data;
+
+	tasklet_schedule(&host->card_tasklet);
+	return IRQ_HANDLED;
+}
+
 static u16 sdhci_dove_readw(struct sdhci_host *host, int reg)
 {
 	u16 ret;
@@ -50,16 +60,25 @@ static u16 sdhci_dove_readw(struct sdhci_host *host, int reg)
 
 static u32 sdhci_dove_readl(struct sdhci_host *host, int reg)
 {
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_dove_priv *priv = pltfm_host->priv;
 	u32 ret;
 
+	ret = readl(host->ioaddr + reg);
+
 	switch (reg) {
 	case SDHCI_CAPABILITIES:
-		ret = readl(host->ioaddr + reg);
 		/* Mask the support for 3.0V */
 		ret &= ~SDHCI_CAN_VDD_300;
 		break;
-	default:
-		ret = readl(host->ioaddr + reg);
+	case SDHCI_PRESENT_STATE:
+		if (gpio_is_valid(priv->gpio_cd)) {
+			if (gpio_get_value(priv->gpio_cd) == 0)
+				ret |= SDHCI_CARD_PRESENT;
+			else
+				ret &= ~SDHCI_CARD_PRESENT;
+		}
+		break;
 	}
 	return ret;
 }
@@ -94,6 +113,23 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 
+	if (pdev->dev.of_node) {
+		priv->gpio_cd = of_get_named_gpio(pdev->dev.of_node,
+						  "cd-gpios", 0);
+	} else {
+		priv->gpio_cd = -EINVAL;
+	}
+
+	if (gpio_is_valid(priv->gpio_cd)) {
+		ret = gpio_request(priv->gpio_cd, "sdhci-cd");
+		if (ret) {
+			dev_err(&pdev->dev, "card detect gpio request failed: %d\n",
+				ret);
+			return ret;
+		}
+		gpio_direction_input(priv->gpio_cd);
+	}
+
 	host = sdhci_pltfm_init(pdev, &sdhci_dove_pdata);
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
@@ -112,13 +148,33 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_sdhci_add;
 
+	/*
+	 * We must request the IRQ after sdhci_add_host(), as the tasklet only
+	 * gets setup in sdhci_add_host() and we oops.
+	 */
+	if (gpio_is_valid(priv->gpio_cd)) {
+		ret = request_irq(gpio_to_irq(priv->gpio_cd),
+				  sdhci_dove_carddetect_irq,
+				  IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+				  mmc_hostname(host->mmc), host);
+		if (ret) {
+			dev_err(&pdev->dev, "card detect irq request failed: %d\n",
+				ret);
+			goto err_request_irq;
+		}
+	}
+
 	return 0;
 
+err_request_irq:
+	sdhci_remove_host(host, 0);
 err_sdhci_add:
 	if (!IS_ERR(priv->clk))
 		clk_disable_unprepare(priv->clk);
 	sdhci_pltfm_free(pdev);
 err_sdhci_pltfm_init:
+	if (gpio_is_valid(priv->gpio_cd))
+		gpio_free(priv->gpio_cd);
 	return ret;
 }
 
@@ -130,6 +186,11 @@ static int __devexit sdhci_dove_remove(struct platform_device *pdev)
 
 	sdhci_pltfm_unregister(pdev);
 
+	if (gpio_is_valid(priv->gpio_cd)) {
+		free_irq(gpio_to_irq(priv->gpio_cd), host);
+		gpio_free(priv->gpio_cd);
+	}
+
 	if (!IS_ERR(priv->clk))
 		clk_disable_unprepare(priv->clk);
 
-- 
1.7.4.4

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Shawn Guo]

On Thu, Nov 22, 2012 at 11:55:51PM +0000, Russell King wrote:
> This commit taken from Rabeeh's Cubox kernel and re-worked for DT;
> Sebastian Hasselbrath is believed to be the original author.
> 
> Some Cuboxes require a GPIO for card detection; this implements the
> optional GPIO support for card detection.  This GPIO is logic 0 for
> card inserted.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/mmc/host/sdhci-dove.c |   73 +++++++++++++++++++++++++++++++++++++---
>  1 files changed, 67 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c
> index 4af64f3..e621448 100644
> --- a/drivers/mmc/host/sdhci-dove.c
> +++ b/drivers/mmc/host/sdhci-dove.c
> @@ -19,20 +19,30 @@
>   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
> -#include <linux/err.h>
> -#include <linux/io.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> -#include <linux/module.h>
> +#include <linux/gpio.h>
> +#include <linux/io.h>
>  #include <linux/mmc/host.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_gpio.h>
>  
>  #include "sdhci-pltfm.h"
>  
>  struct sdhci_dove_priv {
>  	struct clk *clk;
> +	int gpio_cd;
>  };
>  
> +static irqreturn_t sdhci_dove_carddetect_irq(int irq, void *data)
> +{
> +	struct sdhci_host *host = data;
> +
> +	tasklet_schedule(&host->card_tasklet);
> +	return IRQ_HANDLED;
> +}
> +
>  static u16 sdhci_dove_readw(struct sdhci_host *host, int reg)
>  {
>  	u16 ret;
> @@ -50,16 +60,25 @@ static u16 sdhci_dove_readw(struct sdhci_host *host, int reg)
>  
>  static u32 sdhci_dove_readl(struct sdhci_host *host, int reg)
>  {
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_dove_priv *priv = pltfm_host->priv;
>  	u32 ret;
>  
> +	ret = readl(host->ioaddr + reg);
> +
>  	switch (reg) {
>  	case SDHCI_CAPABILITIES:
> -		ret = readl(host->ioaddr + reg);
>  		/* Mask the support for 3.0V */
>  		ret &= ~SDHCI_CAN_VDD_300;
>  		break;
> -	default:
> -		ret = readl(host->ioaddr + reg);
> +	case SDHCI_PRESENT_STATE:
> +		if (gpio_is_valid(priv->gpio_cd)) {
> +			if (gpio_get_value(priv->gpio_cd) == 0)
> +				ret |= SDHCI_CARD_PRESENT;
> +			else
> +				ret &= ~SDHCI_CARD_PRESENT;
> +		}
> +		break;

It seems that the helpers in drivers/mmc/core/slot-gpio.c can help to
simplify the patch a lot.  With sdhci core code being calling
mmc_gpio_get_cd() to query card present, the fake SDHCI_PRESENT_STATE
implementation above can be totally saved.

Shawn

>  	}
>  	return ret;
>  }
> @@ -94,6 +113,23 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
>  
>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>  
> +	if (pdev->dev.of_node) {
> +		priv->gpio_cd = of_get_named_gpio(pdev->dev.of_node,
> +						  "cd-gpios", 0);
> +	} else {
> +		priv->gpio_cd = -EINVAL;
> +	}
> +
> +	if (gpio_is_valid(priv->gpio_cd)) {
> +		ret = gpio_request(priv->gpio_cd, "sdhci-cd");
> +		if (ret) {
> +			dev_err(&pdev->dev, "card detect gpio request failed: %d\n",
> +				ret);
> +			return ret;
> +		}
> +		gpio_direction_input(priv->gpio_cd);
> +	}
> +
>  	host = sdhci_pltfm_init(pdev, &sdhci_dove_pdata);
>  	if (IS_ERR(host)) {
>  		ret = PTR_ERR(host);
> @@ -112,13 +148,33 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_sdhci_add;
>  
> +	/*
> +	 * We must request the IRQ after sdhci_add_host(), as the tasklet only
> +	 * gets setup in sdhci_add_host() and we oops.
> +	 */
> +	if (gpio_is_valid(priv->gpio_cd)) {
> +		ret = request_irq(gpio_to_irq(priv->gpio_cd),
> +				  sdhci_dove_carddetect_irq,
> +				  IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +				  mmc_hostname(host->mmc), host);
> +		if (ret) {
> +			dev_err(&pdev->dev, "card detect irq request failed: %d\n",
> +				ret);
> +			goto err_request_irq;
> +		}
> +	}
> +
>  	return 0;
>  
> +err_request_irq:
> +	sdhci_remove_host(host, 0);
>  err_sdhci_add:
>  	if (!IS_ERR(priv->clk))
>  		clk_disable_unprepare(priv->clk);
>  	sdhci_pltfm_free(pdev);
>  err_sdhci_pltfm_init:
> +	if (gpio_is_valid(priv->gpio_cd))
> +		gpio_free(priv->gpio_cd);
>  	return ret;
>  }
>  
> @@ -130,6 +186,11 @@ static int __devexit sdhci_dove_remove(struct platform_device *pdev)
>  
>  	sdhci_pltfm_unregister(pdev);
>  
> +	if (gpio_is_valid(priv->gpio_cd)) {
> +		free_irq(gpio_to_irq(priv->gpio_cd), host);
> +		gpio_free(priv->gpio_cd);
> +	}
> +
>  	if (!IS_ERR(priv->clk))
>  		clk_disable_unprepare(priv->clk);
>  
> -- 
> 1.7.4.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Russell King - ARM Linux]

On Fri, Nov 23, 2012 at 04:31:20PM +0800, Shawn Guo wrote:
> On Thu, Nov 22, 2012 at 11:55:51PM +0000, Russell King wrote:
> > This commit taken from Rabeeh's Cubox kernel and re-worked for DT;
> > Sebastian Hasselbrath is believed to be the original author.
> > 
> > Some Cuboxes require a GPIO for card detection; this implements the
> > optional GPIO support for card detection.  This GPIO is logic 0 for
> > card inserted.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/mmc/host/sdhci-dove.c |   73 +++++++++++++++++++++++++++++++++++++---
> >  1 files changed, 67 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c
> > index 4af64f3..e621448 100644
> > --- a/drivers/mmc/host/sdhci-dove.c
> > +++ b/drivers/mmc/host/sdhci-dove.c
> > @@ -19,20 +19,30 @@
> >   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> >   */
> >  
> > -#include <linux/err.h>
> > -#include <linux/io.h>
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> > -#include <linux/module.h>
> > +#include <linux/gpio.h>
> > +#include <linux/io.h>
> >  #include <linux/mmc/host.h>
> > +#include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_gpio.h>
> >  
> >  #include "sdhci-pltfm.h"
> >  
> >  struct sdhci_dove_priv {
> >  	struct clk *clk;
> > +	int gpio_cd;
> >  };
> >  
> > +static irqreturn_t sdhci_dove_carddetect_irq(int irq, void *data)
> > +{
> > +	struct sdhci_host *host = data;
> > +
> > +	tasklet_schedule(&host->card_tasklet);
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static u16 sdhci_dove_readw(struct sdhci_host *host, int reg)
> >  {
> >  	u16 ret;
> > @@ -50,16 +60,25 @@ static u16 sdhci_dove_readw(struct sdhci_host *host, int reg)
> >  
> >  static u32 sdhci_dove_readl(struct sdhci_host *host, int reg)
> >  {
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct sdhci_dove_priv *priv = pltfm_host->priv;
> >  	u32 ret;
> >  
> > +	ret = readl(host->ioaddr + reg);
> > +
> >  	switch (reg) {
> >  	case SDHCI_CAPABILITIES:
> > -		ret = readl(host->ioaddr + reg);
> >  		/* Mask the support for 3.0V */
> >  		ret &= ~SDHCI_CAN_VDD_300;
> >  		break;
> > -	default:
> > -		ret = readl(host->ioaddr + reg);
> > +	case SDHCI_PRESENT_STATE:
> > +		if (gpio_is_valid(priv->gpio_cd)) {
> > +			if (gpio_get_value(priv->gpio_cd) == 0)
> > +				ret |= SDHCI_CARD_PRESENT;
> > +			else
> > +				ret &= ~SDHCI_CARD_PRESENT;
> > +		}
> > +		break;
> 
> It seems that the helpers in drivers/mmc/core/slot-gpio.c can help to
> simplify the patch a lot.  With sdhci core code being calling
> mmc_gpio_get_cd() to query card present, the fake SDHCI_PRESENT_STATE
> implementation above can be totally saved.

Does this work with the sdhci stuff?  I notice that sdhci does some
special additional resetting of the host when it detects that a card
is removed; this would be lost if that were to be used because the
tasklet in sdhci.c would be bypassed.

So no, I don't think slot-gpio.c can be used here.

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Shawn Guo]

On Fri, Nov 23, 2012 at 08:57:26AM +0000, Russell King - ARM Linux wrote:
> Does this work with the sdhci stuff?

Honestly, I'm not sure, but I guess it does, since I have seen
sdhci-pxav3 driver using the helpers.  Anyway, I'm going to adopt
the helpers for sdhci-esdhc-imx driver to find it out.

Shawn

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Russell King - ARM Linux]

On Fri, Nov 23, 2012 at 08:14:27PM +0800, Shawn Guo wrote:
> On Fri, Nov 23, 2012 at 08:57:26AM +0000, Russell King - ARM Linux wrote:
> > Does this work with the sdhci stuff?
> 
> Honestly, I'm not sure, but I guess it does, since I have seen
> sdhci-pxav3 driver using the helpers.  Anyway, I'm going to adopt
> the helpers for sdhci-esdhc-imx driver to find it out.

The thing that worries me is this:

static void sdhci_tasklet_card(unsigned long param)
{
...
        /* Check host->mrq first in case we are runtime suspended */
        if (host->mrq &&
            !(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
                pr_err("%s: Card removed during transfer!\n",
                        mmc_hostname(host->mmc));
                pr_err("%s: Resetting controller.\n",
                        mmc_hostname(host->mmc));

                sdhci_reset(host, SDHCI_RESET_CMD);
                sdhci_reset(host, SDHCI_RESET_DATA);

                host->mrq->cmd->error = -ENOMEDIUM;
                tasklet_schedule(&host->finish_tasklet);
        }
...
        mmc_detect_change(host->mmc, msecs_to_jiffies(200));
}


That gets called whenever a card is inserted/removed by the SDHCI code (if
the SDHCI card detect is wired), or in my case by the interrupt routine
the code in my patch adds.

The slot-gpio.c stuff directly calls into mmc_detect_change (a) with a
shorter delay, and (b) completely omits the above handling and resetting.
My guess from the above code is that it'll work fine 90% of the time
(because you'll remove the card without an active request), but the above
code looks like it's addressing a corner case which will be omitted with
the "generic" slot-gpio.c solution.

So I don't think it's a good idea to use slot-gpio.c in this case.

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Chris Ball]

Hi, adding Guennadi,

On Fri, Nov 23 2012, Russell King - ARM Linux wrote:
> On Fri, Nov 23, 2012 at 08:14:27PM +0800, Shawn Guo wrote:
>> On Fri, Nov 23, 2012 at 08:57:26AM +0000, Russell King - ARM Linux wrote:
>> > Does this work with the sdhci stuff?
>> 
>> Honestly, I'm not sure, but I guess it does, since I have seen
>> sdhci-pxav3 driver using the helpers.  Anyway, I'm going to adopt
>> the helpers for sdhci-esdhc-imx driver to find it out.
>
> The thing that worries me is this:
>
> static void sdhci_tasklet_card(unsigned long param)
> {
> ...
>         /* Check host->mrq first in case we are runtime suspended */
>         if (host->mrq &&
>             !(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
>                 pr_err("%s: Card removed during transfer!\n",
>                         mmc_hostname(host->mmc));
>                 pr_err("%s: Resetting controller.\n",
>                         mmc_hostname(host->mmc));
>
>                 sdhci_reset(host, SDHCI_RESET_CMD);
>                 sdhci_reset(host, SDHCI_RESET_DATA);
>
>                 host->mrq->cmd->error = -ENOMEDIUM;
>                 tasklet_schedule(&host->finish_tasklet);
>         }
> ...
>         mmc_detect_change(host->mmc, msecs_to_jiffies(200));
> }
>
>
> That gets called whenever a card is inserted/removed by the SDHCI code (if
> the SDHCI card detect is wired), or in my case by the interrupt routine
> the code in my patch adds.
>
> The slot-gpio.c stuff directly calls into mmc_detect_change (a) with a
> shorter delay, and (b) completely omits the above handling and resetting.
> My guess from the above code is that it'll work fine 90% of the time
> (because you'll remove the card without an active request), but the above
> code looks like it's addressing a corner case which will be omitted with
> the "generic" slot-gpio.c solution.
>
> So I don't think it's a good idea to use slot-gpio.c in this case.

Guennadi, what are your thoughts about consolidating this reset logic
between the sdhci tasklet and slot-gpio?  It would certainly be nice to
use slot-gpio in cases like this one, so it's worth fixing.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Guennadi Liakhovetski]

Hi Chris

On Sun, 25 Nov 2012, Chris Ball wrote:

> Hi, adding Guennadi,
> 
> On Fri, Nov 23 2012, Russell King - ARM Linux wrote:
> > On Fri, Nov 23, 2012 at 08:14:27PM +0800, Shawn Guo wrote:
> >> On Fri, Nov 23, 2012 at 08:57:26AM +0000, Russell King - ARM Linux wrote:
> >> > Does this work with the sdhci stuff?
> >> 
> >> Honestly, I'm not sure, but I guess it does, since I have seen
> >> sdhci-pxav3 driver using the helpers.  Anyway, I'm going to adopt
> >> the helpers for sdhci-esdhc-imx driver to find it out.
> >
> > The thing that worries me is this:
> >
> > static void sdhci_tasklet_card(unsigned long param)
> > {
> > ...
> >         /* Check host->mrq first in case we are runtime suspended */
> >         if (host->mrq &&
> >             !(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
> >                 pr_err("%s: Card removed during transfer!\n",
> >                         mmc_hostname(host->mmc));
> >                 pr_err("%s: Resetting controller.\n",
> >                         mmc_hostname(host->mmc));
> >
> >                 sdhci_reset(host, SDHCI_RESET_CMD);
> >                 sdhci_reset(host, SDHCI_RESET_DATA);
> >
> >                 host->mrq->cmd->error = -ENOMEDIUM;
> >                 tasklet_schedule(&host->finish_tasklet);
> >         }
> > ...
> >         mmc_detect_change(host->mmc, msecs_to_jiffies(200));
> > }
> >
> >
> > That gets called whenever a card is inserted/removed by the SDHCI code (if
> > the SDHCI card detect is wired), or in my case by the interrupt routine
> > the code in my patch adds.
> >
> > The slot-gpio.c stuff directly calls into mmc_detect_change (a) with a
> > shorter delay, and (b) completely omits the above handling and resetting.
> > My guess from the above code is that it'll work fine 90% of the time
> > (because you'll remove the card without an active request), but the above
> > code looks like it's addressing a corner case which will be omitted with
> > the "generic" slot-gpio.c solution.
> >
> > So I don't think it's a good idea to use slot-gpio.c in this case.
> 
> Guennadi, what are your thoughts about consolidating this reset logic
> between the sdhci tasklet and slot-gpio?  It would certainly be nice to
> use slot-gpio in cases like this one, so it's worth fixing.

Sure, this can be added. As for how - I see at least two possibilities: 
(1) put the complete above block in a new mmc host operation and just call 
it from the GPIO card-detect ISR, (2) taking into account, that many 
(nearly all? all?) host drivers keep a pointer to the current mrq in their 
private data struct, we could instead add such a pointer to struct 
mmc_host, then the check "request in progress" could also be generalised, 
and the operation would just have to reset the host and complete the 
request (in the sdhci case schedule the task).

Note, that there's already a .hw_reset() operation, used by sdhci-pci only 
so far, still, it seems we cannot (easily) hijack it.

So, what would be the preferred way?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Chris Ball]

Hi Guennadi,

On Mon, Nov 26 2012, Guennadi Liakhovetski wrote:
>> Guennadi, what are your thoughts about consolidating this reset logic
>> between the sdhci tasklet and slot-gpio?  It would certainly be nice to
>> use slot-gpio in cases like this one, so it's worth fixing.
>
> Sure, this can be added. As for how - I see at least two possibilities: 
> (1) put the complete above block in a new mmc host operation and just call 
> it from the GPIO card-detect ISR, (2) taking into account, that many 
> (nearly all? all?) host drivers keep a pointer to the current mrq in their 
> private data struct, we could instead add such a pointer to struct 
> mmc_host, then the check "request in progress" could also be generalised, 
> and the operation would just have to reset the host and complete the 
> request (in the sdhci case schedule the task).

They both sound pretty attractive.  Maybe we start out with (1), which
would create a patch we could more reasonably send to stable@ to get
slot-gpio handling the reset during transfers properly in older kernels,
and then refactor into (2) later?

> Note, that there's already a .hw_reset() operation, used by sdhci-pci
> only so far, still, it seems we cannot (easily) hijack it.

That one's implementing an eMMC 4.5 spec feature, not related to this
kind of reset.

Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Guennadi Liakhovetski]

Hi Chris

On Mon, 3 Dec 2012, Chris Ball wrote:

> Hi Guennadi,
> 
> On Mon, Nov 26 2012, Guennadi Liakhovetski wrote:
> >> Guennadi, what are your thoughts about consolidating this reset logic
> >> between the sdhci tasklet and slot-gpio?  It would certainly be nice to
> >> use slot-gpio in cases like this one, so it's worth fixing.
> >
> > Sure, this can be added. As for how - I see at least two possibilities: 
> > (1) put the complete above block in a new mmc host operation and just call 
> > it from the GPIO card-detect ISR, (2) taking into account, that many 
> > (nearly all? all?) host drivers keep a pointer to the current mrq in their 
> > private data struct, we could instead add such a pointer to struct 
> > mmc_host, then the check "request in progress" could also be generalised, 
> > and the operation would just have to reset the host and complete the 
> > request (in the sdhci case schedule the task).
> 
> They both sound pretty attractive.  Maybe we start out with (1), which
> would create a patch we could more reasonably send to stable@ to get
> slot-gpio handling the reset during transfers properly in older kernels,
> and then refactor into (2) later?

Just posted 3 patches for this, have a look if that's what you were 
thinking about. Not sure though why this is needed for stable, but I'm 
probably just missing some crucial information on the topic.

Thanks
Guennadi

> > Note, that there's already a .hw_reset() operation, used by sdhci-pci
> > only so far, still, it seems we cannot (easily) hijack it.
> 
> That one's implementing an eMMC 4.5 spec feature, not related to this
> kind of reset.
> 
> Thanks!
> 
> - Chris.
> -- 
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Chris Ball]

Hi Guennadi,

On Tue, Dec 04 2012, Guennadi Liakhovetski wrote:
>> They both sound pretty attractive.  Maybe we start out with (1), which
>> would create a patch we could more reasonably send to stable@ to get
>> slot-gpio handling the reset during transfers properly in older kernels,
>> and then refactor into (2) later?
>
> Just posted 3 patches for this, have a look if that's what you were 
> thinking about. Not sure though why this is needed for stable, but I'm 
> probably just missing some crucial information on the topic.

Thanks!  I'll take a look.  I agree that this isn't definitely needed
in -stable, but I'm glad we have the option if someone finds that their
host isn't functioning after card removal during a transfer.

Russell, are you happy with switching sdhci-dove over to slot-gpio with
this patchset?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Russell King - ARM Linux]

On Tue, Dec 04, 2012 at 11:06:39AM -0500, Chris Ball wrote:
> Hi Guennadi,
> 
> On Tue, Dec 04 2012, Guennadi Liakhovetski wrote:
> >> They both sound pretty attractive.  Maybe we start out with (1), which
> >> would create a patch we could more reasonably send to stable@ to get
> >> slot-gpio handling the reset during transfers properly in older kernels,
> >> and then refactor into (2) later?
> >
> > Just posted 3 patches for this, have a look if that's what you were 
> > thinking about. Not sure though why this is needed for stable, but I'm 
> > probably just missing some crucial information on the topic.
> 
> Thanks!  I'll take a look.  I agree that this isn't definitely needed
> in -stable, but I'm glad we have the option if someone finds that their
> host isn't functioning after card removal during a transfer.
> 
> Russell, are you happy with switching sdhci-dove over to slot-gpio with
> this patchset?

I'd rather not until I've moved my cubox kernel tree to v3.7 (when it's
out.)  Keeping stuff straight between that tree and mainline is far from
easy - I'm having to maintain two independent patches for each change,
one against each kernel tree, one gets tested (the one in the cubox tree)
the other (against mainline) does not.

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Chris Ball]

Hi,

On Tue, Dec 04 2012, Russell King - ARM Linux wrote:
>> Russell, are you happy with switching sdhci-dove over to slot-gpio with
>> this patchset?
>
> I'd rather not until I've moved my cubox kernel tree to v3.7 (when it's
> out.)  Keeping stuff straight between that tree and mainline is far from
> easy - I'm having to maintain two independent patches for each change,
> one against each kernel tree, one gets tested (the one in the cubox tree)
> the other (against mainline) does not.

Okay -- I'll take the patchset as-is for 3.8, with a plan to move to
slot-gpio after you're caught up.  Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 3/3] MMC: sdhci-dove: allow GPIOs to be used for card detection on Dove

[Shawn Guo]

On Thu, Nov 22, 2012 at 11:55:51PM +0000, Russell King wrote:
> This commit taken from Rabeeh's Cubox kernel and re-worked for DT;
> Sebastian Hasselbrath is believed to be the original author.
> 
> Some Cuboxes require a GPIO for card detection; this implements the
> optional GPIO support for card detection.  This GPIO is logic 0 for
> card inserted.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/mmc/host/sdhci-dove.c |   73 +++++++++++++++++++++++++++++++++++++---
>  1 files changed, 67 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c
> index 4af64f3..e621448 100644
> --- a/drivers/mmc/host/sdhci-dove.c
> +++ b/drivers/mmc/host/sdhci-dove.c
> @@ -19,20 +19,30 @@
>   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
> -#include <linux/err.h>
> -#include <linux/io.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> -#include <linux/module.h>
> +#include <linux/gpio.h>
> +#include <linux/io.h>
>  #include <linux/mmc/host.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_gpio.h>
>  
>  #include "sdhci-pltfm.h"
>  
>  struct sdhci_dove_priv {
>  	struct clk *clk;
> +	int gpio_cd;
>  };
>  
> +static irqreturn_t sdhci_dove_carddetect_irq(int irq, void *data)
> +{
> +	struct sdhci_host *host = data;
> +
> +	tasklet_schedule(&host->card_tasklet);
> +	return IRQ_HANDLED;
> +}
> +
>  static u16 sdhci_dove_readw(struct sdhci_host *host, int reg)
>  {
>  	u16 ret;
> @@ -50,16 +60,25 @@ static u16 sdhci_dove_readw(struct sdhci_host *host, int reg)
>  
>  static u32 sdhci_dove_readl(struct sdhci_host *host, int reg)
>  {
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_dove_priv *priv = pltfm_host->priv;
>  	u32 ret;
>  
> +	ret = readl(host->ioaddr + reg);
> +
>  	switch (reg) {
>  	case SDHCI_CAPABILITIES:
> -		ret = readl(host->ioaddr + reg);
>  		/* Mask the support for 3.0V */
>  		ret &= ~SDHCI_CAN_VDD_300;
>  		break;
> -	default:
> -		ret = readl(host->ioaddr + reg);
> +	case SDHCI_PRESENT_STATE:
> +		if (gpio_is_valid(priv->gpio_cd)) {
> +			if (gpio_get_value(priv->gpio_cd) == 0)
> +				ret |= SDHCI_CARD_PRESENT;
> +			else
> +				ret &= ~SDHCI_CARD_PRESENT;
> +		}
> +		break;
>  	}
>  	return ret;
>  }
> @@ -94,6 +113,23 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
>  
>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>  
> +	if (pdev->dev.of_node) {
> +		priv->gpio_cd = of_get_named_gpio(pdev->dev.of_node,
> +						  "cd-gpios", 0);
> +	} else {
> +		priv->gpio_cd = -EINVAL;
> +	}
> +
> +	if (gpio_is_valid(priv->gpio_cd)) {
> +		ret = gpio_request(priv->gpio_cd, "sdhci-cd");
> +		if (ret) {
> +			dev_err(&pdev->dev, "card detect gpio request failed: %d\n",
> +				ret);
> +			return ret;
> +		}
> +		gpio_direction_input(priv->gpio_cd);
> +	}
> +
>  	host = sdhci_pltfm_init(pdev, &sdhci_dove_pdata);
>  	if (IS_ERR(host)) {
>  		ret = PTR_ERR(host);
> @@ -112,13 +148,33 @@ static int __devinit sdhci_dove_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_sdhci_add;
>  
> +	/*
> +	 * We must request the IRQ after sdhci_add_host(), as the tasklet only
> +	 * gets setup in sdhci_add_host() and we oops.
> +	 */
> +	if (gpio_is_valid(priv->gpio_cd)) {
> +		ret = request_irq(gpio_to_irq(priv->gpio_cd),
> +				  sdhci_dove_carddetect_irq,
> +				  IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +				  mmc_hostname(host->mmc), host);
> +		if (ret) {
> +			dev_err(&pdev->dev, "card detect irq request failed: %d\n",
> +				ret);
> +			goto err_request_irq;
> +		}
> +	}
> +
>  	return 0;
>  
> +err_request_irq:
> +	sdhci_remove_host(host, 0);
>  err_sdhci_add:
>  	if (!IS_ERR(priv->clk))
>  		clk_disable_unprepare(priv->clk);
>  	sdhci_pltfm_free(pdev);
>  err_sdhci_pltfm_init:
> +	if (gpio_is_valid(priv->gpio_cd))
> +		gpio_free(priv->gpio_cd);
>  	return ret;
>  }
>  
> @@ -130,6 +186,11 @@ static int __devexit sdhci_dove_remove(struct platform_device *pdev)
>  
>  	sdhci_pltfm_unregister(pdev);
>  
> +	if (gpio_is_valid(priv->gpio_cd)) {
> +		free_irq(gpio_to_irq(priv->gpio_cd), host);
> +		gpio_free(priv->gpio_cd);
> +	}
> +

You can probably use devm_gpio_request() and devm_request_irq() to save
these cleanups like what patch #1 is doing?

Shawn

>  	if (!IS_ERR(priv->clk))
>  		clk_disable_unprepare(priv->clk);
>  
> -- 
> 1.7.4.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html