[RFC][PATCH] MMC: Use write timeout value as read from CSR

[RFC][PATCH] MMC: Use write timeout value as read from CSR

[Matt Fleming]

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

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                     status, i;
        struct scratch          *scratch = host->data;
+       unsigned int            timeout_ns;

        if (host->mmc->use_spi_crc)
                scratch->crc_val = cpu_to_be16(
@@ -673,7 +689,11 @@ mmc_spi_writeblock(struct mmc_spi_host *host,
struct spi_transfer *t)
                if (scratch->status[i] != 0)
                        return 0;
        }
-       return mmc_spi_wait_unbusy(host, writeblock_timeout);
+
+       timeout_ns = mmc_get_write_timeout(host, data);
+
+       return mmc_spi_wait_unbusy(host,
+               ktime_add_ns(ktime_set(0, 0), timeout_ns));
 }

 /*
@@ -832,7 +852,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct
mmc_command *cmd,
                                t->len);

                        if (direction == DMA_TO_DEVICE)
-                               status = mmc_spi_writeblock(host, t);
+                               status = mmc_spi_writeblock(host, t, data);
                        else
                                status = mmc_spi_readblock(host, t);
                        if (status < 0)
@@ -872,6 +892,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct
mmc_command *cmd,
                struct scratch  *scratch = host->data;
                int             tmp;
                const unsigned  statlen = sizeof(scratch->status);
+               unsigned int    timeout_ns;

                dev_dbg(&spi->dev, "    mmc_spi: STOP_TRAN\n");

@@ -917,7 +938,9 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct
mmc_command *cmd,
                        if (scratch->status[tmp] != 0)
                                return;
                }
-               tmp = mmc_spi_wait_unbusy(host, writeblock_timeout);
+               timeout_ns = mmc_get_write_timeout(host, data);
+               tmp = mmc_spi_wait_unbusy(host,
+                       ktime_add_ns(ktime_set(0, 0), timeout_ns));
                if (tmp < 0 && !data->error)
                        data->error = tmp;
        }

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mmc-spi-use-timeout-from-card.patch --]
[-- Type: text/x-diff; name=mmc-spi-use-timeout-from-card.patch, Size: 2955 bytes --]

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			status, i;
 	struct scratch		*scratch = host->data;
+	unsigned int		timeout_ns;
 
 	if (host->mmc->use_spi_crc)
 		scratch->crc_val = cpu_to_be16(
@@ -673,7 +689,11 @@ mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t)
 		if (scratch->status[i] != 0)
 			return 0;
 	}
-	return mmc_spi_wait_unbusy(host, writeblock_timeout);
+
+	timeout_ns = mmc_get_write_timeout(host, data);
+
+	return mmc_spi_wait_unbusy(host,
+		ktime_add_ns(ktime_set(0, 0), timeout_ns));
 }
 
 /*
@@ -832,7 +852,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
 				t->len);
 
 			if (direction == DMA_TO_DEVICE)
-				status = mmc_spi_writeblock(host, t);
+				status = mmc_spi_writeblock(host, t, data);
 			else
 				status = mmc_spi_readblock(host, t);
 			if (status < 0)
@@ -872,6 +892,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
 		struct scratch	*scratch = host->data;
 		int		tmp;
 		const unsigned	statlen = sizeof(scratch->status);
+		unsigned int	timeout_ns;
 
 		dev_dbg(&spi->dev, "    mmc_spi: STOP_TRAN\n");
 
@@ -917,7 +938,9 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
 			if (scratch->status[tmp] != 0)
 				return;
 		}
-		tmp = mmc_spi_wait_unbusy(host, writeblock_timeout);
+		timeout_ns = mmc_get_write_timeout(host, data);
+		tmp = mmc_spi_wait_unbusy(host,
+			ktime_add_ns(ktime_set(0, 0), timeout_ns));
 		if (tmp < 0 && !data->error)
 			data->error = tmp;
 	}

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Pierre Ossman]

On Mon, 1 Sep 2008 16:12:03 +0100
"Matt Fleming" <mattjfleming@googlemail.com> wrote:

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

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.

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Matt Fleming]

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

2008/9/7 Pierre Ossman <drzeus-mmc@drzeus.cx>:
>
> 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.
>

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, ktime_t
timeout, unsigned n, u8 byte)
        return -ETIMEDOUT;
 }

+/*
+ * Return the timeout value for this card.
+ */
+static ktime_t
+mmc_get_timeout(struct mmc_spi_host *host, struct mmc_data *data)
+{
+       unsigned int timeout_ns;
+
+       /* If card hasn't been initialised, use default timeouts. */
+       if (!host->mmc->card) {
+               if (data->flags & MMC_DATA_WRITE)
+                       timeout_ns = 250000000;
+               else
+                       timeout_ns = 100000000;
+       } else {
+               timeout_ns = data->timeout_ns;
+               timeout_ns += data->timeout_clks * 1000  * 1000 /
+                       (host->mmc->card->host->ios.clock);
+       }
+
+       /*
+        * The reason we're adding 0 and a nanosecond value here is because
+        * the timeout_ns value may be greater than one second, which
+        * ktime_set() does not handle properly. The correct logic to handle
+        * this overflow is in ktime_add_ns() however.
+        */
+       return ktime_add_ns(ktime_set(0, 0), timeout_ns);
+}
+
 static inline int
 mmc_spi_wait_unbusy(struct mmc_spi_host *host, ktime_t timeout)
 {
        return mmc_spi_skip(host, timeout, sizeof(host->data->status), 0);
 }

