2009-03-11 13:29:05

by Wolfgang Mües

[permalink] [raw]
Subject: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

From: Wolfgang Muees <[email protected]>

o Some SD cards have very high timeouts in SPI mode.
So adjust the timeouts from theory to practice.

Signed-off-by: Wolfgang Muees <[email protected]>

---
This is one of a line of patches to enhance the usability of
the mmc spi host port driver from "don't work with most SD cards"
to "work with nearly all SD cards" (including those ugly cards
with non-byte-aligned responses).

diff -uprN 2_6_29_rc7_patch4_no_crc_on_CID_CSD/drivers/mmc/core/core.c 2_6_29_rc7_patch5_extra_spi_timeouts/drivers/mmc/core/core.c
--- 2_6_29_rc7_patch4_no_crc_on_CID_CSD/drivers/mmc/core/core.c 2009-03-04 02:05:22.000000000 +0100
+++ 2_6_29_rc7_patch5_extra_spi_timeouts/drivers/mmc/core/core.c 2009-03-10 12:06:16.000000000 +0100
@@ -297,6 +297,21 @@ void mmc_set_data_timeout(struct mmc_dat
data->timeout_clks = 0;
}
}
+ /*
+ * Some cards need very high timeouts if driven in SPI mode.
+ * The worst observed timeout was 900ms after writing a
+ * continuous stream of data until the internal logic
+ * overflowed.
+ */
+ if (mmc_host_is_spi(card->host)) {
+ if (data->flags & MMC_DATA_WRITE) {
+ if (data->timeout_ns < 1000000000)
+ data->timeout_ns = 1000000000; /* 1s */
+ } else {
+ if (data->timeout_ns < 100000000)
+ data->timeout_ns = 100000000; /* 100ms */
+ }
+ }
}
EXPORT_SYMBOL(mmc_set_data_timeout);

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


2009-03-11 14:02:35

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

On Wed, Mar 11, 2009 at 02:28:39PM +0100, Wolfgang M?es wrote:
> From: Wolfgang Muees <[email protected]>
>
> o Some SD cards have very high timeouts in SPI mode.
> So adjust the timeouts from theory to practice.
>

[...]

