cd-gpio and SDHCI presence

cd-gpio and SDHCI presence

[Chris Ball]

Hi Guennadi,

I'm having trouble using a cd-gpio.  After I request it, I get the
IRQ and mmc_rescan() triggers successfully, but mmc_attach_sd()
fails because every command returns -123 (ENOMEDIUM), due to:

sdhci.c:
	/* If polling, assume that the card is always present. */
	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
		present = true;
	else
		present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
				SDHCI_CARD_PRESENT;

	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
		host->mrq->cmd->error = -ENOMEDIUM;
		tasklet_schedule(&host->finish_tasklet);
	} else { 
		...

"present" is false because SDHCI_PRESENT_STATE doesn't have the presence
bit sent (if it did, we wouldn't need a cd-gpio, right?) and the command
fails.

And we can't just set SDHCI_QUIRK_BROKEN_CARD_DETECTION, because that
would turn on polling, which we don't want.  How's this supposed to work?

Maybe your system was differently broken, and we need some new way to
communicate that neither the PRESENT bit nor polling should be used?

Thanks,

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

Re: cd-gpio and SDHCI presence

[Guennadi Liakhovetski]

Hi Chris

On Mon, 17 Sep 2012, Chris Ball wrote:

> Hi Guennadi,
> 
> I'm having trouble using a cd-gpio.  After I request it, I get the
> IRQ and mmc_rescan() triggers successfully, but mmc_attach_sd()
> fails because every command returns -123 (ENOMEDIUM), due to:
> 
> sdhci.c:
> 	/* If polling, assume that the card is always present. */
> 	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
> 		present = true;
> 	else
> 		present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
> 				SDHCI_CARD_PRESENT;
> 
> 	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
> 		host->mrq->cmd->error = -ENOMEDIUM;
> 		tasklet_schedule(&host->finish_tasklet);
> 	} else { 
> 		...
> 
> "present" is false because SDHCI_PRESENT_STATE doesn't have the presence
> bit sent (if it did, we wouldn't need a cd-gpio, right?) and the command
> fails.
> 
> And we can't just set SDHCI_QUIRK_BROKEN_CARD_DETECTION, because that
> would turn on polling, which we don't want.  How's this supposed to work?

Sorry, I'm not very familiar with the SDHCI hardware or the driver, so, 
I'll be speculating here.

cd-gpio can function in two modes: polling and IRQ. IRQ is enabled if a 
number of conditions is satisfied: gpio_to_irq() returns a non-negative 
number, the user doesn't force polling by setting MMC_CAP_NEEDS_POLL and 
request(_threaded)_irq() is successful. If either of the three conditions 
fail, GPIO polling will be used.

Now, concerning SDHCI. cd-gpio doesn't magically make all MMC drivers 
capable of using GPIO card detection, sorry:-) To use it you should first 
make sure your driver can take card-present or card-detect information 
from a GPIO. Then the slot-gpio module shall be used to add this GPIO CD 
functionality to your driver instead of cooking it up by yourself.

>From the above it seems to me, that your SDHCI instance is not yet quite 
trained to take GPIO input for card-detection:-) Looking through various 
SDHCI implementations I see several drivers already implementing and using 
various GPIO slot services internally: tegra, spear, s3c, pci, esdhc-imx. 
They all seem to work with their own GPIO CD implementations, which are 
very similar to the slot-gpio one and it shouldn't be too difficult to 
replace them with the latter.

Concerning how specifically it should work in your case - I'm not sure, 
sorry. What platform are you working with? Does it already have GPIO 
support? If it did, it should have been pretty simple to replace it with 
the generic one from slot-gpio. Looking at the code, it seems to me, that 
even on SDHCI systems, where a GPIO is used for card detection, the 
controller is sometimes still able to recognise the card's presence. This 
seems to be the case on tegra, pci (except cafe),... However, e.g., s3c 
uses a dirty hack of setting and clearing 
SDHCI_QUIRK_BROKEN_CARD_DETECTION in their CD routine... In general, I 
think, it should suffice to do in sdhci_request() (and others) something 
like

	/* If polling, assume that the card is always present. */
	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
		present = true;
	else
		present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
				SDHCI_CARD_PRESENT;

	if (!present) {
		int ret = mmc_gpio_get_cd(host->mmc);
		if (ret > 0)
			present = true;
	}

Also, once a card has been detected, shouldn't it be possible to use 
mmc_card_present() to check its presence?

Thanks
Guennadi

> Maybe your system was differently broken, and we need some new way to
> communicate that neither the PRESENT bit nor polling should be used?
> 
> 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/

Re: cd-gpio and SDHCI presence

[Chris Ball]

Hi Guennadi, thanks for replying so quickly,

On Mon, Sep 17 2012, Guennadi Liakhovetski wrote:
>> "present" is false because SDHCI_PRESENT_STATE doesn't have the presence
>> bit sent (if it did, we wouldn't need a cd-gpio, right?) and the command
>> fails.
>> 
>> And we can't just set SDHCI_QUIRK_BROKEN_CARD_DETECTION, because that
>> would turn on polling, which we don't want.  How's this supposed to work?
>
> Sorry, I'm not very familiar with the SDHCI hardware or the driver, so, 
> I'll be speculating here.
>
> cd-gpio can function in two modes: polling and IRQ. IRQ is enabled if a 
> number of conditions is satisfied: gpio_to_irq() returns a non-negative 
> number, the user doesn't force polling by setting MMC_CAP_NEEDS_POLL and 
> request(_threaded)_irq() is successful. If either of the three conditions 
> fail, GPIO polling will be used.

Right, I'm in IRQ mode.

> Now, concerning SDHCI. cd-gpio doesn't magically make all MMC drivers 
> capable of using GPIO card detection, sorry:-) To use it you should first 
> make sure your driver can take card-present or card-detect information 
> from a GPIO. Then the slot-gpio module shall be used to add this GPIO CD 
> functionality to your driver instead of cooking it up by yourself.
>
> From the above it seems to me, that your SDHCI instance is not yet quite 
> trained to take GPIO input for card-detection:-) Looking through various 
> SDHCI implementations I see several drivers already implementing and using 
> various GPIO slot services internally: tegra, spear, s3c, pci, esdhc-imx. 
> They all seem to work with their own GPIO CD implementations, which are 
> very similar to the slot-gpio one and it shouldn't be too difficult to 
> replace them with the latter.