-static int mmc_spi_readtoken(struct mmc_spi_host *host)
+static int mmc_spi_readtoken(struct mmc_spi_host *host, ktime_t timeout)
 {
-       return mmc_spi_skip(host, readblock_timeout, 1, 0xff);
+       return mmc_spi_skip(host, timeout, 1, 0xff);
 }


@@ -605,7 +633,8 @@ 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                     status, i;
@@ -673,7 +702,7 @@ mmc_spi_writeblock(struct mmc_spi_host *host,
struct spi_transfer *t)
                if (scratch->status[i] != 0)
                        return 0;
        }
-       return mmc_spi_wait_unbusy(host, writeblock_timeout);
+       return mmc_spi_wait_unbusy(host, mmc_get_timeout(host, data));
 }

 /*
@@ -693,7 +722,8 @@ mmc_spi_writeblock(struct mmc_spi_host *host,
struct spi_transfer *t)
  * STOP_TRANSMISSION command.
  */
 static int
-mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t)
+mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
+       struct mmc_data *data)
 {
        struct spi_device       *spi = host->spi;
        int                     status;
@@ -707,7 +737,7 @@ mmc_spi_readblock(struct mmc_spi_host *host,
struct spi_transfer *t)
                return status;
        status = scratch->status[0];
        if (status == 0xff || status == 0)
-               status = mmc_spi_readtoken(host);
+               status = mmc_spi_readtoken(host, mmc_get_timeout(host, data));

        if (status == SPI_TOKEN_SINGLE) {
                if (host->dma_dev) {
@@ -832,9 +862,9 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct
mmc_command *cmd,
                                t->len);

                        if (direction == DMA_TO_DEVICE)
-                               status = mmc_spi_writeblock(host, t);
+                               status = mmc_spi_writeblock(host, t, data);
                        else
-                               status = mmc_spi_readblock(host, t);
+                               status = mmc_spi_readblock(host, t, data);
                        if (status < 0)
                                break;

@@ -917,7 +947,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct
mmc_command *cmd,
                        if (scratch->status[tmp] != 0)
                                return;
                }
-               tmp = mmc_spi_wait_unbusy(host, writeblock_timeout);
+               tmp = mmc_spi_wait_unbusy(host, mmc_get_timeout(host, data));
                if (tmp < 0 && !data->error)
                        data->error = tmp;
        }
--
1.5.5.2

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-MMC-Use-timeout-values-from-CSR.patch --]
[-- Type: text/x-diff; name=0001-MMC-Use-timeout-values-from-CSR.patch, Size: 5163 bytes --]

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, ktime_t timeout, unsigned n, u8 byte)
 	return -ETIMEDOUT;
 }
 