> + /*
> + * Some cards need very high timeouts if driven in SPI mode.
> + * The worst observed timeout was 900ms after writing a
> + * continuous stream of data until the internal logic
> + * overflowed.
> + */
> + if (mmc_host_is_spi(card->host)) {
> + if (data->flags & MMC_DATA_WRITE) {
> + if (data->timeout_ns < 1000000000)
> + data->timeout_ns = 1000000000; /* 1s */

I am correct in thinking that this patch, in conjuction with your other
patch, "[PATCH 6/7] mmc_spi: convert timeout handling to jiffies and avoid
busy waiting", will now penalize my working card and mandate a timeout
of 1 second?

Without your patch 6 at least mmc_spi_skip() would busy-wait for the
response, and if my card completed in less than 1 second then it'd just
return quicker.

It seems you've introduced a performance hit on all MMC over SPI cards.

2009-03-11 14:55:35

by Wolfgang Mües

[permalink] [raw]
Subject: Re: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

Matt,

Am Mittwoch, 11. M?rz 2009 schrieb Matt Fleming:
> I am correct in thinking that this patch, in conjuction with your other
> patch, "[PATCH 6/7] mmc_spi: convert timeout handling to jiffies and avoid
> busy waiting", will now penalize my working card and mandate a timeout
> of 1 second?
>
> Without your patch 6 at least mmc_spi_skip() would busy-wait for the
> response, and if my card completed in less than 1 second then it'd just
> return quicker.
>
> It seems you've introduced a performance hit on all MMC over SPI cards.

No.

A timeout is a maximum time to wait, not a minimum time.
Waiting is terminated by a response or by the timeout, whichever comes sooner.

My patch 6 in mmc_spi_skip() is doing a busy-wait for a short while ( less
than 1 jiffie), and starts to call schedule() inside the loop if the card is
slower.

My goal was to avoid the long-lasting busy waiting. I have measured times up
to 900ms! With my patch, the longest busy waiting will be 1 jiffie.

And yes, if the SD card is sending its response after 5 jiffies, it is
recognized only after the scheduler schedules this process, which will incure
a delay to the data transfer. The amount of delay is determined by the number
of running processes and the number of HZ.

The typical case for this is a series of data block writes to the SD card.
Until the internal card buffers are full, such writes will be acknowledged by
a fast response. But if the card buffers are full, you will get one long
delay until the card controller has written most of the pending data to
flash.

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

2009-03-11 15:46:20

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

On Wed, Mar 11, 2009 at 03:55:00PM +0100, Wolfgang M?es wrote:
>
> My patch 6 in mmc_spi_skip() is doing a busy-wait for a short while ( less
> than 1 jiffie), and starts to call schedule() inside the loop if the card is
> slower.
>

OK, but if my machine runs at 100 HZ then a jiffie is 10ms. Previously
(without your patch) we waited for 300ms in the write case and 100ms in
the read case. So, it's not unreasonable to think that a response is
going to take more than 10ms. With your patch we're almost always going
to schedule() with no indication of exactly when we're going to come
back.

> My goal was to avoid the long-lasting busy waiting. I have measured times up
> to 900ms! With my patch, the longest busy waiting will be 1 jiffie.
>

I agree that busy-waiting for 900ms would be a bit mad. Is there a
reason that you didn't implement this with msleep() as was noted in the
comment above the timeout?


/* REVISIT investigate msleep() to avoid busy-wait I/O
* in at least some cases.
*/


> And yes, if the SD card is sending its response after 5 jiffies, it is
> recognized only after the scheduler schedules this process, which will incure
> a delay to the data transfer. The amount of delay is determined by the number
> of running processes and the number of HZ.
>

Have you benchmarked this case? Do you know approximately how long it
is before we return from the schedule() under various workloads?

2009-03-11 16:14:48

by Wolfgang Mües

[permalink] [raw]
Subject: Re: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

Matt,

Am Mittwoch, 11. M?rz 2009 schrieb Matt Fleming:
> OK, but if my machine runs at 100 HZ then a jiffie is 10ms.

Yes.

> Previously (without your patch) we waited for 300ms in the write case
> and 100ms in the read case.

It depends... There was a change between somewhere between 2.6.26 (300+100ms)
and 2.6.29 (timeouts calculated from the card parameters).

> So, it's not unreasonable to think that a response is
> going to take more than 10ms.

Mosts responses (data read and data write with empty buffers in the card) are
immediately, mostly below 1 ms.

> With your patch we're almost always going
> to schedule()

No. See above.

> I agree that busy-waiting for 900ms would be a bit mad.

Yes, but this was the previous reality!

> Is there a reason that you didn't implement this with msleep()
> as was noted in the comment above the timeout?

Yes. msleep() is a busy waiting. It is implemented in terms of usleep(), which
is also busy waiting. The old comment is wrong.

> Do you know approximately how long it
> is before we return from the schedule() under various workloads?

The Linux scheduler tend to prefer processes which call schedule() often, so
there is a high chance that this process will return from the schedule() at
the very next tick.

So my timing is:

- busy waiting for 1-2 jiffies
- non-busy-waiting with a delay of 0-1 jiffies afterwards.

I have done some speed measurements for SD card reads and writes, and the
non-busy waiting has not incured a notable speed decrease.

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

2009-03-11 20:17:58

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

On Wednesday 11 March 2009, Wolfgang M?es wrote:
> > Is there a reason that you didn't implement this with msleep()
> > as was noted in the comment above the timeout?
>
> Yes. msleep() is a busy waiting. It is implemented in terms of usleep(),
> which is also busy waiting. The old comment is wrong.

I think you're confused. A *delay() call will busy-wait.
But a *sleep() call like msleep() will schedule.

(These speed concerns apply primarily to patch #6, not
this one ...)

2009-03-11 20:18:21

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

On Wednesday 11 March 2009, Wolfgang M?es wrote:
> From: Wolfgang Muees <[email protected]>
>
> o Some SD cards have very high timeouts in SPI mode.
> So adjust the timeouts from theory to practice.
>
> Signed-off-by: Wolfgang Muees <[email protected]>

Acked-by: David Brownell <[email protected]>


> ---
> This is one of a line of patches to enhance the usability of
> the mmc spi host port driver from "don't work with most SD cards"
> to "work with nearly all SD cards" (including those ugly cards
> with non-byte-aligned responses).
>
> diff -uprN 2_6_29_rc7_patch4_no_crc_on_CID_CSD/drivers/mmc/core/core.c 2_6_29_rc7_patch5_extra_spi_timeouts/drivers/mmc/core/core.c
> --- 2_6_29_rc7_patch4_no_crc_on_CID_CSD/drivers/mmc/core/core.c 2009-03-04 02:05:22.000000000 +0100
> +++ 2_6_29_rc7_patch5_extra_spi_timeouts/drivers/mmc/core/core.c 2009-03-10 12:06:16.000000000 +0100
> @@ -297,6 +297,21 @@ void mmc_set_data_timeout(struct mmc_dat
> data->timeout_clks = 0;
> }
> }
> + /*
> + * Some cards need very high timeouts if driven in SPI mode.
> + * The worst observed timeout was 900ms after writing a
> + * continuous stream of data until the internal logic
> + * overflowed.
> + */
> + if (mmc_host_is_spi(card->host)) {
> + if (data->flags & MMC_DATA_WRITE) {
> + if (data->timeout_ns < 1000000000)
> + data->timeout_ns = 1000000000; /* 1s */
> + } else {
> + if (data->timeout_ns < 100000000)
> + data->timeout_ns = 100000000; /* 100ms */
> + }
> + }
> }
> EXPORT_SYMBOL(mmc_set_data_timeout);
>
> ---
> 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
>
>


