cd-gpio and SDHCI presence

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?

From the above it seems to me, that your SDHCI instance is not yet quite

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.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Quoted 6 lines

"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?

From: Guennadi Liakhovetski <[email protected]>

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 <[email protected]>

---
 drivers/mmc/host/sdhci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/mmc/host/sdhci.c +8 -0

index d98b199..0e15c79 100644
@@ -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);

Signature · 8 lines

1.7.11.2



- Chris.
-- 
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Quoted 7 lines · Chris Ball

From: Guennadi Liakhovetski <[email protected]>

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.

Quoted 7 lines

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 <[email protected]>