+/*
+ * Return the timeout value for this card.
+ */
+static ktime_t
+mmc_get_timeout(struct mmc_spi_host *host, struct mmc_data *data)
+{
+	unsigned int timeout_ns;
+
+	/* If card hasn't been initialised, use default timeouts. */
+	if (!host->mmc->card) {
+		if (data->flags & MMC_DATA_WRITE)
+			timeout_ns = 250000000;
+		else
+			timeout_ns = 100000000;
+	} else {
+		timeout_ns = data->timeout_ns;
+		timeout_ns += data->timeout_clks * 1000  * 1000 /
+			(host->mmc->card->host->ios.clock);
+	}
+
+	/* 
+	 * The reason we're adding 0 and a nanosecond value here is because
+	 * the timeout_ns value may be greater than one second, which 
+	 * ktime_set() does not handle properly. The correct logic to handle
+	 * this overflow is in ktime_add_ns() however.
+	 */
+	return ktime_add_ns(ktime_set(0, 0), timeout_ns);
+}
+
 static inline int
 mmc_spi_wait_unbusy(struct mmc_spi_host *host, ktime_t timeout)
 {
 	return mmc_spi_skip(host, timeout, sizeof(host->data->status), 0);
 }
 
-static int mmc_spi_readtoken(struct mmc_spi_host *host)
+static int mmc_spi_readtoken(struct mmc_spi_host *host, ktime_t timeout)
 {
-	return mmc_spi_skip(host, readblock_timeout, 1, 0xff);
+	return mmc_spi_skip(host, timeout, 1, 0xff);
 }
 
 
@@ -605,7 +633,8 @@ 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			status, i;
@@ -673,7 +702,7 @@ mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t)
 		if (scratch->status[i] != 0)
 			return 0;
 	}
-	return mmc_spi_wait_unbusy(host, writeblock_timeout);
+	return mmc_spi_wait_unbusy(host, mmc_get_timeout(host, data));
 }
 
 /*
@@ -693,7 +722,8 @@ mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t)
  * STOP_TRANSMISSION command.
  */
 static int
-mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t)
+mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
+	struct mmc_data *data)
 {
 	struct spi_device	*spi = host->spi;
 	int			status;
@@ -707,7 +737,7 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t)
 		return status;
 	status = scratch->status[0];
 	if (status == 0xff || status == 0)
-		status = mmc_spi_readtoken(host);
+		status = mmc_spi_readtoken(host, mmc_get_timeout(host, data));
 
 	if (status == SPI_TOKEN_SINGLE) {
 		if (host->dma_dev) {
@@ -832,9 +862,9 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
 				t->len);
 
 			if (direction == DMA_TO_DEVICE)
-				status = mmc_spi_writeblock(host, t);
+				status = mmc_spi_writeblock(host, t, data);
 			else
-				status = mmc_spi_readblock(host, t);
+				status = mmc_spi_readblock(host, t, data);
 			if (status < 0)
 				break;
 
@@ -917,7 +947,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
 			if (scratch->status[tmp] != 0)
 				return;
 		}
-		tmp = mmc_spi_wait_unbusy(host, writeblock_timeout);
+		tmp = mmc_spi_wait_unbusy(host, mmc_get_timeout(host, data));
 		if (tmp < 0 && !data->error)
 			data->error = tmp;
 	}
-- 
1.5.5.2

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[David Brownell]

On Monday 08 September 2008, Matt Fleming wrote:
> 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>

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.

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Pierre Ossman]

On Mon, 8 Sep 2008 14:28:00 +0100
"Matt Fleming" <mattjfleming@googlemail.com> wrote:

> 2008/9/7 Pierre Ossman <drzeus-mmc@drzeus.cx>:
> >
> > 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.
> >
> 
> You're right, I changed the patch accordingly (I also fixed the read
> timeout path).
> 

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.

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Matt Fleming]

2008/9/9 Pierre Ossman <drzeus-mmc@drzeus.cx>:
> On Mon, 8 Sep 2008 14:28:00 +0100
> "Matt Fleming" <mattjfleming@googlemail.com> wrote:
>
>> You're right, I changed the patch accordingly (I also fixed the read
>> timeout path).
>>
>
> 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.
>

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

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Pierre Ossman]

