[PATCH v3 2/3]set timeout control reg for such SDHCI host

[PATCH v3 2/3]set timeout control reg for such SDHCI host

[Chuanxiao Dong]

>From b1cc696f0fbb0c48f024359977624a862ece93f4 Mon Sep 17 00:00:00 2001
From: Chuanxiao Dong <chuanxiao.dong@intel.com>
Date: Thu, 2 Dec 2010 15:53:37 +0800
Subject: [PATCH 2/3] set timeout control reg for SDHCI host when sending erase cmd

Since if erasing needs longer time than the timeout host can wait, host will
generate a timeout interrupt and this will make erasing failed. So before
starting erasing, set the timeout control register value to be the maximum
one which is long enough for SDHCI host to wait.

Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
---
 drivers/mmc/core/core.c  |    4 ++++
 drivers/mmc/host/sdhci.c |    6 ++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 9fe16c2..fbac265 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -170,6 +170,9 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 
 	mrq->cmd->error = 0;
 	mrq->cmd->mrq = mrq;
+	if (mrq->cmd->opcode != MMC_ERASE)
+		mrq->cmd->erase_timeout = 0;
+
 	if (mrq->data) {
 		BUG_ON(mrq->data->blksz > host->max_blk_size);
 		BUG_ON(mrq->data->blocks > host->max_blk_count);
@@ -190,6 +193,7 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 			mrq->data->stop = mrq->stop;
 			mrq->stop->error = 0;
 			mrq->stop->mrq = mrq;
+			mrq->stop->erase_timeout = 0;
 		}
 	}
 	mmc_host_clk_ungate(host);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b4448a9..de9685d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -949,6 +949,12 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	if (cmd->data)
 		flags |= SDHCI_CMD_DATA;
 
+	if (host->quirks & SDHCI_QUIRK_FORCE_ERASE_SINGLE) {
+		/* Set the timeout to be the maximum value */
+		if (cmd->erase_timeout)
+			sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
+	}
+
 	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
 
-- 
1.6.6.1

Re: [PATCH v3 2/3]set timeout control reg for such SDHCI host

[Wolfram Sang]

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

On Thu, Dec 02, 2010 at 07:26:13PM +0800, Chuanxiao Dong wrote:

> +	if (host->quirks & SDHCI_QUIRK_FORCE_ERASE_SINGLE) {
> +		/* Set the timeout to be the maximum value */
> +		if (cmd->erase_timeout)
> +			sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
> +	}
> +
>  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);

Hmm, this looks like another argument for Philip's idea to always use
the maximum timeout value and skip the quirks related to it?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

RE: [PATCH v3 2/3]set timeout control reg for such SDHCI host

[Dong, Chuanxiao]

 > 
> On Thu, Dec 02, 2010 at 07:26:13PM +0800, Chuanxiao Dong wrote:
> 
> > +	if (host->quirks & SDHCI_QUIRK_FORCE_ERASE_SINGLE) {
> > +		/* Set the timeout to be the maximum value */
> > +		if (cmd->erase_timeout)
> > +			sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
> > +	}
> > +
> >  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags),
> > SDHCI_COMMAND);
> 
> Hmm, this looks like another argument for Philip's idea to always use the maximum
> timeout value and skip the quirks related to it?

Yes, if always using the maximum timeout value is OK for other command, the patch2 can be removed I think.
The new added quirk in the serials patches is used to set the limitation of request queue, not only just to set the timeout control reg.
Even the timeout value was set to be 0xE (the maximum value), erasing too many sectors can still be failed since the timeout time was still not longer enough.
So the count of erased sectors passed down by request queue should be reduced by using this quirk.

Re: [PATCH v3 2/3]set timeout control reg for such SDHCI host

[Wolfram Sang]

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

On Fri, Dec 03, 2010 at 10:38:59AM +0800, Dong, Chuanxiao wrote:
>  > 
> > On Thu, Dec 02, 2010 at 07:26:13PM +0800, Chuanxiao Dong wrote:
> > 
> > > +	if (host->quirks & SDHCI_QUIRK_FORCE_ERASE_SINGLE) {
> > > +		/* Set the timeout to be the maximum value */
> > > +		if (cmd->erase_timeout)
> > > +			sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
> > > +	}
> > > +
> > >  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags),
> > > SDHCI_COMMAND);
> > 
> > Hmm, this looks like another argument for Philip's idea to always use the maximum
> > timeout value and skip the quirks related to it?
> 
> Yes, if always using the maximum timeout value is OK for other command, the patch2 can be removed I think.
> The new added quirk in the serials patches is used to set the limitation of request queue, not only just to set the timeout control reg.
> Even the timeout value was set to be 0xE (the maximum value), erasing too many sectors can still be failed since the timeout time was still not longer enough.
> So the count of erased sectors passed down by request queue should be reduced by using this quirk.

Yes, I think I understand the issue. Have you tried adding a callback, so we
don't have the options between MAX_UINT and '1' but rather MAX_UINT and return
value of the callback?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

RE: [PATCH v3 2/3]set timeout control reg for such SDHCI host

[Dong, Chuanxiao]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org
> [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Wolfram Sang
> Sent: Sunday, December 05, 2010 5:34 AM
> To: Dong, Chuanxiao
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; cjb@laptop.org;
> arjan@linux.intel.com; alan@linux.intel.com; akpm@linux-foundation.org;
> adrian.hunter@nokia.com; prakity@marvell.com
> Subject: Re: [PATCH v3 2/3]set timeout control reg for such SDHCI host
> 
> On Fri, Dec 03, 2010 at 10:38:59AM +0800, Dong, Chuanxiao wrote:
> >  >
> > > On Thu, Dec 02, 2010 at 07:26:13PM +0800, Chuanxiao Dong wrote:
> > >
> > > > +	if (host->quirks & SDHCI_QUIRK_FORCE_ERASE_SINGLE) {
> > > > +		/* Set the timeout to be the maximum value */
> > > > +		if (cmd->erase_timeout)
> > > > +			sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
> > > > +	}
> > > > +
> > > >  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags),
> > > > SDHCI_COMMAND);
> > >
> > > Hmm, this looks like another argument for Philip's idea to always
> > > use the maximum timeout value and skip the quirks related to it?
> >
> > Yes, if always using the maximum timeout value is OK for other command, the
> patch2 can be removed I think.
> > The new added quirk in the serials patches is used to set the limitation of request
> queue, not only just to set the timeout control reg.
> > Even the timeout value was set to be 0xE (the maximum value), erasing too many
> sectors can still be failed since the timeout time was still not longer enough.
> > So the count of erased sectors passed down by request queue should be reduced
> by using this quirk.
> 
> Yes, I think I understand the issue. Have you tried adding a callback, so we don't
> have the options between MAX_UINT and '1' but rather MAX_UINT and return
> value of the callback?

I added a function to calculate a more suitable max_discard_sectors value for SDHCI host controller in version 1 patches. This value was calculated by the timeout time host controller can wait and the erase timeout. Is that the callback you mean?
In this version, I added a function called mmc_set_discard_limit to detect whether the host has a new cap called MMC_CAP_ERASE_SINGLE. They can be found in patch1. If host has been set MMC_CAP_ERASE_SINGLE cap, just returns '1'. And if not, return MAX_UINT.
I used this way in version 3 patches instead of the calculated way since I thought using an easier way to work around a hardware issue was better. Any suggestion about solving this kind of problem?

Thanks
Chuanxiao