I'm using sdhci-pxav3, which is a small sdhci-pltfm driver.

I see now, thanks; what I was missing is that the other drivers are either:

* using IO accessors to fake the presence bit being set (esdhc-imx)
* using the ugly hack of just falling back to polling (s3c)
* running on a controller that doesn't have this problem (tegra?)

So the answer to "how's this supposed to work?" was "your driver needs
a workaround for reporting the presence bit", I guess.  Hopefully making
the change below should let us remove this unnecessary code from
esdhc-imx and s3c.

> Concerning how specifically it should work in your case - I'm not sure, 
> sorry. What platform are you working with? Does it already have GPIO 
> support? If it did, it should have been pretty simple to replace it with 
> the generic one from slot-gpio. Looking at the code, it seems to me, that 
> even on SDHCI systems, where a GPIO is used for card detection, the 
> controller is sometimes still able to recognise the card's presence. This 
> seems to be the case on tegra, pci (except cafe),... However, e.g., s3c 
> uses a dirty hack of setting and clearing 
> SDHCI_QUIRK_BROKEN_CARD_DETECTION in their CD routine... In general, I 
> think, it should suffice to do in sdhci_request() (and others) something 
> like
>
> 	/* If polling, assume that the card is always present. */
> 	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
> 		present = true;
> 	else
> 		present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
> 				SDHCI_CARD_PRESENT;
>
> 	if (!present) {
> 		int ret = mmc_gpio_get_cd(host->mmc);
> 		if (ret > 0)
> 			present = true;
> 	}

Makes sense, and works fine.  I'll prepare a formal patch and send it
out now -- please could you reply with your Signed-off-by, since I'll
put you in the From field?

> Also, once a card has been detected, shouldn't it be possible to use 
> mmc_card_present() to check its presence?

Sounds right, although that happens too late to help here.

Thanks very much!

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

[PATCH] mmc: sdhci: Test cd-gpio instead of SDHCI presence when probing

[Chris Ball]

From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Previously to this patch, an SDHCI platform that uses a GPIO for
card detection instead of the internal SDHCI_CARD_PRESENT bit on
the presence register would fail to bring up a new card because
logic in sdhci_request() fails the request if that bit is 0.