On Tue, 9 Sep 2008 08:34:27 +0100
"Matt Fleming" <mattjfleming@googlemail.com> wrote:

> 2008/9/9 Pierre Ossman <drzeus-mmc@drzeus.cx>:
> > On Mon, 8 Sep 2008 14:28:00 +0100
> > "Matt Fleming" <mattjfleming@googlemail.com> wrote:
> >
> >> You're right, I changed the patch accordingly (I also fixed the read
> >> timeout path).
> >>
> >
> > 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.
> >
> 
> What fields in the request structure? Are you talking about struct mmc_request ?

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.

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Matt Fleming]

2008/9/9 Pierre Ossman <drzeus-mmc@drzeus.cx>:
> On Tue, 9 Sep 2008 08:34:27 +0100
> "Matt Fleming" <mattjfleming@googlemail.com> wrote:
>
>> 2008/9/9 Pierre Ossman <drzeus-mmc@drzeus.cx>:
>> > On Mon, 8 Sep 2008 14:28:00 +0100
>> > "Matt Fleming" <mattjfleming@googlemail.com> wrote:
>> >
>> >> You're right, I changed the patch accordingly (I also fixed the read
>> >> timeout path).
>> >>
>> >
>> > 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.
>> >
>>
>> What fields in the request structure? Are you talking about struct mmc_request ?
>
> Yup, timeout_ns and timeout_clks.
>

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

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Pierre Ossman]

On Tue, 9 Sep 2008 08:59:11 +0100
"Matt Fleming" <mattjfleming@googlemail.com> wrote:

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

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.

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Matt Fleming]

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

2008/9/9 Pierre Ossman <drzeus-mmc@drzeus.cx>:
> On Tue, 9 Sep 2008 08:59:11 +0100
> "Matt Fleming" <mattjfleming@googlemail.com> wrote:
>
>>
>> OK, just to be clear, where are those fields not used? They are used
>> in the new mmc_get_timeout() function.
>>
>
> Sorry, you're right. I just noticed the !host->mmc->card part and
> assumed you had your own logic in there.
>

No worries.

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

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mmc-sdio-use-csr-timeout.patch --]
[-- Type: text/x-diff; name=mmc-sdio-use-csr-timeout.patch, Size: 531 bytes --]

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

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Matt Fleming]

2008/9/9 Matt Fleming <mattjfleming@googlemail.com>:
> 2008/9/9 Pierre Ossman <drzeus-mmc@drzeus.cx>:
>
>> 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?
>>
>
> 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 :)
>

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

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Pierre Ossman]

On Tue, 9 Sep 2008 10:42:53 +0100
"Matt Fleming" <mattjfleming@googlemail.com> wrote:

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

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.

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Matt Fleming]

2008/9/15 Pierre Ossman <drzeus-mmc@drzeus.cx>:
> On Mon, 15 Sep 2008 09:03:41 +0100
> "Matt Fleming" <mattjfleming@googlemail.com> wrote:
>
>> Yeah it does, but the card pointer isn't copied to host->card. That
>> was the point I was trying to make above.
>
> I understood that, but you shouldn't be using host->card to begin with.
> Host drivers shouldn't be concerned with card specifics at all as the
> logic can sometimes be quite complex. All information the host drivers
> need should be in the request structure. If it isn't, then we need to
> fix the request structure, not change the drivers to start poking
> deeper into the MMC stack.
>

OK, I can see your point here. However, this is a completely different
change to my original patch. Would it not make more sense to queue my
original patch and then for me to write some patches to move all the
timeout info into the request structure?

> Looking further at the specs, this needs to be handled int the caller
> (i.e. mmc_send_cid() and mmc_send_csd()). The specs specify the
> timeouts used for those commands and they are actually different from
> the timeouts used elsewhere. IOW this could not be properly handled in
> some generic helper.
>

Again, this is a good idea but a different change to the patch I wrote
originally.

So, what I propose is this. Could you please queue my bug fix (I can
send the two patches again) and then I will begin working on the
generic patches to fix the issues that you've raised in this thread?
How does that sound?

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Pierre Ossman]

