[PATCH 2.6.20-rc2] Add a quirk to allow at least some ENE PCI SD card readers to work again

[PATCH 2.6.20-rc2] Add a quirk to allow at least some ENE PCI SD card readers to work again

[Darren Salt]

Add a quirk to allow at least some ENE PCI SD card readers to work again

Support for these devices was broken for 2.6.18-rc1 and later by commit
146ad66eac836c0b976c98f428d73e1f6a75270d, which added voltage level support.

This restores the previous behaviour for these devices (PCI ID 1524:0550).

Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>

diff -ur linux-2.6.20-rc.orig/drivers/mmc/sdhci.c linux-2.6.20-rc/drivers/mmc/sdhci.c
--- linux-2.6.20-rc.orig/drivers/mmc/sdhci.c	2006-12-30 15:34:11.000000000 +0000
+++ linux-2.6.20-rc/drivers/mmc/sdhci.c	2006-12-31 02:46:48.000000000 +0000
@@ -37,6 +37,7 @@
 #define SDHCI_QUIRK_FORCE_DMA				(1<<1)
 /* Controller doesn't like some resets when there is no card inserted. */
 #define SDHCI_QUIRK_NO_CARD_NO_RESET			(1<<2)
+#define SDHCI_QUIRK_FORCE_POWER				(1<<3)
 
 static const struct pci_device_id pci_ids[] __devinitdata = {
 	{
@@ -65,6 +66,14 @@
 		.driver_data	= SDHCI_QUIRK_FORCE_DMA,
 	},
 
+	{
+		.vendor		= PCI_VENDOR_ID_ENE,
+		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.driver_data	= SDHCI_QUIRK_FORCE_POWER,
+	},
+
 	{	/* Generic SD host controller */
 		PCI_DEVICE_CLASS((PCI_CLASS_SYSTEM_SDHCI << 8), 0xFFFF00)
 	},
@@ -671,6 +680,12 @@
 {
 	u8 pwr;
 
+	if (host->chip->quirks & SDHCI_QUIRK_FORCE_POWER) {
+		writeb((power != (unsigned short) -1) ? 0xFF : 0,
+			host->ioaddr + SDHCI_POWER_CONTROL);
+		goto out;
+	}
+
 	if (host->power == power)
 		return;
 
diff -ur linux-2.6.20-rc.orig/include/linux/pci_ids.h linux-2.6.20-rc/include/linux/pci_ids.h
--- linux-2.6.20-rc.orig/include/linux/pci_ids.h	2006-12-30 15:34:21.000000000 +0000
+++ linux-2.6.20-rc/include/linux/pci_ids.h	2006-12-31 02:46:48.000000000 +0000
@@ -1968,6 +1968,7 @@
 #define PCI_DEVICE_ID_TOPIC_TP560	0x0000
 
 #define PCI_VENDOR_ID_ENE		0x1524
+#define PCI_DEVICE_ID_ENE_CB712_SD	0x0550
 #define PCI_DEVICE_ID_ENE_1211		0x1211
 #define PCI_DEVICE_ID_ENE_1225		0x1225
 #define PCI_DEVICE_ID_ENE_1410		0x1410

-- 
| Darren Salt    | linux or ds at              | nr. Ashington, | Toon
| RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army
| + Buy local produce. Try to walk or cycle. TRANSPORT CAUSES GLOBAL WARMING.

I think therefore I create bugs.

Re: [PATCH 2.6.20-rc2] Add a quirk to allow at least some ENE PCI SD card readers to work again

[Pierre Ossman]

Darren Salt wrote:
> Add a quirk to allow at least some ENE PCI SD card readers to work again
> 
> Support for these devices was broken for 2.6.18-rc1 and later by commit
> 146ad66eac836c0b976c98f428d73e1f6a75270d, which added voltage level support.
> 
> This restores the previous behaviour for these devices (PCI ID 1524:0550).
> 
> Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>
> 

Oh? If this is the source of problems for ENE controllers then this is indeed a magnificent find. Good work.

I'd like to know a little more about it though:

- Exactly what errors where you seeing without this patch?

- The patch effectively sets only the highest power. Have you tried other bit combinations to figure out if all of these are really needed?

  (This also means that the current patch is broken as the limited voltage range needs to also be reported to the MMC layer).

- Could you change the patch so that it covers all ENE controllers and send it out for testing on sdhci-devel? That way we could see if there are any more ENE controllers that will benefit from this
quirk. Just remember to ask people for a lspci.

Again, very good work.

Rgds
-- 
     -- Pierre Ossman

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

Re: [PATCH 2.6.20-rc2] Add a quirk to allow at least some ENE PCI SD card readers to work again

[Darren Salt]

I demand that Pierre Ossman may or may not have written...

> Darren Salt wrote:
>> Add a quirk to allow at least some ENE PCI SD card readers to work again

>> Support for these devices was broken for 2.6.18-rc1 and later by commit
>> 146ad66eac836c0b976c98f428d73e1f6a75270d, which added voltage level
>> support.

>> This restores the previous behaviour for these devices (PCI ID 1524:0550).

>> Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>

> Oh? If this is the source of problems for ENE controllers then this is
> indeed a magnificent find. Good work.

> I'd like to know a little more about it though:

> - Exactly what errors where you seeing without this patch?

The device was recognised, but insertion of a card was effectively not being
noticed, with no messages in the kernel log. (I say 'effectively': interrupts
were definitely being received and processed.)

> - The patch effectively sets only the highest power. Have you tried other
> bit combinations to figure out if all of these are really needed?

I have now... bits 4 to 7 are ignored, so 0x0F is fine; and it's what's
needed since lower values don't work.

It turns out that the reader only supports one voltage (caps &
SDHCI_CAN_VDD_330 is true, the others are false) and the code was already
selecting the correct value for the hardware. So I tried reverting the patch
and ensuring that the first writeb() in sdhci_set_power() isn't done if the
second one will be - and things started working properly again.

> (This also means that the current patch is broken as the limited voltage
> range needs to also be reported to the MMC layer).

> - Could you change the patch so that it covers all ENE controllers and send
> it out for testing on sdhci-devel? That way we could see if there are any
> more ENE controllers that will benefit from this quirk. Just remember to
> ask people for a lspci.

Sent as a follow-up to this message.

(BTW, is there a version of Thunderbird which preserves Mail-Followup-To in
followups? I ask because I see that that header got lost...)

-- 
| Darren Salt    | linux or ds at              | nr. Ashington, | Toon
| RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army
| + Generate power using sun, wind, water, nuclear.      FORGET COAL AND OIL.

File already exists, 0:1

[PATCH 2.6.20-rc2] Add a quirk to allow ENE PCI SD card readers to work again

[Darren Salt]

Add a quirk to allow ENE PCI SD card readers to work again

Support for these devices was broken for 2.6.18-rc1 and later by commit
146ad66eac836c0b976c98f428d73e1f6a75270d, which added voltage level support.

This restores the previous behaviour for these devices by ensuring that when
the voltage is changed, only one write to set the voltage is performed.

It may be that both writes are needed if the voltage is being changed between
two non-zero values or that it's safe to ensure that only one write is done
if the hardware only supports one voltage; I don't know whether either is the
case nor can I test since I have only the one SD reader (1524:0550), and it
supports just the one voltage.

Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>

diff -ur linux-2.6.20-rc2.orig/drivers/mmc/sdhci.c linux-2.6.20-rc2/drivers/mmc/sdhci.c
--- linux-2.6.20-rc2.orig/drivers/mmc/sdhci.c	2006-12-30 15:34:11.000000000 +0000
+++ linux-2.6.20-rc2/drivers/mmc/sdhci.c	2006-12-31 16:43:50.000000000 +0000
@@ -37,6 +37,7 @@
 #define SDHCI_QUIRK_FORCE_DMA				(1<<1)
 /* 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)
 
 static const struct pci_device_id pci_ids[] __devinitdata = {
 	{
@@ -65,6 +66,16 @@
 		.driver_data	= SDHCI_QUIRK_FORCE_DMA,
 	},
 
+	{
+		.class		= PCI_CLASS_SYSTEM_SDHCI << 8,
+		.class_mask	= 0xFFFF00,
+		.vendor		= PCI_VENDOR_ID_ENE,
+		.device		= PCI_ANY_ID,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.driver_data	= SDHCI_QUIRK_SINGLE_POWER_WRITE,
+	},
+
 	{	/* Generic SD host controller */
 		PCI_DEVICE_CLASS((PCI_CLASS_SYSTEM_SDHCI << 8), 0xFFFF00)
 	},
@@ -669,16 +680,17 @@
 
 static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
 {
-	u8 pwr;
+	u8 pwr = 0;
 
 	if (host->power == power)
 		return;
 
-	writeb(0, host->ioaddr + SDHCI_POWER_CONTROL);
-
 	if (power == (unsigned short)-1)
 		goto out;
 
+	if ((host->chip->quirks & SDHCI_QUIRK_SINGLE_POWER_WRITE) == 0)
+		writeb(0, host->ioaddr + SDHCI_POWER_CONTROL);
+
 	pwr = SDHCI_POWER_ON;
 
 	switch (power) {

-- 
| Darren Salt    | linux or ds at              | nr. Ashington, | Toon
| RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army
|   Kill all extremists!

Confucius say: Many pages make thick book.

Re: [PATCH 2.6.20-rc2] Add a quirk to allow ENE PCI SD card readers to work again

[Darren Salt]

I demand that I definitely did write...

> Add a quirk to allow ENE PCI SD card readers to work again

> Support for these devices was broken for 2.6.18-rc1 and later by commit
> 146ad66eac836c0b976c98f428d73e1f6a75270d, which added voltage level
> support.

> This restores the previous behaviour for these devices by ensuring that
> when the voltage is changed, only one write to set the voltage is
> performed.
[snip]

Oops, forgot to ask - could people test and report back? lspci output (for
your device) may be helpful.

-- 
| Darren Salt    | d @ youmustbejoking,demon,co,uk | nr. Ashington, | Toon
| RISC OS, Linux | s   zap,tartarus,org            | Northumberland | Army
| + Travel less. Share transport more.           PRODUCE LESS CARBON DIOXIDE.

regular guy: n. A person who occurs at fixed or pre-arranged intervals.

Re: [PATCH 2.6.20-rc2] Add a quirk to allow ENE PCI SD card readers to work again

[Pierre Ossman]

Darren Salt wrote:
> Add a quirk to allow ENE PCI SD card readers to work again
> 

Ok, I think we can consider testing over. I will apply this upstream
with the original host mask (as we didn't get any reports of it helping
on other ene controllers).

Thanks again for doing the legwork on this.

Rgds
-- 
     -- Pierre Ossman

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

Re: [Sdhci-devel] [PATCH 2.6.20-rc2] Add a quirk to allow ENE PCI SD card readers to work again

[Pierre Ossman]

Hi Darren,

It has come to my attention that the current routine for setting power
is not compliant with the specification. As such, I'd like you to try
the following and see if removes the need for your patch:

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index dc1b04a..13ac2fe 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -685,23 +690,21 @@ static void sdhci_set_power(struct sdhci_host
*host, unsigned short power)
        if (power == (unsigned short)-1)
                goto out;

-       pwr = SDHCI_POWER_ON;
-
        switch (power) {
        case MMC_VDD_170:
        case MMC_VDD_180:
        case MMC_VDD_190:
-               pwr |= SDHCI_POWER_180;
+               pwr = SDHCI_POWER_180;
                break;
        case MMC_VDD_290:
        case MMC_VDD_300:
        case MMC_VDD_310:
-               pwr |= SDHCI_POWER_300;
+               pwr = SDHCI_POWER_300;
                break;
        case MMC_VDD_320:
        case MMC_VDD_330:
        case MMC_VDD_340:
-               pwr |= SDHCI_POWER_330;
+               pwr = SDHCI_POWER_330;
                break;
        default:
                BUG();
@@ -709,6 +712,10 @@ static void sdhci_set_power(struct sdhci_host
*host, unsigned short power)

        writeb(pwr, host->ioaddr + SDHCI_POWER_CONTROL);

+       pwr |= SDHCI_POWER_ON;
+
+       writeb(pwr, host->ioaddr + SDHCI_POWER_CONTROL);
+
 out:
        host->power = power;
 }


I'd appreciate if you could test this sooner rather than later as the
merge window is just around the corner.

Rgds
-- 
     -- Pierre Ossman

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

Re: [Sdhci-devel] [PATCH 2.6.20-rc2] Add a quirk to allow ENE PCI SD

[Darren Salt]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]

I demand that Pierre Ossman may or may not have written...

> Hi Darren,

> It has come to my attention that the current routine for setting power
> is not compliant with the specification. As such, I'd like you to try
> the following and see if removes the need for your patch:

Your patch was mangled by Thunderbird. IME, it always does this; you should
attach patches, not include them inline.

Still, it was easy enough to apply the patch manually.

> I'd appreciate if you could test this sooner rather than later as the merge
> window is just around the corner.

It doesn't work.

After applying my patch and fixing up the rejects, it still doesn't work. I
need to disable the first of the writeb() calls mentioned in the last hunk of
your patch for it to work again. I have the impression that the hardware
doesn't like the power-on bit not being set :-|

... hmm, it looks like there's a small bug in my patch: the label "out" needs
to be before the last writeb() otherwise, if power is -1, no write will
happen regardless. I'm attaching a fixed version along with an adapted
version of your patch.

Pierre, if you're happy to sign off the modified version of your patch, feel
free to convert my not-yet-signed-off-by into a normal signed-off-by.

-- 
| Darren Salt    | linux or ds at              | nr. Ashington, | Toon
| RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army
|   <URL:http://www.youmustbejoking.demon.co.uk/> (PGP 2.6, GPG keys)

Break up a relationship. Buy a computer.

[-- Attachment #2: add-a-quirk-to-allow-at-least-some-ENE-PCI-SD-card-readers-to-work-again.patch --]
[-- Type: text/plain, Size: 1957 bytes --]

Add a quirk to allow at least some ENE PCI SD card readers to work again

Support for these devices was broken for 2.6.18-rc1 and later by commit
146ad66eac836c0b976c98f428d73e1f6a75270d, which added voltage level support.

This restores the previous behaviour for these devices (PCI ID 1524:0550).

Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>

diff -ur linux-2.6.20-rc6.orig/drivers/mmc/sdhci.c linux-2.6.20-rc6/drivers/mmc/sdhci.c
--- linux-2.6.20-rc6.orig/drivers/mmc/sdhci.c	2006-12-30 15:34:11.000000000 +0000
+++ linux-2.6.20-rc6/drivers/mmc/sdhci.c	2007-02-02 20:04:54.000000000 +0000
@@ -37,6 +37,7 @@
 #define SDHCI_QUIRK_FORCE_DMA				(1<<1)
 /* 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)
 
 static const struct pci_device_id pci_ids[] __devinitdata = {
 	{
@@ -65,6 +66,16 @@
 		.driver_data	= SDHCI_QUIRK_FORCE_DMA,
 	},
 
+	{
+		.class		= PCI_CLASS_SYSTEM_SDHCI << 8,
+		.class_mask	= 0xFFFF00,
+		.vendor		= PCI_VENDOR_ID_ENE,
+		.device		= PCI_ANY_ID,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.driver_data	= SDHCI_QUIRK_SINGLE_POWER_WRITE,
+	},
+
 	{	/* Generic SD host controller */
 		PCI_DEVICE_CLASS((PCI_CLASS_SYSTEM_SDHCI << 8), 0xFFFF00)
 	},
@@ -669,16 +680,17 @@
 
 static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
 {
-	u8 pwr;
+	u8 pwr = 0;
 
 	if (host->power == power)
 		return;
 
-	writeb(0, host->ioaddr + SDHCI_POWER_CONTROL);
-
 	if (power == (unsigned short)-1)
 		goto out;
 
+	if ((host->chip->quirks & SDHCI_QUIRK_SINGLE_POWER_WRITE) == 0)
+		writeb(0, host->ioaddr + SDHCI_POWER_CONTROL);
+
 	pwr = SDHCI_POWER_ON;
 
 	switch (power) {
@@ -701,9 +713,9 @@
 		BUG();
 	}
 
+out:
 	writeb(pwr, host->ioaddr + SDHCI_POWER_CONTROL);
 
-out:
 	host->power = power;
 }
 

[-- Attachment #3: make-the-routine-for-setting-power-compliant-with-the-SDHCI-specification.patch --]
[-- Type: text/plain, Size: 1436 bytes --]

Make the routine for setting power compliant with the specification.

Certain ENE chips seem to be sufficiently non-compliant that they don't like
this change; however, the single-power-write quirk should be enough to handle
them (and is, in the case of at least one such device).

Not-yet-signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>

diff -ur linux-2.6.20-rc6.orig/drivers/mmc/sdhci.c linux-2.6.20-rc6/drivers/mmc/sdhci.c
--- linux-2.6.20-rc6.orig/drivers/mmc/sdhci.c	2007-02-02 20:04:54.000000000 +0000
+++ linux-2.6.20-rc6/drivers/mmc/sdhci.c	2007-02-02 20:14:28.000000000 +0000
@@ -691,28 +691,31 @@
 	if ((host->chip->quirks & SDHCI_QUIRK_SINGLE_POWER_WRITE) == 0)
 		writeb(0, host->ioaddr + SDHCI_POWER_CONTROL);
 
-	pwr = SDHCI_POWER_ON;
-
 	switch (power) {
 	case MMC_VDD_170:
 	case MMC_VDD_180:
 	case MMC_VDD_190:
-		pwr |= SDHCI_POWER_180;
+		pwr = SDHCI_POWER_180;
 		break;
 	case MMC_VDD_290:
 	case MMC_VDD_300:
 	case MMC_VDD_310:
-		pwr |= SDHCI_POWER_300;
+		pwr = SDHCI_POWER_300;
 		break;
 	case MMC_VDD_320:
 	case MMC_VDD_330:
 	case MMC_VDD_340:
-		pwr |= SDHCI_POWER_330;
+		pwr = SDHCI_POWER_330;
 		break;
 	default:
 		BUG();
 	}
 
+	if ((host->chip->quirks & SDHCI_QUIRK_SINGLE_POWER_WRITE) == 0)
+		writeb(pwr, host->ioaddr + SDHCI_POWER_CONTROL);
+
+	pwr |= SDHCI_POWER_ON;
+
 out:
 	writeb(pwr, host->ioaddr + SDHCI_POWER_CONTROL);
 

Re: [Sdhci-devel] [PATCH 2.6.20-rc2] Add a quirk to allow ENE PCI SD

[Pierre Ossman]

Darren Salt wrote:
> 
> Your patch was mangled by Thunderbird. IME, it always does this; you should
> attach patches, not include them inline.
> 
> Still, it was easy enough to apply the patch manually.
> 

It's braindead, I was lazy and hoping it wouldn't crap things up too
badly. (it usually works if the lines are short enough)

>> I'd appreciate if you could test this sooner rather than later as the merge
>> window is just around the corner.
> 
> It doesn't work.
> 
> After applying my patch and fixing up the rejects, it still doesn't work. I
> need to disable the first of the writeb() calls mentioned in the last hunk of
> your patch for it to work again. I have the impression that the hardware
> doesn't like the power-on bit not being set :-|
> 

Now this isn't good. It means that I have to controllers that require
conflicting deviations from the standard. What a pain.

> ... hmm, it looks like there's a small bug in my patch: the label "out" needs
> to be before the last writeb() otherwise, if power is -1, no write will
> happen regardless. I'm attaching a fixed version along with an adapted
> version of your patch.
> 

I already fixed up that before I committed it to my tree, so no sweat. )

> Pierre, if you're happy to sign off the modified version of your patch, feel
> free to convert my not-yet-signed-off-by into a normal signed-off-by.
> 

I'll rework that into a separate quirk to indicate it's because of
broken hardware. It's only for unreleased hw so it's no rush. I just
wanted to see if it was related to your problem.

Rgds
-- 
     -- Pierre Ossman

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

Re: [PATCH 2.6.20-rc2] Add a quirk to allow at least some ENE PCI SD card readers to work again

[Pierre Ossman]

Darren Salt wrote:
> 
> (BTW, is there a version of Thunderbird which preserves Mail-Followup-To in
> followups? I ask because I see that that header got lost...)
> 

No idea, I'm just a user. ;)

Its handling of Mail-Followup-To is generally rather annoying. It doesn't strip out my own email from the list, so a "Reply All" includes myself in the to list.

Rgds
-- 
     -- Pierre Ossman

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