Some drivers worked around this in various ways: esdhc-imx defines
an IO accessor to fake the presence bit being true, s3c turns on
polling (which stops the SDHCI driver from checking the bit) after
a card's inserted.  But none of this should be necessary; the real
fix is to check whether we're using a GPIO and avoid relying on
the presence bit if so, as this patch implements.

Signed-off-by: Chris Ball <cjb@laptop.org>
---
 drivers/mmc/host/sdhci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d98b199..0e15c79 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -28,6 +28,7 @@
 #include <linux/mmc/mmc.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/slot-gpio.h>
 
 #include "sdhci.h"
 
@@ -1293,6 +1294,13 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
 				SDHCI_CARD_PRESENT;
 
+	/* If we're using a cd-gpio, testing the presence bit might fail. */
+	if (!present) {
+		int ret = mmc_gpio_get_cd(host->mmc);
+		if (ret > 0)
+			present = true;
+	}
+
 	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
 		host->mrq->cmd->error = -ENOMEDIUM;
 		tasklet_schedule(&host->finish_tasklet);
-- 
1.7.11.2



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

Re: [PATCH] mmc: sdhci: Test cd-gpio instead of SDHCI presence when probing

[Guennadi Liakhovetski]

On Mon, 17 Sep 2012, Chris Ball wrote:

> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> Previously to this patch, an SDHCI platform that uses a GPIO for
> card detection instead of the internal SDHCI_CARD_PRESENT bit on
> the presence register would fail to bring up a new card because
> logic in sdhci_request() fails the request if that bit is 0.
> 
> Some drivers worked around this in various ways: esdhc-imx defines
> an IO accessor to fake the presence bit being true, s3c turns on
> polling (which stops the SDHCI driver from checking the bit) after
> a card's inserted.  But none of this should be necessary; the real
> fix is to check whether we're using a GPIO and avoid relying on
> the presence bit if so, as this patch implements.
> 

Well, ok, thanks for attributing this patch to me:-) I guess then my Sob 
should go in the first, as you'll be forwarding the patch upstream? But 
feel free to swap them if you disagree

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> Signed-off-by: Chris Ball <cjb@laptop.org>
> ---
>  drivers/mmc/host/sdhci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index d98b199..0e15c79 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -28,6 +28,7 @@
>  #include <linux/mmc/mmc.h>
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/slot-gpio.h>
>  
>  #include "sdhci.h"
>  
> @@ -1293,6 +1294,13 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  		present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
>  				SDHCI_CARD_PRESENT;
>  
> +	/* If we're using a cd-gpio, testing the presence bit might fail. */
> +	if (!present) {
> +		int ret = mmc_gpio_get_cd(host->mmc);
> +		if (ret > 0)
> +			present = true;
> +	}
> +
>  	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
>  		host->mrq->cmd->error = -ENOMEDIUM;
>  		tasklet_schedule(&host->finish_tasklet);
> -- 
> 1.7.11.2
> 
> 
> 
> - 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/

Re: [PATCH] mmc: sdhci: Test cd-gpio instead of SDHCI presence when probing

[Chris Ball]

Hi,

On Mon, Sep 17 2012, Guennadi Liakhovetski wrote:
> On Mon, 17 Sep 2012, Chris Ball wrote:
>
>> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> 
>> Previously to this patch, an SDHCI platform that uses a GPIO for
>> card detection instead of the internal SDHCI_CARD_PRESENT bit on
>> the presence register would fail to bring up a new card because
>> logic in sdhci_request() fails the request if that bit is 0.
>> 
>> Some drivers worked around this in various ways: esdhc-imx defines
>> an IO accessor to fake the presence bit being true, s3c turns on
>> polling (which stops the SDHCI driver from checking the bit) after
>> a card's inserted.  But none of this should be necessary; the real
>> fix is to check whether we're using a GPIO and avoid relying on
>> the presence bit if so, as this patch implements.
>> 
>
> Well, ok, thanks for attributing this patch to me:-) I guess then my Sob 
> should go in the first, as you'll be forwarding the patch upstream? But 
> feel free to swap them if you disagree
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Yes, I'll put yours in first.  Pushed to mmc-next for 3.7; thanks!

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