2009-03-12 08:18:55

by Wolfgang Mües

[permalink] [raw]
Subject: Re: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

David,

Am Mittwoch, 11. M?rz 2009 schrieb David Brownell:
> I think you're confused. A *delay() call will busy-wait.
> But a *sleep() call like msleep() will schedule.

Yes. You are right. I was confused. Sorry for that.

> (These speed concerns apply primarily to patch #6, not
> this one ...)

But I think the consequences of patch #6 are over-estimated. I have not noted
a slowdown in filesystem I/O. Most responses are coming in while the loop is
in busy-waiting. And for most embedded systems, it _is_ important to don't
allow long periods of busy-waiting.

There are other problems which are more notable: I have found that SDHC cards
(with FAT32, clustersize = 4 KByte) are much SLOWER than the older SD cards
(with FAT16, clustersize = 16-32 KByte). I will do some investigation here.

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

2009-03-15 11:28:21

by Pierre Ossman

[permalink] [raw]
Subject: Re: [PATCH 5/7] mmc_spi: allow higher timeouts for SPI mode

On Wed, 11 Mar 2009 14:28:39 +0100
Wolfgang Mües <[email protected]> wrote:

> From: Wolfgang Muees <[email protected]>
>
> o Some SD cards have very high timeouts in SPI mode.
> So adjust the timeouts from theory to practice.
>
> Signed-off-by: Wolfgang Muees <[email protected]>
>
> ---
> This is one of a line of patches to enhance the usability of
> the mmc spi host port driver from "don't work with most SD cards"
> to "work with nearly all SD cards" (including those ugly cards
> with non-byte-aligned responses).
>
> diff -uprN 2_6_29_rc7_patch4_no_crc_on_CID_CSD/drivers/mmc/core/core.c 2_6_29_rc7_patch5_extra_spi_timeouts/drivers/mmc/core/core.c
> --- 2_6_29_rc7_patch4_no_crc_on_CID_CSD/drivers/mmc/core/core.c 2009-03-04 02:05:22.000000000 +0100
> +++ 2_6_29_rc7_patch5_extra_spi_timeouts/drivers/mmc/core/core.c 2009-03-10 12:06:16.000000000 +0100
> @@ -297,6 +297,21 @@ void mmc_set_data_timeout(struct mmc_dat
> data->timeout_clks = 0;
> }
> }
> + /*
> + * Some cards need very high timeouts if driven in SPI mode.
> + * The worst observed timeout was 900ms after writing a
> + * continuous stream of data until the internal logic
> + * overflowed.
> + */
> + if (mmc_host_is_spi(card->host)) {
> + if (data->flags & MMC_DATA_WRITE) {
> + if (data->timeout_ns < 1000000000)
> + data->timeout_ns = 1000000000; /* 1s */
> + } else {
> + if (data->timeout_ns < 100000000)
> + data->timeout_ns = 100000000; /* 100ms */
> + }
> + }
> }
> EXPORT_SYMBOL(mmc_set_data_timeout);
>

In the future, there are macros called NSEC_PER_SEC and similar that
can be used to increase readability instead of the comments.

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.


Attachments:
signature.asc (198.00 B)