On Mon, 15 Sep 2008 10:24:22 +0100
"Matt Fleming" <mattjfleming@googlemail.com> wrote:

> 
> OK, I can see your point here. However, this is a completely different
> change to my original patch. Would it not make more sense to queue my
> original patch and then for me to write some patches to move all the
> timeout info into the request structure?
> 

It's better to get it right directly. I'd consider this a bug fix
patch, so it can be applied at any time during the development cycle.

> 
> So, what I propose is this. Could you please queue my bug fix (I can
> send the two patches again) and then I will begin working on the
> generic patches to fix the issues that you've raised in this thread?
> How does that sound?

It shouldn't take that long to write the more proper solution, so get
to it and it should be possible to get in even for .27.

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

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Matt Fleming]

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

2008/9/20 Pierre Ossman <drzeus-mmc@drzeus.cx>:
>
> It shouldn't take that long to write the more proper solution, so get
> to it and it should be possible to get in even for .27.
>

OK, attached is the latest attempt at this patch. Because a lot of
host drivers manipulate timeout_ns and timeout_clks I decided not
touch them at all. So, I created a new member of the mmc_data struct
that has the timeout value as a ktime_t. I'm still unsure of how
exactly to tackle the mmc_send_cid and mmc_send_csd() cases, the
timeout value used in the spec is Ncr, where can I find this value?


diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 044d84e..b5c6f5f 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;
        }

@@ -269,6 +271,11 @@ void mmc_set_data_timeout(struct mmc_data *data,
const struct mmc_card *card)
        data->timeout_ns = card->csd.tacc_ns * mult;
        data->timeout_clks = card->csd.tacc_clks * mult;

+       data->ktimeout = ktime_set(0, 0);
+       data->ktimeout = ktime_add_ns(data->ktimeout, data->timeout_ns);
+       data->ktimeout = ktime_add_ns(data->ktimeout,
+               data->timeout_clks * 1000000 / card->host->ios.clock);
+
        /*
         * SD cards also have an upper limit on the timeout.
         */
@@ -290,6 +297,8 @@ void mmc_set_data_timeout(struct mmc_data *data,
const struct mmc_card *card)
                if (timeout_us > limit_us || mmc_card_blockaddr(card)) {
                        data->timeout_ns = limit_us * 1000;
                        data->timeout_clks = 0;
+                       data->ktimeout = ktime_add_ns(ktime_set(0, 0),
+                                               data->timeout_ns);
                }
        }
 }
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 7503b81..cdeb2e5 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -95,8 +95,6 @@
  * 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)


@@ -220,9 +218,9 @@ mmc_spi_wait_unbusy(struct mmc_spi_host *host,
ktime_t timeout)
        return mmc_spi_skip(host, timeout, sizeof(host->data->status), 0);
 }

-static int mmc_spi_readtoken(struct mmc_spi_host *host)
+static int mmc_spi_readtoken(struct mmc_spi_host *host, ktime_t timeout)
 {
-       return mmc_spi_skip(host, readblock_timeout, 1, 0xff);
+       return mmc_spi_skip(host, timeout, 1, 0xff);
 }


