Currently, the MMC/SD over SPI code has a hard-coded timeout value of
250ms on writes. This is correct for SD cards and is specified in the
spec, but it is not correct for MMC cards. For MMC cards the values
that is read from the CSR should be used.

There is already code to ensure that the write timeout value for SD
cards does not exceed 250ms, this patch only affects the MMC case.

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 7503b81..2818837 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -34,6 +34,7 @@

 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>             /* for R1_SPI_* bit values */
+#include <linux/mmc/card.h>

 #include <linux/spi/spi.h>
 #include <linux/spi/mmc_spi.h>
@@ -96,7 +97,6 @@
  * shorter timeouts ... but why bother?
  */
 #define readblock_timeout      ktime_set(0, 100 * 1000 * 1000)
-#define writeblock_timeout     ktime_set(0, 250 * 1000 * 1000)
 #define r1b_timeout            ktime_set(3, 0)


@@ -225,6 +225,20 @@ static int mmc_spi_readtoken(struct mmc_spi_host *host)
        return mmc_spi_skip(host, readblock_timeout, 1, 0xff);
 }

+/*
+ * Return the write timeout value (in nanoseconds) for this card.
+ */
+static inline unsigned int
+mmc_get_write_timeout(struct mmc_spi_host *host, struct mmc_data *data)
+{
+       unsigned int timeout_ns;
+
+       timeout_ns = data->timeout_ns;
+       timeout_ns += data->timeout_clks * 1000  * 1000 /
+               (host->mmc->card->host->ios.clock);
+
+       return timeout_ns;
+}

 /*
  * Note that for SPI, cmd->resp[0] is not the same data as "native" protocol
@@ -605,11 +619,13 @@ mmc_spi_setup_data_message(
  * Return negative errno, else success.
  */
 static int
-mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t)
+mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t,
+       struct mmc_data *data)
 {
        struct spi_device       *spi = host->spi;
        int                 ...
[ message continues ]

On Mon, 1 Sep 2008 16:12:03 +0100

This actually isn't fully correct for SD either as it only specifies
that 250 ms is the upper bound on the timeout.

I believe the proper way of solving this is to have mmc_spi respect the
timeouts set in the request structure. Modify mmc_set_data_timeout() if
necessary.

Rgds
-- 
     -- Pierre Ossman

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

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

[ message continues ]

You're right, I changed the patch accordingly (I also fixed the read
timeout path).

From 03b2d04b2e597ec073f2ff61224df1d3dee3b9de Mon Sep 17 00:00:00 2001
From: Matthew Fleming <matthew.fleming@imgtec.com>
Date: Mon, 8 Sep 2008 14:10:14 +0100
Subject: [PATCH] MMC: Use timeout values from CSR

Currently, the MMC/SD over SPI code has a hard-coded timeout value of
250ms on writes and 100ms on reads. This is correct for SDHC cards
and is specified in the spec, but it is not correct for MMC/SD cards.

For MMC cards the values that are read from the CSR should be used.
For SD cards the values from the CSR should be used as long as they
do not exceed 250ms for writes and 100ms for reads.

Signed-off-by: Matthew Fleming <matthew.fleming@imgtec.com>
---
 drivers/mmc/host/mmc_spi.c |   56 +++++++++++++++++++++++++++++++++----------
 1 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 7503b81..35f18f5 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -34,6 +34,7 @@

 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>             /* for R1_SPI_* bit values */
+#include <linux/mmc/card.h>

 #include <linux/spi/spi.h>
 #include <linux/spi/mmc_spi.h>
@@ -89,14 +90,12 @@
 #define MMC_SPI_BLOCKSIZE      512


-/* These fixed timeouts come from the latest SD specs, which say to ignore
- * the CSD values.  The R1B value is for card erase (e.g. the "I forgot the
+/*
+ * The R1B value is for card erase (e.g. the "I forgot the
  * card's password" scenario); it's mostly applied to STOP_TRANSMISSION after
  * reads which takes nowhere near that long.  Older cards may be able to use
  * shorter timeouts ... but why bother?
  */
-#define readblock_timeout      ktime_set(0, 100 * 1000 * 1000)
-#define writeblock_timeout     ktime_set(0, 250 * 1000 * 1000)
 #define r1b_timeout            ktime_set(3, 0)


@@ -214,15 +213,44 @@ mmc_spi_skip(struct mmc_spi_host *host, ...
[ message continues ]

OK by me.  I was viewing those values as ceilings, such that
waiting too long wouldn't much matter.  If there are cases
where the CSR values are larger, they clearly should trump.

--

[ message continues ]

On Mon, 8 Sep 2008 14:28:00 +0100

This still doesn't use the fields from the request structure. E.g. SDIO
support is probably still broken here as it mandates a timeout of 1
second for data transfers.

-- 
     -- Pierre Ossman

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

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

[ message continues ]

What fields in the request structure? Are you talking about struct mmc_request ?
--

[ message continues ]

On Tue, 9 Sep 2008 08:34:27 +0100

Yup, timeout_ns and timeout_clks.

-- 
     -- Pierre Ossman

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

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

[ message continues ]

OK, just to be clear, where are those fields not used? They are used
in the new mmc_get_timeout() function.

I'm assuming SDIO should use the same logic as MMC for working out the
timeout value? Or is there an upper limit of 1 second on the timeout
for SDIO? I am not very confident in my understanding of the nuances
of SDIO. Also, I don't have any SDIO hardware so I can't test this
change (which is the reason I didn't change it in the first place).
--

[ message continues ]

On Tue, 9 Sep 2008 08:59:11 +0100

Sorry, you're right. I just noticed the !host->mmc->card part and
assumed you had your own logic in there.

Why do you have that part though? What case have you found where you
need the timeouts and do not have properly set timeout fields?

-- 
     -- Pierre Ossman

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

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

[ message continues ]

This check was put in place because the function was being called
before the card structure was setup properly. I didn't actually work
out the call path but it stopped the kmmcd thread oopsing :)

How does this patch look for the SDIO case?


diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 044d84e..5ebfe35 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -249,8 +249,10 @@ void mmc_set_data_timeout(struct mmc_data *data,
const struct mmc_card *card)
         * SDIO cards only define an upper 1 s limit on access.
         */
        if (mmc_card_sdio(card)) {
-               data->timeout_ns = 1000000000;
-               data->timeout_clks = 0;
+               if (data->timeout_ns > 1000000000) {
+                       data->timeout_ns = 1000000000;
+                       data->timeout_clks = 0;
+               }
                return;
        }
[ message continues ]

The stack trace where this check is needed is here,

mmc_get_timeout()
mmc_spi_readblock()
mmc_spi_request()
mmc_wait_for_req()
mmc_send_cxd_data()
mmc_send_csd()
mmc_attach_sd()

Maybe it would be a good idea to move the line at
drivers/mmc/core/mmc.c:447 up before the call to mmc_send_csd()? Then
it'd probably be possible to remove that !host->mmc->card check from
mmc_get_timeout().
--

[ message continues ]

On Tue, 9 Sep 2008 10:42:53 +0100

No, that shouldn't be needed. If you look at mmc_send_cxd_data(), it
dutifully calls mmc_set_data_timeout(), so your check isn't needed (for
that case at least).

mmc_send_cid() is a bit more problematic. We probably need to
restructure things so that a mmc_card structure is available there as
well.

Rgds
-- 
     -- Pierre Ossman

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

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

[ message continues ]