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

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

[Wolfgang Mües]

From: Wolfgang Muees <wolfgang.mues@auerswald.de>

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

Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>

---
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: Wolfgang.Mues@Auerswald.de
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

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

[Matt Fleming]

On Wed, Mar 11, 2009 at 02:28:39PM +0100, Wolfgang Mües wrote:
> From: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 
> 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.

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

[Wolfgang Mües]

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: Wolfgang.Mues@Auerswald.de
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

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

[Matt Fleming]

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?

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

[Wolfgang Mües]

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: Wolfgang.Mues@Auerswald.de
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

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

[David Brownell]

On Wednesday 11 March 2009, Wolfgang Mües wrote:
> From: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 
> o Some SD cards have very high timeouts in SPI mode.
>   So adjust the timeouts from theory to practice.
> 
> Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>


> ---
> 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: Wolfgang.Mues@Auerswald.de
> 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
> 
> 

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

[David Brownell]

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

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

[Wolfgang Mües]

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: Wolfgang.Mues@Auerswald.de
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

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

[Pierre Ossman]

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

On Wed, 11 Mar 2009 14:28:39 +0100
Wolfgang Mües <wolfgang.mues@auerswald.de> wrote:

> From: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 
> o Some SD cards have very high timeouts in SPI mode.
>   So adjust the timeouts from theory to practice.
> 
> Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 
> ---
> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]