@@ -605,7 +603,8 @@ 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,
+       ktime_t writeblock_timeout)
 {
        struct spi_device       *spi = host->spi;
        int                     status, i;
@@ -693,7 +692,8 @@ mmc_spi_writeblock(struct mmc_spi_host *host,
struct spi_transfer *t)
  * STOP_TRANSMISSION command.
  */
 static int
-mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t)
+mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
+       ktime_t readblock_timeout)
 {
        struct spi_device       *spi = host->spi;
        int                     status;
@@ -707,7 +707,7 @@ mmc_spi_readblock(struct mmc_spi_host *host,
struct spi_transfer *t)
                return status;
        status = scratch->status[0];
        if (status == 0xff || status == 0)
-               status = mmc_spi_readtoken(host);
+               status = mmc_spi_readtoken(host, readblock_timeout);

        if (status == SPI_TOKEN_SINGLE) {
                if (host->dma_dev) {
@@ -832,9 +832,11 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct
mmc_command *cmd,
                                t->len);

                        if (direction == DMA_TO_DEVICE)
-                               status = mmc_spi_writeblock(host, t);
+                               status = mmc_spi_writeblock(host, t,
+                                               data->ktimeout);
                        else
-                               status = mmc_spi_readblock(host, t);
+                               status = mmc_spi_readblock(host, t,
+                                               data->ktimeout);
                        if (status < 0)
                                break;

@@ -917,7 +919,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct
mmc_command *cmd,
                        if (scratch->status[tmp] != 0)
                                return;
                }
-               tmp = mmc_spi_wait_unbusy(host, writeblock_timeout);
+               tmp = mmc_spi_wait_unbusy(host, data->ktimeout);
                if (tmp < 0 && !data->error)
                        data->error = tmp;
        }
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 143cebf..e518391 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -10,6 +10,7 @@

 #include <linux/interrupt.h>
 #include <linux/device.h>
+#include <linux/ktime.h>

 struct request;
 struct mmc_data;
@@ -99,6 +100,7 @@ struct mmc_command {
 struct mmc_data {
        unsigned int            timeout_ns;     /* data timeout (in
ns, max 80ms) */
        unsigned int            timeout_clks;   /* data timeout (in clocks) */
+       ktime_t                 ktimeout;       /* data timeout */
        unsigned int            blksz;          /* data block size */
        unsigned int            blocks;         /* number of blocks */
        unsigned int            error;          /* data error */

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mmc-for-pierre-use-ktime.patch --]
[-- Type: text/x-diff; name=mmc-for-pierre-use-ktime.patch, Size: 4591 bytes --]

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 044d84e..b5c6f5f 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;
 	}
 
@@ -269,6 +271,11 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 	data->timeout_ns = card->csd.tacc_ns * mult;
 	data->timeout_clks = card->csd.tacc_clks * mult;
 
+	data->ktimeout = ktime_set(0, 0);
+	data->ktimeout = ktime_add_ns(data->ktimeout, data->timeout_ns);
+	data->ktimeout = ktime_add_ns(data->ktimeout,
+		data->timeout_clks * 1000000 / card->host->ios.clock);
+
 	/*
 	 * SD cards also have an upper limit on the timeout.
 	 */
@@ -290,6 +297,8 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 		if (timeout_us > limit_us || mmc_card_blockaddr(card)) {
 			data->timeout_ns = limit_us * 1000;
 			data->timeout_clks = 0;
+			data->ktimeout = ktime_add_ns(ktime_set(0, 0),
+						data->timeout_ns);
 		}
 	}
 }
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 7503b81..cdeb2e5 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -95,8 +95,6 @@
  * 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)
 
 
@@ -220,9 +218,9 @@ mmc_spi_wait_unbusy(struct mmc_spi_host *host, ktime_t timeout)
 	return mmc_spi_skip(host, timeout, sizeof(host->data->status), 0);
 }
 
-static int mmc_spi_readtoken(struct mmc_spi_host *host)
+static int mmc_spi_readtoken(struct mmc_spi_host *host, ktime_t timeout)
 {
-	return mmc_spi_skip(host, readblock_timeout, 1, 0xff);
+	return mmc_spi_skip(host, timeout, 1, 0xff);
 }
 
 
@@ -605,7 +603,8 @@ 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,
+	ktime_t writeblock_timeout)
 {
 	struct spi_device	*spi = host->spi;
 	int			status, i;
@@ -693,7 +692,8 @@ mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t)
  * STOP_TRANSMISSION command.
  */
 static int
-mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t)
+mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
+	ktime_t readblock_timeout)
 {
 	struct spi_device	*spi = host->spi;
 	int			status;
@@ -707,7 +707,7 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t)
 		return status;
 	status = scratch->status[0];
 	if (status == 0xff || status == 0)
