David,
I have done some fixes and enhancements to the MMC SPI host controller driver.
Practical results with SD/SDHC cards from different vendors are much better now.
The patch is for kernel 2.6.28.
(I do not read the linux kernel mailing list)
Signed-off-by: Wolfgang Muees <[email protected]>
============= snip ====================
--- 2.6.28/drivers/mmc/host/mmc_spi.c 2008-11-24 10:26:06.000000000 +0100
+++ new/drivers/mmc/host/mmc_spi.c 2009-01-06 14:52:59.000000000 +0100
@@ -25,6 +25,7 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
#include <linux/hrtimer.h>
+#include <linux/sched.h>
#include <linux/delay.h>
#include <linux/bio.h>
#include <linux/dma-mapping.h>
@@ -186,10 +187,10 @@
static int
mmc_spi_skip(struct mmc_spi_host *host, ktime_t timeout, unsigned n, u8 byte)
{
- u8 *cp = host->data->status;
-
- timeout = ktime_add(timeout, ktime_get());
-
+ u8 *cp = host->data->status;
+ unsigned long timeout_jiffies = (unsigned long) ((ktime_to_us(timeout) * HZ) / 1000000);
+ unsigned long starttime = jiffies;
+
while (1) {
int status;
unsigned i;
@@ -203,11 +204,13 @@
return cp[i];
}
- /* REVISIT investigate msleep() to avoid busy-wait I/O
- * in at least some cases.
- */
- if (ktime_to_ns(ktime_sub(ktime_get(), timeout)) > 0)
+ if ((jiffies - starttime) > timeout_jiffies)
break;
+ /* If we need long timeouts, we may release the CPU. */
+ /* We use jiffies here because we want to have a relation between
+ elapsed time and the blocking of the scheduler. */
+ if ((jiffies - starttime) > 1)
+ schedule();
}
return -ETIMEDOUT;
}
@@ -280,7 +283,9 @@
* It'd probably be better to memcpy() the first chunk and
* avoid extra i/o calls...
*/
- for (i = 2; i < 9; i++) {
+ /* Note we check for more than 8 bytes, because in practice,
+ some SD cards are slow... */
+ for (i = 2; i < 16; i++) {
value = mmc_spi_readbytes(host, 1);
if (value < 0)
goto done;
@@ -609,6 +614,15 @@
struct spi_device *spi = host->spi;
int status, i;
struct scratch *scratch = host->data;
+ u32 pattern;
+
+ /* The MMC framework does a good job of computing timeouts
+ according to the mmc/sd standard. However, we found that in
+ SPI mode, there are many cards which need a longer timeout
+ of 1s after receiving a long stream of write data. */
+ struct timeval tv = ktime_to_timeval(timeout);
+ if (tv.tv_sec == 0)
+ timeout = ktime_set(1,0);
if (host->mmc->use_spi_crc)
scratch->crc_val = cpu_to_be16(
@@ -636,8 +650,23 @@
* doesn't necessarily tell whether the write operation succeeded;
* it just says if the transmission was ok and whether *earlier*
* writes succeeded; see the standard.
- */
- switch (SPI_MMC_RESPONSE_CODE(scratch->status[0])) {
+ *
+ * In practice, there are (even modern SDHC-)Cards which need
+ * some bits to send the response, so we have to cope with this
+ * situation and check the response bit-by-bit. Arggh!!!
+ */
+ pattern = scratch->status[0] << 24;
+ pattern |= scratch->status[1] << 16;
+ pattern |= scratch->status[2] << 8;
+ pattern |= scratch->status[3];
+
+ /* left-adjust to leading 0 bit */
+ while (pattern & 0x80000000)
+ pattern <<= 1;
+ /* right-adjust for pattern matching. Code is in bit 4..0 now. */
+ pattern >>= 27;
+
+ switch (pattern) {
case SPI_RESPONSE_ACCEPTED:
status = 0;
break;
@@ -668,9 +697,9 @@
/* Return when not busy. If we didn't collect that status yet,
* we'll need some more I/O.
*/
- for (i = 1; i < sizeof(scratch->status); i++) {
- if (scratch->status[i] != 0)
- return 0;
+ for (i = 4; i < sizeof(scratch->status); i++) {
+ if (scratch->status[i] & 0x01)
+ return 0;
}
return mmc_spi_wait_unbusy(host, timeout);
}
@@ -743,7 +772,11 @@
return -EIO;
}
- if (host->mmc->use_spi_crc) {
+ /* Omitt the CRC check for CID and CSD reads. There are some SDHC
+ cards which don't supply a valid CRC after CID reads.
+ Note that the CID has it's own CRC7 value inside the data block.
+ */
+ if (host->mmc->use_spi_crc && (t->len == MMC_SPI_BLOCKSIZE)) {
u16 crc = crc_itu_t(0, t->rx_buf, t->len);
be16_to_cpus(&scratch->crc_val);
@@ -1206,8 +1239,15 @@
* rising edge ... meaning SPI modes 0 or 3. So either SPI mode
* should be legit. We'll use mode 0 since it seems to be a
* bit less troublesome on some hardware ... unclear why.
- */
- spi->mode = SPI_MODE_0;
+ *
+ * If the platform_data specifies mode 3, trust the platform_data
+ * and use this one. This allows for platforms which do not support
+ * mode 0.
+ */
+ if (spi->mode != SPI_MODE_3)
+ /* set our default */
+ spi->mode = SPI_MODE_0;
+
spi->bits_per_word = 8;
status = spi_setup(spi);
========= snip ============================
best regards
i. A. Wolfgang Mües
--
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94
E-Mail: [email protected]
Web: http://www.auerswald.de
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald
On Tue, Jan 06, 2009 at 03:17:01PM +0100, Wolfgang M??es wrote:
> +
> + /* The MMC framework does a good job of computing timeouts
> + according to the mmc/sd standard. However, we found that in
> + SPI mode, there are many cards which need a longer timeout
> + of 1s after receiving a long stream of write data. */
> + struct timeval tv = ktime_to_timeval(timeout);
> + if (tv.tv_sec == 0)
> + timeout = ktime_set(1,0);
>
Which cards are you having issues with?
Modifying the code to no longer abide by the standard doesn't seem like a
win to me.
Matt,
Am Dienstag, 6. Januar 2009 schrieb Matt Fleming:
> Which cards are you having issues with?
This was a noname 4 GByte SDHC card. If you write a continious stream of data
at max. speed, the card generates a BUSY signal every 10 seconds for nearly a
whole second.
> Modifying the code to no longer abide by the standard doesn't seem like a
> win to me.
I strongly disagree with you.
While each and every SD card should conform to the standard, a driver for a
host controller should be as forgiving as possible. If the driver cannot cope
with cards sold today, the user has a bad experience.
I code drivers for people and not for standards.
Remember, this is only a timeout. Lengthen the timeout will have no
consequences for cards which behave.
best regards
?
i. A. Wolfgang M?es
--
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94
E-Mail: [email protected]
Web: http://www.auerswald.de
?
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grash?fen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Gesch?ftsf?hrungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Gesch?ftsf?hrer: Dipl-Ing. Gerhard Auerswald
On Thu, Jan 08, 2009 at 09:22:08AM +0100, Wolfgang M?es wrote:
> Matt,
>
> Am Dienstag, 6. Januar 2009 schrieb Matt Fleming:
> > Which cards are you having issues with?
>
> This was a noname 4 GByte SDHC card. If you write a continious stream of data
> at max. speed, the card generates a BUSY signal every 10 seconds for nearly a
> whole second.
>
Ouch :-/
> > Modifying the code to no longer abide by the standard doesn't seem like a
> > win to me.
>
> I strongly disagree with you.
>
> While each and every SD card should conform to the standard, a driver for a
> host controller should be as forgiving as possible. If the driver cannot cope
> with cards sold today, the user has a bad experience.
>
> I code drivers for people and not for standards.
>
> Remember, this is only a timeout. Lengthen the timeout will have no
> consequences for cards which behave.
My concern is that completely broken cards may be able to get away with more
if the timeout is extended. There is clearly a tradeoff here between
supporting non-conforming cards and easily detecting broken cards.
Someone suggested to me off list that having a configuration option to
enable/disable the longer timeout is a good compromise, and I agree.
Matt,
Am Donnerstag, 8. Januar 2009 schrieb Matt Fleming:
> My concern is that completely broken cards may be able to get away with
> more if the timeout is extended.
Ah, and you think that the failure of these cards in some embedded devices
will push the vendors of these cards to fix their bugs?
Sorry, I don't think so. If this driver will run in more than 50% of all SD
card devices: yes. But in reality, it will be below 1%...
> There is clearly a tradeoff here between supporting non-conforming cards
> and easily detecting broken cards.
IMHO, not supporting a non-conforming card (if you CAN) is ignorance.
Ignorance to the user, which you leave alone with her problems.
> Someone suggested to me off list that having a configuration option to
> enable/disable the longer timeout is a good compromise, and I agree.
I don't see value in this compromise. The person which is able to turn the
configuration option will be != the user of the device and != the owner of
the card.
best regards
?
i. A. Wolfgang M?es
--
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94
E-Mail: [email protected]
Web: http://www.auerswald.de
?
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grash?fen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Gesch?ftsf?hrungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Gesch?ftsf?hrer: Dipl-Ing. Gerhard Auerswald
Matt,
I think I should explain a little bit more the situation with the current
implementation of the MMC SPI driver...
The SPI mode is not used widely to drive SD cards. Most SD capable devices are
using 1 bit or 4 bit SD mode. SPI mode is only used by some embedded devices
which have no native SD host controller.
So, in practice, most of the available SD cards have bugs in the SPI mode.
These bugs are unlikely to fix by the SD card vendors because SPI mode is not
in focus. In general, Sandisk has the best SPI mode.
The current MMC SPI driver without my patch was able to work with only ONE of
my cards (a new microSD card from Sandisk). All other cards failed. Common
causes of failure are implementation bugs in the SD card: missing response
timing or timeouts.
With my patch applied, all of my available cards (with the exception of one
card which fails because of a bit offset error in reading blocks) are
working. For the bit-offset error, I have found no easy and performant
solution. (You have to bitshift the whole datastream).
So, essential, all of my patch is for common SPI bugs in SD cards - not only
the timeout adjust you comlpained about.
So, my patch changes the status of the MMC SPI driver from "don't work for the
majority of cards" to "work for the majority of cards".
best regards
?
i. A. Wolfgang M?es
--
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94
E-Mail: [email protected]
Web: http://www.auerswald.de
?
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grash?fen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Gesch?ftsf?hrungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Gesch?ftsf?hrer: Dipl-Ing. Gerhard Auerswald
On Tue, 6 Jan 2009 15:17:01 +0100
Wolfgang M__es <[email protected]> wrote:
> David,
>
> I have done some fixes and enhancements to the MMC SPI host controller driver.
> Practical results with SD/SDHC cards from different vendors are much better now.
> The patch is for kernel 2.6.28.
>
> (I do not read the linux kernel mailing list)
Please reissue this patch with a *full* changelog. One which actually
describes these fixes and enhancements. For fixes, fully decribe the
problem which was fixed. For each enhancement, describe why that
enhancement is desirable/useful, if that is not obvious.
Before sending, please pass the patch through scripts/checkpatch.pl.
This one adds various whitespace gremlins which you probably wouldn't
have sent, had you known they were there.
>
> ============= snip ====================
> --- 2.6.28/drivers/mmc/host/mmc_spi.c 2008-11-24 10:26:06.000000000 +0100
> +++ new/drivers/mmc/host/mmc_spi.c 2009-01-06 14:52:59.000000000 +0100
> @@ -25,6 +25,7 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
> #include <linux/hrtimer.h>
> +#include <linux/sched.h>
> #include <linux/delay.h>
> #include <linux/bio.h>
> #include <linux/dma-mapping.h>
> @@ -186,10 +187,10 @@
> static int
> mmc_spi_skip(struct mmc_spi_host *host, ktime_t timeout, unsigned n, u8 byte)
> {
> - u8 *cp = host->data->status;
> -
> - timeout = ktime_add(timeout, ktime_get());
> -
> + u8 *cp = host->data->status;
> + unsigned long timeout_jiffies = (unsigned long) ((ktime_to_us(timeout) * HZ) / 1000000);
ktime_to_us() returns s64. Dividing that by 1000000 introduces the
risk that the compiler will try to generate a 64-bit divide, which will
fail to link on x86_32. Fixable by converting to unsigned long before
dividing, or via do_div().
layout: we don't *have* to do complex 100-column initialisations at the
definition site. it's perfectly OK and often better to do:
unsigned long timeout_jiffies;
...
timeout_jiffies = ...;
Please use USECS_PER_SEC for clarity, if appropriate.
> + unsigned long starttime = jiffies;
> +
> while (1) {
> int status;
> unsigned i;
> @@ -203,11 +204,13 @@
> return cp[i];
> }
>
> - /* REVISIT investigate msleep() to avoid busy-wait I/O
> - * in at least some cases.
> - */
> - if (ktime_to_ns(ktime_sub(ktime_get(), timeout)) > 0)
> + if ((jiffies - starttime) > timeout_jiffies)
time_after/time_before/etc would be preferred.
> break;
> + /* If we need long timeouts, we may release the CPU. */
> + /* We use jiffies here because we want to have a relation between
> + elapsed time and the blocking of the scheduler. */
> + if ((jiffies - starttime) > 1)
time_after()...
> + schedule();
does this actually do anything useful? Presumably the
mmc_spi_readbytes() will block (unless SPI can to 10GB/sec), so I
wouldn't expect this schedule() to help anything.
If it _does_ help then we have bigger problems - if nothing esle is
runnable, we'll burn power like mad and a cpu_relax() might be needed.
> }
> return -ETIMEDOUT;
> }
> @@ -280,7 +283,9 @@
> * It'd probably be better to memcpy() the first chunk and
> * avoid extra i/o calls...
> */
> - for (i = 2; i < 9; i++) {
> + /* Note we check for more than 8 bytes, because in practice,
> + some SD cards are slow... */
> + for (i = 2; i < 16; i++) {
> value = mmc_spi_readbytes(host, 1);
> if (value < 0)
> goto done;
> @@ -609,6 +614,15 @@
> struct spi_device *spi = host->spi;
> int status, i;
> struct scratch *scratch = host->data;
> + u32 pattern;
> +
> + /* The MMC framework does a good job of computing timeouts
> + according to the mmc/sd standard. However, we found that in
> + SPI mode, there are many cards which need a longer timeout
> + of 1s after receiving a long stream of write data. */
> + struct timeval tv = ktime_to_timeval(timeout);
layout: Putting a big hole in the local definitions like this is
inviting someone else to later put some code statments *before* this
definition. Then some other people will get compilation warnings.
This would be better:
u32 pattern;
struct timeval tv;
/* The MMC framework does a good job of computing timeouts
according to the mmc/sd standard. However, we found that in
SPI mode, there are many cards which need a longer timeout
of 1s after receiving a long stream of write data. */
tv = ktime_to_timeval(timeout);
Please format comments in the standard way:
/*
* The MMC framework does a good job of computing timeouts
* according to the mmc/sd standard. However, we found that in
* SPI mode, there are many cards which need a longer timeout
* of 1s after receiving a long stream of write data.
*/
and indent them with tabs, not spacespacespacespacespace.
> + if (tv.tv_sec == 0)
> + timeout = ktime_set(1,0);
>
> if (host->mmc->use_spi_crc)
> scratch->crc_val = cpu_to_be16(
> @@ -636,8 +650,23 @@
> * doesn't necessarily tell whether the write operation succeeded;
> * it just says if the transmission was ok and whether *earlier*
> * writes succeeded; see the standard.
> - */
> - switch (SPI_MMC_RESPONSE_CODE(scratch->status[0])) {
> + *
> + * In practice, there are (even modern SDHC-)Cards which need
> + * some bits to send the response, so we have to cope with this
> + * situation and check the response bit-by-bit. Arggh!!!
> + */
> + pattern = scratch->status[0] << 24;
> + pattern |= scratch->status[1] << 16;
> + pattern |= scratch->status[2] << 8;
> + pattern |= scratch->status[3];
> +
> + /* left-adjust to leading 0 bit */
> + while (pattern & 0x80000000)
> + pattern <<= 1;
> + /* right-adjust for pattern matching. Code is in bit 4..0 now. */
> + pattern >>= 27;
hm, wow, that looks like sad hardware.
> + switch (pattern) {
> case SPI_RESPONSE_ACCEPTED:
> status = 0;
> break;
> @@ -668,9 +697,9 @@
> /* Return when not busy. If we didn't collect that status yet,
> * we'll need some more I/O.
> */
> - for (i = 1; i < sizeof(scratch->status); i++) {
> - if (scratch->status[i] != 0)
> - return 0;
> + for (i = 4; i < sizeof(scratch->status); i++) {
> + if (scratch->status[i] & 0x01)
> + return 0;
> }
> return mmc_spi_wait_unbusy(host, timeout);
> }
> @@ -743,7 +772,11 @@
> return -EIO;
> }
>
> - if (host->mmc->use_spi_crc) {
> + /* Omitt the CRC check for CID and CSD reads. There are some SDHC
> + cards which don't supply a valid CRC after CID reads.
> + Note that the CID has it's own CRC7 value inside the data block.
> + */
spelling: "Omit".
layout.
> + if (host->mmc->use_spi_crc && (t->len == MMC_SPI_BLOCKSIZE)) {
> u16 crc = crc_itu_t(0, t->rx_buf, t->len);
>
> be16_to_cpus(&scratch->crc_val);
> @@ -1206,8 +1239,15 @@
> * rising edge ... meaning SPI modes 0 or 3. So either SPI mode
> * should be legit. We'll use mode 0 since it seems to be a
> * bit less troublesome on some hardware ... unclear why.
> - */
> - spi->mode = SPI_MODE_0;
> + *
> + * If the platform_data specifies mode 3, trust the platform_data
> + * and use this one. This allows for platforms which do not support
> + * mode 0.
> + */
> + if (spi->mode != SPI_MODE_3)
> + /* set our default */
> + spi->mode = SPI_MODE_0;
> +
> spi->bits_per_word = 8;
>
> status = spi_setup(spi);
I just want to throw my .02 SEK in here.
In essence, I agree with both of you. :)
We should do what we can to force vendors to follow standards. But as
Wolfgang says, punishing their users rarely gets the job done.
Especially when the user has no idea that the card is to blame.
My general approach in these conditions is to work around the bugs as
much as possible (as long as those workarounds doesn't make life
difficult for compliant cards), and try to shame the vendor by logging
buggy cards.
So Wolfgang, if you could separate out your workarounds to independent
patches and see what can be done to complain in dmesg about hardware
bugs (without flooding dmesg).
Rgds
--
-- Pierre Ossman
WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.