sdhci: Add quirk to support polling for card presence

PATCH In review Philip Langdale 3 participants 5 messages May 13, 2007 – May 18, 2007

Subsystems (3)

PCI MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Philip Langdale

author

2007-05-13 23:21 UTC #0 lore raw

PATCH 1/1 sdhci: Add quirk to support polling for card presence

There is apparently at least one instance of the Ricoh SDHCI implementation out there where card insertion (and possibly removal) interrupts do not work - so the only way to detect a change in the presence of a card is to poll.

This changes adds a polling quirk for the particular model reported by Tobias. The change is tentative as it assumes that removal interrupts do not work, but if they do work, then we can optimise things by not polling when there is a card present. So, it should not be committed until Tobias has confirmed the removal behaviour one way or the other and the change is accordingly updated.

The polling frequency is 3 seconds because I've been running powertop and am paranoid about wakeups now. :-) I did some testing locally by masking out the interrupt events and 3 seconds seems usable enough to me.

Signed-off-by Philip Langdale <[email protected]>

drivers/mmc/sdhci.c +46 -5

index ff5bf73..8c5b2f5 100644
@@ -34,6 +34,7 @@ static unsigned int debug_quirks = 0;
 /* Controller doesn't like some resets when there is no card inserted. */
 #define SDHCI_QUIRK_NO_CARD_NO_RESET			(1<<2)
 #define SDHCI_QUIRK_SINGLE_POWER_WRITE			(1<<3)
+#define SDHCI_QUIRK_NO_CARD_DETECT_INT			(1<<4)
 
 static const struct pci_device_id pci_ids[] __devinitdata = {
 	{

@@ -48,6 +49,16 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	{
 		.vendor		= PCI_VENDOR_ID_RICOH,
 		.device		= PCI_DEVICE_ID_RICOH_R5C822,
+		.subvendor	= PCI_SUBVENDOR_ID_FUJITSU_SIEMENS,
+		.subdevice	= PCI_SUBDEVICE_ID_FUJITSU_SIEMENS_SI1520,
+		.driver_data	= SDHCI_QUIRK_FORCE_DMA |
+				  SDHCI_QUIRK_NO_CARD_NO_RESET |
+				  SDHCI_QUIRK_NO_CARD_DETECT_INT,
+	},
+
+	{
+		.vendor		= PCI_VENDOR_ID_RICOH,
+		.device		= PCI_DEVICE_ID_RICOH_R5C822,
 		.subvendor	= PCI_ANY_ID,
 		.subdevice	= PCI_ANY_ID,
 		.driver_data	= SDHCI_QUIRK_FORCE_DMA |

@@ -788,13 +799,15 @@ static void sdhci_tasklet_card(unsigned long param)
 {
 	struct sdhci_host *host;
 	unsigned long flags;
+	int present;
 
 	host = (struct sdhci_host*)param;
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	if (!(readl(host->ioaddr + SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
-		if (host->mrq) {
+	present = readl(host->ioaddr + SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT;
+	if (present != host->present) {
+		if (!present && host->mrq) {
 			printk(KERN_ERR "%s: Card removed during transfer!\n",
 				mmc_hostname(host->mmc));
 			printk(KERN_ERR "%s: Resetting controller.\n",

@@ -806,11 +819,18 @@ static void sdhci_tasklet_card(unsigned long param)
 			host->mrq->cmd->error = MMC_ERR_FAILED;
 			tasklet_schedule(&host->finish_tasklet);
 		}
-	}
 
-	spin_unlock_irqrestore(&host->lock, flags);
+		spin_unlock_irqrestore(&host->lock, flags);
+
+		host->present = present;
+
+		mmc_detect_change(host->mmc, msecs_to_jiffies(500));
+	}
 
-	mmc_detect_change(host->mmc, msecs_to_jiffies(500));
+	if (host->chip->quirks & SDHCI_QUIRK_NO_CARD_DETECT_INT) {
+		host->detect_timer.expires = jiffies + 3 * HZ;
+		add_timer(&host->detect_timer);
+	}
 }
 
 static void sdhci_tasklet_finish(unsigned long param)

@@ -865,6 +885,15 @@ static void sdhci_tasklet_finish(unsigned long param)
 	mmc_request_done(host->mmc, mrq);
 }
 
+static void sdhci_detect_timer(unsigned long data)
+{
+	struct sdhci_host *host;
+
+	host = (struct sdhci_host*)data;
+
+	tasklet_schedule(&host->card_tasklet);
+}
+
 static void sdhci_timeout_timer(unsigned long data)
 {
 	struct sdhci_host *host;

@@ -1329,6 +1358,8 @@ static int __devinit sdhci_probe_slot(struct pci_dev *pdev, int slot)
 	 */
 	mmc->max_blk_count = 65535;
 
+	host->present = readl(host->ioaddr + SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT;;
+
 	/*
 	 * Init tasklets.
 	 */

@@ -1344,6 +1375,12 @@ static int __devinit sdhci_probe_slot(struct pci_dev *pdev, int slot)
 	if (ret)
 		goto untasklet;
 
+	if (host->chip->quirks & SDHCI_QUIRK_NO_CARD_DETECT_INT) {
+		setup_timer(&host->detect_timer, sdhci_detect_timer, (unsigned long)host);
+		host->detect_timer.expires = jiffies + 3 * HZ;
+		add_timer(&host->detect_timer);
+	}
+
 	sdhci_init(host);
 
 #ifdef CONFIG_MMC_DEBUG

@@ -1396,6 +1433,10 @@ static void sdhci_remove_slot(struct pci_dev *pdev, int slot)
 	tasklet_kill(&host->card_tasklet);
 	tasklet_kill(&host->finish_tasklet);
 
+	if (host->chip->quirks & SDHCI_QUIRK_NO_CARD_DETECT_INT) {
+		del_timer_sync(&host->detect_timer);
+	}
+
 	iounmap(host->ioaddr);
 
 	pci_release_region(pdev, host->bar);

drivers/mmc/sdhci.h +3 -0

index 7400f4b..fafd1c4 100644
@@ -198,6 +198,9 @@ struct sdhci_host {
 	struct tasklet_struct	finish_tasklet;
 
 	struct timer_list	timer;		/* Timer for timeouts */
+
+	struct timer_list	detect_timer;	/* Timer for card polling */
+	int			present;	/* Cached card present state */
 };
 
 struct sdhci_chip {

include/linux/pci_ids.h +3 -0

index ae849f0..e10eeaf 100644
@@ -2049,6 +2049,9 @@
 #define PCI_VENDOR_ID_VITESSE		0x1725
 #define PCI_DEVICE_ID_VITESSE_VSC7174	0x7174
 
+#define PCI_SUBVENDOR_ID_FUJITSU_SIEMENS	0x1734
+#define PCI_SUBDEVICE_ID_FUJITSU_SIEMENS_SI1520	0x10ad
+
 #define PCI_VENDOR_ID_LINKSYS		0x1737
 #define PCI_DEVICE_ID_LINKSYS_EG1064	0x1064

Pierre Ossman

2007-05-14 10:30 UTC #1 lore raw

drivers/mmc/sdhci.c static void sdhci_tasklet_card(unsigned long param) · line 825

Quoted 10 lines · Philip Langdale

drivers/mmc/sdhci.c +2 -2

@@ -806,11 +819,18 @@ static void sdhci_tasklet_card(unsigned long param)
 			host->mrq->cmd->error = MMC_ERR_FAILED;
 			tasklet_schedule(&host->finish_tasklet);
 		}
-	}
 
-	spin_unlock_irqrestore(&host->lock, flags);
+		spin_unlock_irqrestore(&host->lock, flags);
+

Where's the unlock for the other branch?

drivers/mmc/sdhci.c static void sdhci_tasklet_card(unsigned long param) · line 835

Quoted 3 lines

drivers/mmc/sdhci.c +1 -0
+		host->present = present;
 
 

And this should be protected by the lock :)

drivers/mmc/sdhci.c static void sdhci_tasklet_card(unsigned long param) · line 829

Quoted 3 lines

drivers/mmc/sdhci.c +2 -0

+		mmc_detect_change(host->mmc, msecs_to_jiffies(500));
+	}

Perhaps we should have a different delay here. After all, we're no longer delaying against an interrupt.

drivers/mmc/sdhci.c static void sdhci_tasklet_card(unsigned long param) · line 836

Quoted 9 lines

drivers/mmc/sdhci.c +4 -1

-	mmc_detect_change(host->mmc, msecs_to_jiffies(500));
+	if (host->chip->quirks & SDHCI_QUIRK_NO_CARD_DETECT_INT) {
+		host->detect_timer.expires = jiffies + 3 * HZ;
+		add_timer(&host->detect_timer);
+	}
 }
 
 static void sdhci_tasklet_finish(unsigned long param)

I wonder if three seconds is a bit much. You might give up and yank the card in that time.

Keep up the good work. :)

Rgds

Signature · 5 lines

     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Philip Langdale

author

2007-05-15 03:22 UTC #2 lore raw

There is apparently at least one instance of the Ricoh SDHCI implementation out there where card insertion and removal interrupts do not work - so the only way to detect a change in the presence of a card is to poll.

This changes adds a polling quirk for the particular model reported. Others may be affected, but we can add them as they arise.

As part of this change, caching of the present state is introduced to reduce the amount of work done when nothing has changed. This cached state is referred to in the standard interrupt path but as it is interrupt driven - the cached and new state should never be the same so it will cause no change in behaviour.

Signed-off-by Philip Langdale <[email protected]>

drivers/mmc/host/sdhci.c +52 -5

index ff5bf73..2d5ec61 100644
@@ -34,6 +34,7 @@ static unsigned int debug_quirks = 0;
 /* Controller doesn't like some resets when there is no card inserted. */
 #define SDHCI_QUIRK_NO_CARD_NO_RESET			(1<<2)
 #define SDHCI_QUIRK_SINGLE_POWER_WRITE			(1<<3)
+#define SDHCI_QUIRK_NO_CARD_DETECT_INT			(1<<4)
 
 static const struct pci_device_id pci_ids[] __devinitdata = {
 	{

@@ -48,6 +49,16 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	{
 		.vendor		= PCI_VENDOR_ID_RICOH,
 		.device		= PCI_DEVICE_ID_RICOH_R5C822,
+		.subvendor	= PCI_SUBVENDOR_ID_FUJITSU_SIEMENS,
+		.subdevice	= PCI_SUBDEVICE_ID_FUJITSU_SIEMENS_SI1520,
+		.driver_data	= SDHCI_QUIRK_FORCE_DMA |
+				  SDHCI_QUIRK_NO_CARD_NO_RESET |
+				  SDHCI_QUIRK_NO_CARD_DETECT_INT,
+	},
+
+	{
+		.vendor		= PCI_VENDOR_ID_RICOH,
+		.device		= PCI_DEVICE_ID_RICOH_R5C822,
 		.subvendor	= PCI_ANY_ID,
 		.subdevice	= PCI_ANY_ID,
 		.driver_data	= SDHCI_QUIRK_FORCE_DMA |

@@ -788,13 +799,18 @@ static void sdhci_tasklet_card(unsigned long param)
 {
 	struct sdhci_host *host;
 	unsigned long flags;
+	int present;
+	int polling;
 
 	host = (struct sdhci_host*)param;
 
+	polling = host->chip->quirks & SDHCI_QUIRK_NO_CARD_DETECT_INT;
+
 	spin_lock_irqsave(&host->lock, flags);
 
-	if (!(readl(host->ioaddr + SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
-		if (host->mrq) {
+	present = readl(host->ioaddr + SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT;
+	if (present != host->present) {
+		if (!present && host->mrq) {
 			printk(KERN_ERR "%s: Card removed during transfer!\n",
 				mmc_hostname(host->mmc));
 			printk(KERN_ERR "%s: Resetting controller.\n",

@@ -806,11 +822,21 @@ static void sdhci_tasklet_card(unsigned long param)
 			host->mrq->cmd->error = MMC_ERR_FAILED;
 			tasklet_schedule(&host->finish_tasklet);
 		}
-	}
 
-	spin_unlock_irqrestore(&host->lock, flags);
+		host->present = present;
+
+		spin_unlock_irqrestore(&host->lock, flags);
+
+		mmc_detect_change(host->mmc,
+				  polling ? 0 : msecs_to_jiffies(500));
+	} else {
+		spin_unlock_irqrestore(&host->lock, flags);
+	}
 
-	mmc_detect_change(host->mmc, msecs_to_jiffies(500));
+	if (polling) {
+		host->detect_timer.expires = jiffies + HZ;
+		add_timer(&host->detect_timer);
+	}
 }
 
 static void sdhci_tasklet_finish(unsigned long param)

@@ -865,6 +891,15 @@ static void sdhci_tasklet_finish(unsigned long param)
 	mmc_request_done(host->mmc, mrq);
 }
 
+static void sdhci_detect_timer(unsigned long data)
+{
+	struct sdhci_host *host;
+
+	host = (struct sdhci_host*)data;
+
+	tasklet_schedule(&host->card_tasklet);
+}
+
 static void sdhci_timeout_timer(unsigned long data)
 {
 	struct sdhci_host *host;

@@ -1329,6 +1364,8 @@ static int __devinit sdhci_probe_slot(struct pci_dev *pdev, int slot)
 	 */
 	mmc->max_blk_count = 65535;
 
+	host->present = readl(host->ioaddr + SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT;;
+
 	/*
 	 * Init tasklets.
 	 */

@@ -1344,6 +1381,12 @@ static int __devinit sdhci_probe_slot(struct pci_dev *pdev, int slot)
 	if (ret)
 		goto untasklet;
 
+	if (host->chip->quirks & SDHCI_QUIRK_NO_CARD_DETECT_INT) {
+		setup_timer(&host->detect_timer, sdhci_detect_timer, (unsigned long)host);
+		host->detect_timer.expires = jiffies + HZ;
+		add_timer(&host->detect_timer);
+	}
+
 	sdhci_init(host);
 
 #ifdef CONFIG_MMC_DEBUG

@@ -1396,6 +1439,10 @@ static void sdhci_remove_slot(struct pci_dev *pdev, int slot)
 	tasklet_kill(&host->card_tasklet);
 	tasklet_kill(&host->finish_tasklet);
 
+	if (host->chip->quirks & SDHCI_QUIRK_NO_CARD_DETECT_INT) {
+		del_timer_sync(&host->detect_timer);
+	}
+
 	iounmap(host->ioaddr);
 
 	pci_release_region(pdev, host->bar);

drivers/mmc/host/sdhci.h +3 -0

index 7400f4b..fafd1c4 100644
@@ -198,6 +198,9 @@ struct sdhci_host {
 	struct tasklet_struct	finish_tasklet;
 
 	struct timer_list	timer;		/* Timer for timeouts */
+
+	struct timer_list	detect_timer;	/* Timer for card polling */
+	int			present;	/* Cached card present state */
 };
 
 struct sdhci_chip {

include/linux/pci_ids.h +3 -0

index ae849f0..e10eeaf 100644
@@ -2049,6 +2049,9 @@
 #define PCI_VENDOR_ID_VITESSE		0x1725
 #define PCI_DEVICE_ID_VITESSE_VSC7174	0x7174
 
+#define PCI_SUBVENDOR_ID_FUJITSU_SIEMENS	0x1734
+#define PCI_SUBDEVICE_ID_FUJITSU_SIEMENS_SI1520	0x10ad
+
 #define PCI_VENDOR_ID_LINKSYS		0x1737
 #define PCI_DEVICE_ID_LINKSYS_EG1064	0x1064

Pierre Ossman

2007-05-17 23:01 UTC #3 lore raw

In reply to Philip Langdale

Tobias, did you test this patch and did it solve your problem?

Quoted 13 lines · Philip Langdale

There is apparently at least one instance of the Ricoh SDHCI implementation out there where card insertion (and possibly removal) interrupts do not work - so the only way to detect a change in the presence of a card is to poll.
This changes adds a polling quirk for the particular model reported by Tobias. The change is tentative as it assumes that removal interrupts do not work, but if they do work, then we can optimise things by not polling when there is a card present. So, it should not be committed until Tobias has confirmed the removal behaviour one way or the other and the change is accordingly updated.

Quoted 8 lines

The polling frequency is 3 seconds because I've been running powertop and am paranoid about wakeups now. :-) I did some testing locally by masking out the interrupt events and 3 seconds seems usable enough to me.
Signed-off-by Philip Langdale <[email protected]>

Rgds

Signature · 5 lines

     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Tobias Bengtsson

2007-05-18 10:58 UTC #4 lore raw

Pierre Ossman skrev:

Quoted 2 lines

Tobias, did you test this patch and did it solve your problem?

I compiled it with 2.6.21-mm2 and 2.6.22-rc1 but both of them hangs at startup after setting up portmap. So atleast init starts.. I suppose I should set up remote syslog to debug.. and ofcourse apply the patch to a vanilla 2.6.21, but right now I have a lot to do.

I know at least one other person with the same hardware who also run linux, [email protected] , perhaps someone could help him apply the patch if he is williing..

regards Tobias