-		status = mmc_spi_readtoken(host);
+		status = mmc_spi_readtoken(host, readblock_timeout);
 
 	if (status == SPI_TOKEN_SINGLE) {
 		if (host->dma_dev) {
@@ -832,9 +832,11 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
 				t->len);
 
 			if (direction == DMA_TO_DEVICE)
-				status = mmc_spi_writeblock(host, t);
+				status = mmc_spi_writeblock(host, t,
+						data->ktimeout);
 			else
-				status = mmc_spi_readblock(host, t);
+				status = mmc_spi_readblock(host, t,
+						data->ktimeout);
 			if (status < 0)
 				break;
 
@@ -917,7 +919,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
 			if (scratch->status[tmp] != 0)
 				return;
 		}
-		tmp = mmc_spi_wait_unbusy(host, writeblock_timeout);
+		tmp = mmc_spi_wait_unbusy(host, data->ktimeout);
 		if (tmp < 0 && !data->error)
 			data->error = tmp;
 	}
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 143cebf..e518391 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -10,6 +10,7 @@
 
 #include <linux/interrupt.h>
 #include <linux/device.h>
+#include <linux/ktime.h>
 
 struct request;
 struct mmc_data;
@@ -99,6 +100,7 @@ struct mmc_command {
 struct mmc_data {
 	unsigned int		timeout_ns;	/* data timeout (in ns, max 80ms) */
 	unsigned int		timeout_clks;	/* data timeout (in clocks) */
+	ktime_t			ktimeout;	/* data timeout */
 	unsigned int		blksz;		/* data block size */
 	unsigned int		blocks;		/* number of blocks */
 	unsigned int		error;		/* data error */

Re: [RFC][PATCH] MMC: Use write timeout value as read from CSR

[Pierre Ossman]

On Wed, 24 Sep 2008 12:16:02 +0100
"Matt Fleming" <mattjfleming@googlemail.com> wrote:

> 2008/9/20 Pierre Ossman <drzeus-mmc@drzeus.cx>:
> >
> > It shouldn't take that long to write the more proper solution, so get
> > to it and it should be possible to get in even for .27.
> >
> 
> OK, attached is the latest attempt at this patch. Because a lot of
> host drivers manipulate timeout_ns and timeout_clks I decided not
> touch them at all. So, I created a new member of the mmc_data struct
> that has the timeout value as a ktime_t.

Hmm... What do you mean manipulate? They all just read it, so I don't
see a need for a new member.

> I'm still unsure of how
> exactly to tackle the mmc_send_cid and mmc_send_csd() cases, the
> timeout value used in the spec is Ncr, where can I find this value?

Right, they left that out of the simplified spec. Sneaky bastards. It's
defined as 64 clock cycles.

> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 044d84e..b5c6f5f 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;
>         }
> 

data->timeout_* is undefined when this function is invoked, so this
code is wrong. It is also unnecessary, so just leave it out.

> @@ -269,6 +271,11 @@ void mmc_set_data_timeout(struct mmc_data *data,
> const struct mmc_card *card)
>         data->timeout_ns = card->csd.tacc_ns * mult;
>         data->timeout_clks = card->csd.tacc_clks * mult;
> 
> +       data->ktimeout = ktime_set(0, 0);
> +       data->ktimeout = ktime_add_ns(data->ktimeout, data->timeout_ns);
> +       data->ktimeout = ktime_add_ns(data->ktimeout,
> +               data->timeout_clks * 1000000 / card->host->ios.clock);
> +
>         /*
>          * SD cards also have an upper limit on the timeout.
>          */

card->host->ios.clock is just the upper bound on the clock, it might be
running slower. This is why the host driver needs to calculate it (well
that, and the fact that different controllers treat timeouts in
different ways so it's not one-size-fits-all).

> @@ -832,9 +832,11 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct
> mmc_command *cmd,
>                                 t->len);
> 
>                         if (direction == DMA_TO_DEVICE)
> -                               status = mmc_spi_writeblock(host, t);
> +                               status = mmc_spi_writeblock(host, t,
> +                                               data->ktimeout);
>                         else
> -                               status = mmc_spi_readblock(host, t);
> +                               status = mmc_spi_readblock(host, t,
> +                                               data->ktimeout);
>                         if (status < 0)
>                                 break;
> 

If you put the calculation somewhere before this chunk instead,
everything should be peachy.

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.