[v4,2/3] mmc: enable TRIM/ERASE caps for SDHCI host

Commit Message

Chuanxiao.Dong Feb. 12, 2011, 6:22 a.m. UTC

None

Comments

Sascha Silbe April 3, 2011, 9:41 a.m. UTC | #1

Excerpts from Chuanxiao Dong's message of Sat Feb 12 07:22:20 +0100 2011:
> SDHCI host controller has TRIM/ERASE capability, enable these caps for erasing
> purpose.
> 
> ERASE command needs R1B response. So for such command with busy signal, before
> sending command, SDHCI host driver will simply set a maximum value for timeout
> control register which is safe to use.
> 
> Signed-off-by: Chuanxiao Dong <[e-mail]>

Tested-By: Sascha Silbe <[e-mail]>

Tested on an OLPC XO-1 using a SanDisk 4GB class 4 card (retail). The
BLKDISCARD ioctl (range 0..card size) returned immediately, no timeout
issue encountered.

Sascha

Andrei Warkentin April 4, 2011, 4:06 a.m. UTC | #2

On Sat, Feb 12, 2011 at 12:22 AM, Chuanxiao Dong
<[e-mail]> wrote:
> SDHCI host controller has TRIM/ERASE capability, enable these caps for erasing
> purpose.
>
> ERASE command needs R1B response. So for such command with busy signal, before
> sending command, SDHCI host driver will simply set a maximum value for timeout
> control register which is safe to use.
>

Awesome. I took a look at the eMMC spec, and the following commands
also have R1B responses:

1) CMD5 (SLEEP_AWAKE)
2) CMD6 (SWITCH)
3) CMD7 (Select/Deselect - for Disconnected->Programming transitions)
4) CMD12 (STOP_TRANSMISSION - for write case)
5) CMD28 (SET_WRITE_PROT)
6) CMD29 (CLR_WRITE_PROT)

...so this should help with timeouts for those commands as well. Thanks!

A
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrei Warkentin April 4, 2011, 4:33 a.m. UTC | #3

On Sun, Apr 3, 2011 at 11:06 PM, Andrei Warkentin <[e-mail]> wrote:
> On Sat, Feb 12, 2011 at 12:22 AM, Chuanxiao Dong
> <[e-mail]> wrote:
>> SDHCI host controller has TRIM/ERASE capability, enable these caps for erasing
>> purpose.
>>
>> ERASE command needs R1B response. So for such command with busy signal, before
>> sending command, SDHCI host driver will simply set a maximum value for timeout
>> control register which is safe to use.
>>
>
> Awesome. I took a look at the eMMC spec, and the following commands
> also have R1B responses:
>
> 1) CMD5 (SLEEP_AWAKE)
> 2) CMD6 (SWITCH)
> 3) CMD7 (Select/Deselect - for Disconnected->Programming transitions)
> 4) CMD12 (STOP_TRANSMISSION - for write case)
> 5) CMD28 (SET_WRITE_PROT)
> 6) CMD29 (CLR_WRITE_PROT)
>
> ...so this should help with timeouts for those commands as well. Thanks!
>
> A
>

On a second thought (and look), I see the following problems with this
patch, which are otherwise going to make the MMC code confusing (and
invalid).

1) Erase timeout value is defined by spec (eMMC or SD) and already
calculated in mmc_set_erase_timeout within core/core.c. You should
honor that value instead of picking the max timeout.
2) The trim/erase commands are not the only commands with variable
timeouts. For example, certain CMD6 operations take different amounts
of time (Partition switch takes ext_csd:PARTITION_SWITCH_TIME) Right
now the command structure contains the field "erase_timeout", which
curiously enough is used nowhere other than getting set in
core/core.c. I propose renaming it to cmd_timeout. If cmd.data is
NULL, then cmd_timeout is used to set timeout for busy-type commands.

A
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 655617c..fe7cbd0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -41,7 +41,6 @@ 
 
 static unsigned int debug_quirks = 0;
 
-static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *);
 static void sdhci_finish_data(struct sdhci_host *);
 
 static void sdhci_send_command(struct sdhci_host *, struct mmc_command *);
@@ -652,16 +651,23 @@  static void sdhci_set_transfer_irqs(struct sdhci_host *host)
 		sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
 }
 
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;
 	u8 ctrl;
+	struct mmc_data *data = cmd->data;
 	int ret;
 
 	WARN_ON(host->data);
 
-	if (data == NULL)
+	if (data == NULL) {
+		/*
+		 * set timeout to be maximum value for command with busy signal.
+		 */
+		if (cmd->flags & MMC_RSP_BUSY)
+			sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
 		return;
+	}
 
 	/* Sanity checks */
 	BUG_ON(data->blksz * data->blocks > 524288);
@@ -921,7 +927,7 @@  static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 
 	host->cmd = cmd;
 
-	sdhci_prepare_data(host, cmd->data);
+	sdhci_prepare_data(host, cmd);
 
 	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
@@ -1939,7 +1945,7 @@  int sdhci_add_host(struct sdhci_host *host)
 		mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
 
 	mmc->f_max = host->max_clk;
-	mmc->caps |= MMC_CAP_SDIO_IRQ;
+	mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE;
 
 	/*
 	 * A controller may support 8-bit width, but the board itself