Set the timeout control register for SDHCI host when it needs to send some
commands which need busy signal. Use the maximum timeout value will be safe.
Signed-off-by: Chuanxiao Dong <[email protected]>
---
drivers/mmc/host/sdhci.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9e15f41..32b7475 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -40,7 +40,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 *);
@@ -651,16 +650,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);
@@ -920,7 +926,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);
--
1.6.6.1
Hi
This patch looks like this. I sent to the RFC patch..
http://marc.info/?l=linux-mmc&m=129109794815028&w=3
Signed-off-by: Jaehoon Chung <[email protected]>
Signed-off-by: Kyungmin Park <[email protected]>
---
drivers/mmc/host/sdhci.c | 6 +++++-
include/linux/mmc/sdhci.h | 3 ++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 782c0ee..3b93d97 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -655,8 +655,12 @@ static void sdhci_prepare_data(struct sdhci_host
*host, struct mmc_data *data)
WARN_ON(host->data);
- if (data == NULL)
+ if (data == NULL) {
+ if ((host->quirks & SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL) &&
+ (host->cmd->flags & MMC_RSP_BUSY))
+ sdhci_writel(host, 0xE, SDHCI_TIMEOUT_CONTROL);
return;
+ }
/* Sanity checks */
BUG_ON(data->blksz * data->blocks > 524288);
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 1fdc673..315ff49 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -83,7 +83,8 @@ struct sdhci_host {
#define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<28)
/* Controller doesn't have HISPD bit field in HI-SPEED SD card */
#define SDHCI_QUIRK_NO_HISPD_BIT (1<<29)
-
+/* Controller need set data timeout value when card is busy */
+#define SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL (1<<30)
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
--
1.6.0.4
2011/2/24 Chuanxiao Dong <[email protected]>:
> Set the timeout control register for SDHCI host when it needs to send some
> commands which need busy signal. Use the maximum timeout value will be safe.
>
> Signed-off-by: Chuanxiao Dong <[email protected]>
> ---
> ?drivers/mmc/host/sdhci.c | ? 14 ++++++++++----
> ?1 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 9e15f41..32b7475 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -40,7 +40,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 *);
> @@ -651,16 +650,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);
> @@ -920,7 +926,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);
>
> --
> 1.6.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
Great. Maybe we wouldn't need to add this quirk since give a maximum timeout value is safe for all host controller I think. We would better save the quirk for other really critical silicon bugs.
> -----Original Message-----
> From: Jae hoon Chung [mailto:[email protected]]
> Sent: Thursday, February 24, 2011 10:18 PM
> To: Dong, Chuanxiao
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Kyungmin Park
> Subject: Re: [PATCH 1/1]mmc: set timeout for SDHCI host before sending busy cmd
>
> Hi
>
> This patch looks like this. I sent to the RFC patch..
>
> http://marc.info/?l=linux-mmc&m=129109794815028&w=3
>
> Signed-off-by: Jaehoon Chung <[email protected]>
> Signed-off-by: Kyungmin Park <[email protected]>
>
> ---
> drivers/mmc/host/sdhci.c | 6 +++++-
> include/linux/mmc/sdhci.h | 3 ++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 782c0ee..3b93d97 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -655,8 +655,12 @@ static void sdhci_prepare_data(struct sdhci_host
> *host, struct mmc_data *data)
>
> WARN_ON(host->data);
>
> - if (data == NULL)
> + if (data == NULL) {
> + if ((host->quirks & SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL)
> &&
> + (host->cmd->flags & MMC_RSP_BUSY))
> + sdhci_writel(host, 0xE, SDHCI_TIMEOUT_CONTROL);
> return;
> + }
>
> /* Sanity checks */
> BUG_ON(data->blksz * data->blocks > 524288);
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 1fdc673..315ff49 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -83,7 +83,8 @@ struct sdhci_host {
> #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<28)
> /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
> #define SDHCI_QUIRK_NO_HISPD_BIT (1<<29)
> -
> +/* Controller need set data timeout value when card is busy */
> +#define SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL (1<<30)
> int irq; /* Device IRQ */
> void __iomem *ioaddr; /* Mapped address */
>
> --
> 1.6.0.4
>
> 2011/2/24 Chuanxiao Dong <[email protected]>:
> > Set the timeout control register for SDHCI host when it needs to send some
> > commands which need busy signal. Use the maximum timeout value will be safe.
> >
> > Signed-off-by: Chuanxiao Dong <[email protected]>
> > ---
> > ?drivers/mmc/host/sdhci.c | ? 14 ++++++++++----
> > ?1 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 9e15f41..32b7475 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -40,7 +40,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
> *);
> > @@ -651,16 +650,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);
> > @@ -920,7 +926,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);
> >
> > --
> > 1.6.6.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > the body of a message to [email protected]
> > More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> >
On Feb 24, 2011, at 6:54 AM, Dong, Chuanxiao wrote:
> Great. Maybe we wouldn't need to add this quirk since give a maximum timeout value is safe for all host controller I think. We would better save the quirk for other really critical silicon bugs.
proposed this a while ago and strongly support just removing the quirk for broken timeout and setting the timeout value to maximum of 0xE.
This also handles the case of the sd device having a timeout value too low. In my testing I have come across SD cards that do not provide the correct value.
We force our pxa168, pxa910, and mmp2 controllers to have 0xE.
Philip
>
>> -----Original Message-----
>> From: Jae hoon Chung [mailto:[email protected]]
>> Sent: Thursday, February 24, 2011 10:18 PM
>> To: Dong, Chuanxiao
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; Kyungmin Park
>> Subject: Re: [PATCH 1/1]mmc: set timeout for SDHCI host before sending busy cmd
>>
>> Hi
>>
>> This patch looks like this. I sent to the RFC patch..
>>
>> http://marc.info/?l=linux-mmc&m=129109794815028&w=3
>>
>> Signed-off-by: Jaehoon Chung <[email protected]>
>> Signed-off-by: Kyungmin Park <[email protected]>
>>
>> ---
>> drivers/mmc/host/sdhci.c | 6 +++++-
>> include/linux/mmc/sdhci.h | 3 ++-
>> 2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 782c0ee..3b93d97 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -655,8 +655,12 @@ static void sdhci_prepare_data(struct sdhci_host
>> *host, struct mmc_data *data)
>>
>> WARN_ON(host->data);
>>
>> - if (data == NULL)
>> + if (data == NULL) {
>> + if ((host->quirks & SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL)
>> &&
>> + (host->cmd->flags & MMC_RSP_BUSY))
>> + sdhci_writel(host, 0xE, SDHCI_TIMEOUT_CONTROL);
>> return;
>> + }
>>
>> /* Sanity checks */
>> BUG_ON(data->blksz * data->blocks > 524288);
>> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
>> index 1fdc673..315ff49 100644
>> --- a/include/linux/mmc/sdhci.h
>> +++ b/include/linux/mmc/sdhci.h
>> @@ -83,7 +83,8 @@ struct sdhci_host {
>> #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 (1<<28)
>> /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
>> #define SDHCI_QUIRK_NO_HISPD_BIT (1<<29)
>> -
>> +/* Controller need set data timeout value when card is busy */
>> +#define SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL (1<<30)
>> int irq; /* Device IRQ */
>> void __iomem *ioaddr; /* Mapped address */
>>
>> --
>> 1.6.0.4
>>
>> 2011/2/24 Chuanxiao Dong <[email protected]>:
>>> Set the timeout control register for SDHCI host when it needs to send some
>>> commands which need busy signal. Use the maximum timeout value will be safe.
>>>
>>> Signed-off-by: Chuanxiao Dong <[email protected]>
>>> ---
>>> drivers/mmc/host/sdhci.c | 14 ++++++++++----
>>> 1 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 9e15f41..32b7475 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -40,7 +40,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
>> *);
>>> @@ -651,16 +650,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);
>>> @@ -920,7 +926,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);
>>>
>>> --
>>> 1.6.6.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 24, 2011 at 10:32:30AM -0800, Philip Rakity wrote:
> proposed this a while ago and strongly support just removing the quirk for broken timeout and setting the timeout value to maximum of 0xE.
>
> This also handles the case of the sd device having a timeout value too low. In my testing I have come across SD cards that do not provide the correct value.
> We force our pxa168, pxa910, and mmp2 controllers to have 0xE.
Yeah, OLPC's CaFe controller -- which might be the same hardware as yours,
actually -- has the same problem.
Does anyone know of a reason (beyond strict spec-compliance, I suppose)
for honoring the timeout value rather than using 0xE everywhere? If not,
I'm willing to try out Philip's suggestion.
Thanks,
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
On Thu, Feb 24, 2011 at 08:15:04PM +0000, Chris Ball wrote:
> On Thu, Feb 24, 2011 at 10:32:30AM -0800, Philip Rakity wrote:
> > proposed this a while ago and strongly support just removing the quirk for broken timeout and setting the timeout value to maximum of 0xE.
> >
> > This also handles the case of the sd device having a timeout value too low. In my testing I have come across SD cards that do not provide the correct value.
> > We force our pxa168, pxa910, and mmp2 controllers to have 0xE.
>
> Yeah, OLPC's CaFe controller -- which might be the same hardware as yours,
> actually -- has the same problem.
>
> Does anyone know of a reason (beyond strict spec-compliance, I suppose)
> for honoring the timeout value rather than using 0xE everywhere? If not,
> I'm willing to try out Philip's suggestion.
+1. A full cycle in linux-next might an idea to be on the safe side? That would
be 2.6.40-material then. Or too slow?
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi,
On Thu, Feb 24, 2011 at 09:43:52PM +0100, Wolfram Sang wrote:
> +1. A full cycle in linux-next might an idea to be on the safe side? That
> would be 2.6.40-material then. Or too slow?
I think it could be okay to merge for .39, but that's mainly because I
don't think we start getting testing with a lot of cards until patches
appear in an -rc1 -- so for something that requires broad testing, I'd
rather merge it for -rc1 with a plan to revert it if we find anything
unexpected.
But I don't have a strong opinion, so if anyone thinks there's a reason
to wait (for example, an existence-proof of a card that misbehaves when
configured with a max timeout value) then I'm happy to do so.
Thanks,
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
On Feb 24, 2011, at 12:15 PM, Chris Ball wrote:
> On Thu, Feb 24, 2011 at 10:32:30AM -0800, Philip Rakity wrote:
>> proposed this a while ago and strongly support just removing the quirk for broken timeout and setting the timeout value to maximum of 0xE.
>>
>> This also handles the case of the sd device having a timeout value too low. In my testing I have come across SD cards that do not provide the correct value.
>> We force our pxa168, pxa910, and mmp2 controllers to have 0xE.
>
> Yeah, OLPC's CaFe controller -- which might be the same hardware as yours,
> actually -- has the same problem.
different controller not same as on mmp2, pxa168, pxa910
>
> Does anyone know of a reason (beyond strict spec-compliance, I suppose)
> for honoring the timeout value rather than using 0xE everywhere? If not,
> I'm willing to try out Philip's suggestion.
>
> Thanks,
>
> --
> Chris Ball <[email protected]> <http://printf.net/>
> One Laptop Per Child