I was pulled in to help with ath6kl development work. As part
of this I was given some PCI "ENE" SDHCI card which can I can slap
on to my desktop. I was then given this dump of patches to apply
to sdhci code to get it to work:
I prefer code upstream though so here is my split and cleanup
of these patches. I've tested it with my ENE card. Since I am
new to SDHCI I would prefer someone with more experience review
these chagnes and the respective commit log messages. I've
tried to make sense of them as best as I can. Unfortunately
I was not given any explanation to these changes except the
patch I got so I tried to make the best of it.
Luis R. Rodriguez (4):
sdhci: allow for multiple delays when sending commands
sdhci: sanity check for triggering interrupts thread
sdhci: cache when the MMC bus width is 4 bits wide
sdhci: add quick for controllers with for CIRQ issues
drivers/mmc/host/sdhci-pci.c | 3 +-
drivers/mmc/host/sdhci.c | 46 +++++++++++++++++++++++++++++++++++------
include/linux/mmc/sdhci.h | 3 ++
3 files changed, 44 insertions(+), 8 deletions(-)
--
1.7.4.15.g7811d
This is at least known to be required for the ENE 714.
Cc: Chris Ball <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Naveen Singh <[email protected]>
Cc: Vipin Mehta <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/mmc/host/sdhci.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9e15f41..c95dfc2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -891,8 +891,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
WARN_ON(host->cmd);
- /* Wait max 10 ms */
- timeout = 10;
+ /* Wait max this amount of ms */
+ timeout = (10*256) + 255;
mask = SDHCI_CMD_INHIBIT;
if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))
@@ -913,7 +913,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
return;
}
timeout--;
- mdelay(1);
+ if (!(timeout & 0xFF))
+ mdelay(1);
}
mod_timer(&host->timer, jiffies + 10 * HZ);
--
1.7.4.15.g7811d
Instead of just assuming an interrupt is pending and
processing it, ensure we have interrupts enabled first
prior to calling our IRQ thread processor.
Cc: Chris Ball <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Naveen Singh <[email protected]>
Cc: Vipin Mehta <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/mmc/host/sdhci.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c95dfc2..7074870 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1616,8 +1616,11 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
intmask &= ~SDHCI_INT_BUS_POWER;
- if (intmask & SDHCI_INT_CARD_INT)
- cardint = 1;
+ if (intmask & SDHCI_INT_CARD_INT) {
+ if (readl(host->ioaddr + SDHCI_INT_ENABLE) & SDHCI_INT_CARD_INT)
+ cardint = 1;
+ }
+
intmask &= ~SDHCI_INT_CARD_INT;
--
1.7.4.15.g7811d
This can later be used when we send commands.
Cc: Chris Ball <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Naveen Singh <[email protected]>
Cc: Vipin Mehta <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/mmc/host/sdhci.c | 7 +++++--
include/linux/mmc/sdhci.h | 1 +
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7074870..a4cf0c0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1206,10 +1206,13 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
} else {
if (host->version >= SDHCI_SPEC_300)
ctrl &= ~SDHCI_CTRL_8BITBUS;
- if (ios->bus_width == MMC_BUS_WIDTH_4)
+ if (ios->bus_width == MMC_BUS_WIDTH_4) {
ctrl |= SDHCI_CTRL_4BITBUS;
- else
+ host->flags |= SDHCI_IN_4BIT_MODE;
+ } else {
ctrl &= ~SDHCI_CTRL_4BITBUS;
+ host->flags &= ~SDHCI_IN_4BIT_MODE;
+ }
}
sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
}
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 83bd9f7..266f796 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -109,6 +109,7 @@ struct sdhci_host {
#define SDHCI_USE_ADMA (1<<1) /* Host is ADMA capable */
#define SDHCI_REQ_USE_DMA (1<<2) /* Use DMA for this req. */
#define SDHCI_DEVICE_DEAD (1<<3) /* Device unresponsive */
+#define SDHCI_IN_4BIT_MODE (1<<4) /* bus is in 4-bit mode */
unsigned int version; /* SDHCI spec. version */
--
1.7.4.15.g7811d
This adds an SDHCI quick for controller which cannot
cannot detect CIRQ reliably when in 4-bit mode. What
we do is while bus is idle we leave it in 1-bit mode at
the controller level.
This allows the PCI SDCHI ENE 712 controller to work
correctly.
Cc: Chris Ball <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Naveen Singh <[email protected]>
Cc: Vipin Mehta <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/mmc/host/sdhci-pci.c | 3 ++-
drivers/mmc/host/sdhci.c | 25 +++++++++++++++++++++++++
include/linux/mmc/sdhci.h | 2 ++
3 files changed, 29 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 2f8d468..8b13f61 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -130,7 +130,8 @@ static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
static const struct sdhci_pci_fixes sdhci_ene_712 = {
.quirks = SDHCI_QUIRK_SINGLE_POWER_WRITE |
- SDHCI_QUIRK_BROKEN_DMA,
+ SDHCI_QUIRK_BROKEN_DMA |
+ SDHCI_QUIRK_1BIT_INTERRUPT,
};
static const struct sdhci_pci_fixes sdhci_ene_714 = {
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a4cf0c0..fac075e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -245,6 +245,27 @@ static void sdhci_led_control(struct led_classdev *led,
}
#endif
+/* handle bus case where controller cannot detect CIRQ reliably when in 4-bit mode */
+static void sdhci_idle_bus_adjust(struct sdhci_host *host, u8 idle)
+{
+ u8 ctrl;
+
+ if (!(host->flags & SDHCI_IN_4BIT_MODE))
+ return;
+
+ if (!(host->quirks & SDHCI_QUIRK_1BIT_INTERRUPT))
+ return;
+
+ /* while bus is idle, leave it in 1-bit mode at the controller level */
+ ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL);
+ ctrl &= ~SDHCI_CTRL_4BITBUS;
+
+ if (!idle)
+ ctrl |= SDHCI_CTRL_4BITBUS;
+
+ writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL);
+}
+
/*****************************************************************************\
* *
* Core functions *
@@ -919,6 +940,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
mod_timer(&host->timer, jiffies + 10 * HZ);
+ sdhci_idle_bus_adjust(host, 0);
+
host->cmd = cmd;
sdhci_prepare_data(host, cmd->data);
@@ -1374,6 +1397,8 @@ static void sdhci_tasklet_finish(unsigned long param)
host->cmd = NULL;
host->data = NULL;
+ sdhci_idle_bus_adjust(host, 1);
+
#ifndef SDHCI_USE_LEDS_CLASS
sdhci_deactivate_led(host);
#endif
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 266f796..4f1b26263 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -85,6 +85,8 @@ struct sdhci_host {
#define SDHCI_QUIRK_NO_HISPD_BIT (1<<29)
/* Controller treats ADMA descriptors with length 0000h incorrectly */
#define SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC (1<<30)
+/* Controller cannot detect CIRQ reliably when in 4-bit mode */
+#define SDHCI_QUIRK_1BIT_INTERRUPT (1<<31)
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
--
1.7.4.15.g7811d
On Fri, Mar 18, 2011 at 3:11 PM, Luis R. Rodriguez
<[email protected]> wrote:
> I was pulled in to help with ath6kl development work. As part
> of this I was given some PCI "ENE" SDHCI card which can I can slap
> on to my desktop. I was then given this dump of patches to apply
> to sdhci code to get it to work:
http://www.kernel.org/pub/linux/kernel/people/mcgrof/patches/ath6kl/2011/03/18/sdhci-orig.patch
> I prefer code upstream though so here is my split and cleanup
> of these patches. I've tested it with my ENE card. Since I am
> new to SDHCI I would prefer someone with more experience review
> these chagnes and the respective commit log messages. I've
> tried to make sense of them as best as I can. Unfortunately
> I was not given any explanation to these changes except the
> patch I got so I tried to make the best of it.
Luis
Hi Luis,
On Fri, Mar 18 2011, Luis R. Rodriguez wrote:
> This is at least known to be required for the ENE 714.
>
> Cc: Chris Ball <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: Naveen Singh <[email protected]>
> Cc: Vipin Mehta <[email protected]>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
I think this wins cjb's "I've never been so confused about what a patch
author thought they were doing before" award.
> ---
> drivers/mmc/host/sdhci.c | 7 ++++---
> 1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 9e15f41..c95dfc2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -891,8 +891,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>
> WARN_ON(host->cmd);
>
> - /* Wait max 10 ms */
> - timeout = 10;
> + /* Wait max this amount of ms */
> + timeout = (10*256) + 255;
>
> mask = SDHCI_CMD_INHIBIT;
> if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))
Okay, so our original plan is to go through the while loop ten times,
which equals ten mdelay(1)s == 10ms, waiting for the inhibit bit to
become unset.
After this hunk of your patch, we're set to go through the loop 2815
times, which would make for 2.8 seconds. That seems excessive.
> @@ -913,7 +913,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
> return;
> }
> timeout--;
> - mdelay(1);
> + if (!(timeout & 0xFF))
> + mdelay(1);
> }
>
> mod_timer(&host->timer, jiffies + 10 * HZ);
But wait, here you decide *not* to call mdelay(1) every time through the
loop, and instead call it only on iterations where the bottom eight bits
are unset. This disqualifies most of the 2815 values that timeout will
be set to, and leaves the following values triggering the mdelay(1):
0x100 0x200 0x300 0x400 0x500 0x600 0x700 0x800 0x900 0xa00
256 512 768 1024 1280 1536 1792 2048 2304 2560
The astute observer will notice that there are ten such values.
So you're calling mdelay(1) ten times. But that's what we were
doing before! The only difference is that now we spin through
the while loop 2815 times instead of 10, and don't perform any
explicit delay on 2805 of them. Or am I missing something?
I think you should try:
(a) Reverting the patch and checking that it's actually needed
(b) Leaving the while loop body alone, but increasing the max
timeout until you bisect to the amount of ms that your controller
actually takes to release the inhibit bit.
(c) Yelling loudly in the direction that this code came from. :)
- Chris.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
On Fri, Mar 18, 2011 at 4:10 PM, Chris Ball <[email protected]> wrote:
> Hi Luis,
>
> On Fri, Mar 18 2011, Luis R. Rodriguez wrote:
>> This is at least known to be required for the ENE 714.
>>
>> Cc: Chris Ball <[email protected]>
>> Cc: Kalle Valo <[email protected]>
>> Cc: Naveen Singh <[email protected]>
>> Cc: Vipin Mehta <[email protected]>
>> Signed-off-by: Luis R. Rodriguez <[email protected]>
>
> I think this wins cjb's "I've never been so confused about what a patch
> author thought they were doing before" award.
I warned you :)
>> ---
>> drivers/mmc/host/sdhci.c | 7 ++++---
>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 9e15f41..c95dfc2 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -891,8 +891,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>
>> WARN_ON(host->cmd);
>>
>> - /* Wait max 10 ms */
>> - timeout = 10;
>> + /* Wait max this amount of ms */
>> + timeout = (10*256) + 255;
>>
>> mask = SDHCI_CMD_INHIBIT;
>> if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))
>
> Okay, so our original plan is to go through the while loop ten times,
> which equals ten mdelay(1)s == 10ms, waiting for the inhibit bit to
> become unset.
>
> After this hunk of your patch, we're set to go through the loop 2815
> times, which would make for 2.8 seconds. That seems excessive.
>
>> @@ -913,7 +913,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>> return;
>> }
>> timeout--;
>> - mdelay(1);
>> + if (!(timeout & 0xFF))
>> + mdelay(1);
>> }
>>
>> mod_timer(&host->timer, jiffies + 10 * HZ);
>
> But wait, here you decide *not* to call mdelay(1) every time through the
> loop, and instead call it only on iterations where the bottom eight bits
> are unset. This disqualifies most of the 2815 values that timeout will
> be set to, and leaves the following values triggering the mdelay(1):
>
> 0x100 0x200 0x300 0x400 0x500 0x600 0x700 0x800 0x900 0xa00
> 256 512 768 1024 1280 1536 1792 2048 2304 2560
>
> The astute observer will notice that there are ten such values.
> So you're calling mdelay(1) ten times. But that's what we were
> doing before! The only difference is that now we spin through
> the while loop 2815 times instead of 10, and don't perform any
> explicit delay on 2805 of them. Or am I missing something?
>
> I think you should try:
>
> (a) Reverting the patch and checking that it's actually needed
> (b) Leaving the while loop body alone, but increasing the max
> timeout until you bisect to the amount of ms that your controller
> actually takes to release the inhibit bit.
> (c) Yelling loudly in the direction that this code came from. :)
Heh thanks for the review, once I even get ath6kl to even load
properly on x86_64 I'll try reverting this POS hunk and see if it
still works without it. Who knows where the hell this patch came from,
as I noted I was given the entire blob as a huge patch and if you look
at it, the original patch even removes some quirk for tegra.
Luis
> I prefer code upstream though so here is my split and cleanup
> of these patches. I've tested it with my ENE card. Since I am
> new to SDHCI I would prefer someone with more experience review
> these chagnes and the respective commit log messages. I've
> tried to make sense of them as best as I can. Unfortunately
> I was not given any explanation to these changes except the
> patch I got so I tried to make the best of it.
You did make the best out of it; still, you probably won't be surprised to hear
that all the patches are, well, not acceptable ;) I agree with Chris about 1/4,
I think 2-4 are for a broken card detection and there is already a quirk for
that. So you could also try dropping 2-4 and use
sdhci.debug_quirks = 0x8028
That should enable the standard quirks for this card and BROKEN_CARD. Can you
alsso please post the PCI-ID of this card? And does it make a difference if you
use the SDIO-WLAN card or a standard SD memory card?
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Sat, Mar 19, 2011 at 02:15:44AM -0700, Wolfram Sang wrote:
>
> > I prefer code upstream though so here is my split and cleanup
> > of these patches. I've tested it with my ENE card. Since I am
> > new to SDHCI I would prefer someone with more experience review
> > these chagnes and the respective commit log messages. I've
> > tried to make sense of them as best as I can. Unfortunately
> > I was not given any explanation to these changes except the
> > patch I got so I tried to make the best of it.
>
> You did make the best out of it; still, you probably won't be surprised to hear
> that all the patches are, well, not acceptable ;) I agree with Chris about 1/4,
> I think 2-4 are for a broken card detection and there is already a quirk for
> that. So you could also try dropping 2-4 and use
>
> sdhci.debug_quirks = 0x8028
>
> That should enable the standard quirks for this card and BROKEN_CARD.
I just tried this, it does not work, in fact my own my patches don't work too,
only the original crap does though for some odd magical reason.
Then again, what I tried was:
insmod ./sdhci.ko debug_quirks=0x8028
Was that what you wanted me to try?
> Can you also please post the PCI-ID of this card?
nsingh@nsingh-desktop ~/wireless-testing (git::master)$ lspci -k -n | grep -3 sdhci
Kernel modules: sky2
03:0a.0 0501: 1524:0730
03:0a.1 0805: 1524:0750
Kernel driver in use: sdhci-pci
Kernel modules: sdhci-pci
03:0a.2 0501: 1524:0720
nsingh@nsingh-desktop ~/wireless-testing (git::master)$ lspci -k | grep -3 sdhci
Kernel modules: sky2
03:0a.0 FLASH memory: ENE Technology Inc ENE PCI Memory Stick Card Reader Controller
03:0a.1 SD Host controller: ENE Technology Inc ENE PCI SmartMedia / xD Card Reader Controller
Kernel driver in use: sdhci-pci
Kernel modules: sdhci-pci
03:0a.2 FLASH memory: ENE Technology Inc Memory Stick Card Reader Controller
> And does it make a difference if you use the SDIO-WLAN card or a standard SD
> memory card?
Um, I don't have physical access to the box, Naveen or Vipin would have to
test this.
Luis
Hi Luis,
> I just tried this, it does not work, in fact my own my patches don't work too,
> only the original crap does though for some odd magical reason.
>
> Then again, what I tried was:
>
> insmod ./sdhci.ko debug_quirks=0x8028
>
> Was that what you wanted me to try?
Yes. I assume this would have helped also (assuming that the original
patchset was working), but looks like more investigation is needed.
> 03:0a.1 0805: 1524:0750
Okay, so you have PCI_DEVICE_ID_ENE_CB714_SD and not
PCI_DEVICE_ID_ENE_CB714_SD_2 (just in case they need to be dealt
differently).
> > And does it make a difference if you use the SDIO-WLAN card or a standard SD
> > memory card?
>
> Um, I don't have physical access to the box, Naveen or Vipin would have to
> test this.
Might be worth.
Thanks,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |