[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[Jaehoon Chung]

When response type is R1b, mask value is added the SDHCI_INT_DAT_END.
but in while(), didn't check that flag.
So sdhci controller didn't work fine.
CMD6 didn't always complete.

Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mmc/sdhci.c |   33 +++++++++++++++++++++++----------
 1 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index fc904b5..0dd08b9 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -124,10 +124,11 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 {
 	struct sdhci_host *host = (struct sdhci_host *)mmc->priv;
 	unsigned int stat = 0;
-	int ret = 0;
+	int i, ret = 0;
 	int trans_bytes = 0, is_aligned = 1;
 	u32 mask, flags, mode;
 	unsigned int timeout, start_addr = 0;
+	unsigned int retry = 10000;
 
 	/* Wait max 10 ms */
 	timeout = 10;
@@ -206,19 +207,31 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 	flush_cache(start_addr, trans_bytes);
 #endif
 	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->cmdidx, flags), SDHCI_COMMAND);
-	do {
+
+	for (i = 0; i < retry; i++) {
 		stat = sdhci_readl(host, SDHCI_INT_STATUS);
-		if (stat & SDHCI_INT_ERROR)
+		if (stat & (SDHCI_INT_RESPONSE | SDHCI_INT_DATA_END)) {
+			sdhci_cmd_done(host, cmd);
+			sdhci_writel(host, mask, SDHCI_INT_STATUS);
+			if (!data) {
+				sdhci_writel(host, stat,  SDHCI_INT_STATUS);
+			}
 			break;
-	} while ((stat & mask) != mask);
+		}
+	}
 
-	if ((stat & (SDHCI_INT_ERROR | mask)) == mask) {
-		sdhci_cmd_done(host, cmd);
-		sdhci_writel(host, mask, SDHCI_INT_STATUS);
-	} else
-		ret = -1;
+	if (i == retry) {
+		printf("%s: waiting for status update\n",__func__);
+		return TIMEOUT;
+	}
+
+	if (stat & SDHCI_INT_TIMEOUT) {
+		return TIMEOUT;
+	} else if (stat & SDHCI_INT_ERROR) {
+		return -1;
+	}
 
-	if (!ret && data)
+	if (data)
 		ret = sdhci_transfer_data(host, data, start_addr);
 
 	stat = sdhci_readl(host, SDHCI_INT_STATUS);

[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[Lei Wen]

Hi Jaehoon,

On Fri, Mar 30, 2012 at 10:39 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> When response type is R1b, mask value is added the SDHCI_INT_DAT_END.
> but in while(), didn't check that flag.
> So sdhci controller didn't work fine.
> CMD6 didn't always complete.

Could you elaborate it more in details?
        do {
                stat = sdhci_readl(host, SDHCI_INT_STATUS);
                if (stat & SDHCI_INT_ERROR)
                        break;
        } while ((stat & mask) != mask);
Here in the while condition, if the status read out don't contain all mask,
then the looping would continue.
Do you mean you just need a retry max time set here?


>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> ?drivers/mmc/sdhci.c | ? 33 +++++++++++++++++++++++----------
> ?1 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index fc904b5..0dd08b9 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -124,10 +124,11 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
> ?{
> ? ? ? ?struct sdhci_host *host = (struct sdhci_host *)mmc->priv;
> ? ? ? ?unsigned int stat = 0;
> - ? ? ? int ret = 0;
> + ? ? ? int i, ret = 0;
> ? ? ? ?int trans_bytes = 0, is_aligned = 1;
> ? ? ? ?u32 mask, flags, mode;
> ? ? ? ?unsigned int timeout, start_addr = 0;
> + ? ? ? unsigned int retry = 10000;
>
> ? ? ? ?/* Wait max 10 ms */
> ? ? ? ?timeout = 10;
> @@ -206,19 +207,31 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
> ? ? ? ?flush_cache(start_addr, trans_bytes);
> ?#endif
> ? ? ? ?sdhci_writew(host, SDHCI_MAKE_CMD(cmd->cmdidx, flags), SDHCI_COMMAND);
> - ? ? ? do {
> +
> + ? ? ? for (i = 0; i < retry; i++) {
> ? ? ? ? ? ? ? ?stat = sdhci_readl(host, SDHCI_INT_STATUS);
> - ? ? ? ? ? ? ? if (stat & SDHCI_INT_ERROR)
> + ? ? ? ? ? ? ? if (stat & (SDHCI_INT_RESPONSE | SDHCI_INT_DATA_END)) {
> + ? ? ? ? ? ? ? ? ? ? ? sdhci_cmd_done(host, cmd);
> + ? ? ? ? ? ? ? ? ? ? ? sdhci_writel(host, mask, SDHCI_INT_STATUS);
> + ? ? ? ? ? ? ? ? ? ? ? if (!data) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sdhci_writel(host, stat, ?SDHCI_INT_STATUS);

Why do two write?

> + ? ? ? ? ? ? ? ? ? ? ? }
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> - ? ? ? } while ((stat & mask) != mask);
> + ? ? ? ? ? ? ? }
> + ? ? ? }
>
> - ? ? ? if ((stat & (SDHCI_INT_ERROR | mask)) == mask) {
> - ? ? ? ? ? ? ? sdhci_cmd_done(host, cmd);
> - ? ? ? ? ? ? ? sdhci_writel(host, mask, SDHCI_INT_STATUS);
> - ? ? ? } else
> - ? ? ? ? ? ? ? ret = -1;
> + ? ? ? if (i == retry) {
> + ? ? ? ? ? ? ? printf("%s: waiting for status update\n",__func__);
> + ? ? ? ? ? ? ? return TIMEOUT;
> + ? ? ? }
> +
> + ? ? ? if (stat & SDHCI_INT_TIMEOUT) {
> + ? ? ? ? ? ? ? return TIMEOUT;
> + ? ? ? } else if (stat & SDHCI_INT_ERROR) {
> + ? ? ? ? ? ? ? return -1;
> + ? ? ? }
>
> - ? ? ? if (!ret && data)
> + ? ? ? if (data)
> ? ? ? ? ? ? ? ?ret = sdhci_transfer_data(host, data, start_addr);
>
> ? ? ? ?stat = sdhci_readl(host, SDHCI_INT_STATUS);


Thanks,
Lei

[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[Jaehoon Chung]

Hi Lei.

First, thanks for implemented the generic sdhci controller.

On 03/30/2012 12:33 PM, Lei Wen wrote:

> Hi Jaehoon,
> 
> On Fri, Mar 30, 2012 at 10:39 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> When response type is R1b, mask value is added the SDHCI_INT_DAT_END.
>> but in while(), didn't check that flag.
>> So sdhci controller didn't work fine.
>> CMD6 didn't always complete.
> 
> Could you elaborate it more in details?
>         do {
>                 stat = sdhci_readl(host, SDHCI_INT_STATUS);
>                 if (stat & SDHCI_INT_ERROR)
>                         break;
>         } while ((stat & mask) != mask);
> Here in the while condition, if the status read out don't contain all mask,
> then the looping would continue.
> Do you mean you just need a retry max time set here?

I found that didn't initialize the eMMC card.
Because when send CMD6, running infinite loop in there.
CMD6's mask is set to SDHCI_INT_RESPONSE and SDHCI_INT_DATA_END.
(Because response type is R1B).
Then mask value maybe is 0x3...
stat = sdhci_readl(host, SDHCI_INT_STATUS);
stat is 0x1.(cmd is done response).
but in while(), stat & mask is 0x1, and mask is 0x3.
...doing while().
If just add the timeout, then always produced the timeout error.

How did you test? Is initialize the eMMC card?
(I tested with eMMC4.41 (exynos4).)

> 
> 
>>
>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/mmc/sdhci.c |   33 +++++++++++++++++++++++----------
>>  1 files changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>> index fc904b5..0dd08b9 100644
>> --- a/drivers/mmc/sdhci.c
>> +++ b/drivers/mmc/sdhci.c
>> @@ -124,10 +124,11 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
>>  {
>>        struct sdhci_host *host = (struct sdhci_host *)mmc->priv;
>>        unsigned int stat = 0;
>> -       int ret = 0;
>> +       int i, ret = 0;
>>        int trans_bytes = 0, is_aligned = 1;
>>        u32 mask, flags, mode;
>>        unsigned int timeout, start_addr = 0;
>> +       unsigned int retry = 10000;
>>
>>        /* Wait max 10 ms */
>>        timeout = 10;
>> @@ -206,19 +207,31 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
>>        flush_cache(start_addr, trans_bytes);
>>  #endif
>>        sdhci_writew(host, SDHCI_MAKE_CMD(cmd->cmdidx, flags), SDHCI_COMMAND);
>> -       do {
>> +
>> +       for (i = 0; i < retry; i++) {
>>                stat = sdhci_readl(host, SDHCI_INT_STATUS);
>> -               if (stat & SDHCI_INT_ERROR)
>> +               if (stat & (SDHCI_INT_RESPONSE | SDHCI_INT_DATA_END)) {
>> +                       sdhci_cmd_done(host, cmd);
>> +                       sdhci_writel(host, mask, SDHCI_INT_STATUS);
>> +                       if (!data) {
>> +                               sdhci_writel(host, stat,  SDHCI_INT_STATUS);
> 
> Why do two write?

We can remove the sdhci_writel(host, mask SDHCI_INT_STATUS).
It's my mistake..i will fix that.

Best Regards,
Jaehoon Chung

[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[Lei Wen]

Hi Jaehoon,

On Fri, Mar 30, 2012 at 12:36 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi Lei.
>
> First, thanks for implemented the generic sdhci controller.

It is my pleasure to share this common code, and I'm glad that it is
used for other platforms now. :)

>
> On 03/30/2012 12:33 PM, Lei Wen wrote:
>
>> Hi Jaehoon,
>>
>> On Fri, Mar 30, 2012 at 10:39 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> When response type is R1b, mask value is added the SDHCI_INT_DAT_END.
>>> but in while(), didn't check that flag.
>>> So sdhci controller didn't work fine.
>>> CMD6 didn't always complete.
>>
>> Could you elaborate it more in details?
>> ? ? ? ? do {
>> ? ? ? ? ? ? ? ? stat = sdhci_readl(host, SDHCI_INT_STATUS);
>> ? ? ? ? ? ? ? ? if (stat & SDHCI_INT_ERROR)
>> ? ? ? ? ? ? ? ? ? ? ? ? break;
>> ? ? ? ? } while ((stat & mask) != mask);
>> Here in the while condition, if the status read out don't contain all mask,
>> then the looping would continue.
>> Do you mean you just need a retry max time set here?
>
> I found that didn't initialize the eMMC card.
> Because when send CMD6, running infinite loop in there.
> CMD6's mask is set to SDHCI_INT_RESPONSE and SDHCI_INT_DATA_END.
> (Because response type is R1B).
> Then mask value maybe is 0x3...
> stat = sdhci_readl(host, SDHCI_INT_STATUS);
> stat is 0x1.(cmd is done response).
> but in while(), stat & mask is 0x1, and mask is 0x3.
> ...doing while().
> If just add the timeout, then always produced the timeout error.

I see your problems. Your silicon only provide SDHCI_INT_RESPONSE for
r1b, while the standard
sdhci spec require both the flag is set when the r1b command is finished.
So my point is you could add a quirk condition check in the previously
endless loop checking.
If the quirk is true, and timeout count is met, then you could jump
out of original looping.
And for later checking, this timeout could be regarded as no error,
since the controller would never
return the SDHCI_INT_DATA_END in your case.

>
> How did you test? Is initialize the eMMC card?
> (I tested with eMMC4.41 (exynos4).)

I test on many Marvell platforms, with sd, mmc, emmc, they all works fine.

>
>>
>>
>>>
>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>> ?drivers/mmc/sdhci.c | ? 33 +++++++++++++++++++++++----------
>>> ?1 files changed, 23 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>> index fc904b5..0dd08b9 100644
>>> --- a/drivers/mmc/sdhci.c
>>> +++ b/drivers/mmc/sdhci.c
>>> @@ -124,10 +124,11 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
>>> ?{
>>> ? ? ? ?struct sdhci_host *host = (struct sdhci_host *)mmc->priv;
>>> ? ? ? ?unsigned int stat = 0;
>>> - ? ? ? int ret = 0;
>>> + ? ? ? int i, ret = 0;
>>> ? ? ? ?int trans_bytes = 0, is_aligned = 1;
>>> ? ? ? ?u32 mask, flags, mode;
>>> ? ? ? ?unsigned int timeout, start_addr = 0;
>>> + ? ? ? unsigned int retry = 10000;
>>>
>>> ? ? ? ?/* Wait max 10 ms */
>>> ? ? ? ?timeout = 10;
>>> @@ -206,19 +207,31 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
>>> ? ? ? ?flush_cache(start_addr, trans_bytes);
>>> ?#endif
>>> ? ? ? ?sdhci_writew(host, SDHCI_MAKE_CMD(cmd->cmdidx, flags), SDHCI_COMMAND);
>>> - ? ? ? do {
>>> +
>>> + ? ? ? for (i = 0; i < retry; i++) {
>>> ? ? ? ? ? ? ? ?stat = sdhci_readl(host, SDHCI_INT_STATUS);
>>> - ? ? ? ? ? ? ? if (stat & SDHCI_INT_ERROR)
>>> + ? ? ? ? ? ? ? if (stat & (SDHCI_INT_RESPONSE | SDHCI_INT_DATA_END)) {
>>> + ? ? ? ? ? ? ? ? ? ? ? sdhci_cmd_done(host, cmd);
>>> + ? ? ? ? ? ? ? ? ? ? ? sdhci_writel(host, mask, SDHCI_INT_STATUS);
>>> + ? ? ? ? ? ? ? ? ? ? ? if (!data) {
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sdhci_writel(host, stat, ?SDHCI_INT_STATUS);
>>
>> Why do two write?
>
> We can remove the sdhci_writel(host, mask SDHCI_INT_STATUS).
> It's my mistake..i will fix that.
>
> Best Regards,
> Jaehoon Chung

Thanks,
Lei

[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[Jaehoon Chung]

On 03/30/2012 02:24 PM, Lei Wen wrote:

> Hi Jaehoon,
> 
> On Fri, Mar 30, 2012 at 12:36 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi Lei.
>>
>> First, thanks for implemented the generic sdhci controller.
> 
> It is my pleasure to share this common code, and I'm glad that it is
> used for other platforms now. :)
> 
>>
>> On 03/30/2012 12:33 PM, Lei Wen wrote:
>>
>>> Hi Jaehoon,
>>>
>>> On Fri, Mar 30, 2012 at 10:39 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>> When response type is R1b, mask value is added the SDHCI_INT_DAT_END.
>>>> but in while(), didn't check that flag.
>>>> So sdhci controller didn't work fine.
>>>> CMD6 didn't always complete.
>>>
>>> Could you elaborate it more in details?
>>>         do {
>>>                 stat = sdhci_readl(host, SDHCI_INT_STATUS);
>>>                 if (stat & SDHCI_INT_ERROR)
>>>                         break;
>>>         } while ((stat & mask) != mask);
>>> Here in the while condition, if the status read out don't contain all mask,
>>> then the looping would continue.
>>> Do you mean you just need a retry max time set here?
>>
>> I found that didn't initialize the eMMC card.
>> Because when send CMD6, running infinite loop in there.
>> CMD6's mask is set to SDHCI_INT_RESPONSE and SDHCI_INT_DATA_END.
>> (Because response type is R1B).
>> Then mask value maybe is 0x3...
>> stat = sdhci_readl(host, SDHCI_INT_STATUS);
>> stat is 0x1.(cmd is done response).
>> but in while(), stat & mask is 0x1, and mask is 0x3.
>> ...doing while().
>> If just add the timeout, then always produced the timeout error.
> 
> I see your problems. Your silicon only provide SDHCI_INT_RESPONSE for
> r1b, while the standard
> sdhci spec require both the flag is set when the r1b command is finished.
> So my point is you could add a quirk condition check in the previously
> endless loop checking.
> If the quirk is true, and timeout count is met, then you could jump
> out of original looping.
> And for later checking, this timeout could be regarded as no error,
> since the controller would never
> return the SDHCI_INT_DATA_END in your case.


Anyway it's need that use the timeout value. right?
And in our case, It's means that use the quirks?
I didn't find that the sdhci spec require the flag is set when the r1b command is finished.
where is specified?
I just understand that the transfer complete bit should be set to 1,
when send the command with busy flag or read/write with data.
Is this means?

If you want to use the quirks, i will test with applied your opinion.

Best Regards,
Jaehoon Chung

> 
>>
>> How did you test? Is initialize the eMMC card?
>> (I tested with eMMC4.41 (exynos4).)
> 
> I test on many Marvell platforms, with sd, mmc, emmc, they all works fine.
> 
>>

[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[Lei Wen]

Hi Jaehoon,

On Fri, Mar 30, 2012 at 2:23 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 03/30/2012 02:24 PM, Lei Wen wrote:
>
>> Hi Jaehoon,
>>
>> On Fri, Mar 30, 2012 at 12:36 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> Hi Lei.
>>>
>>> First, thanks for implemented the generic sdhci controller.
>>
>> It is my pleasure to share this common code, and I'm glad that it is
>> used for other platforms now. :)
>>
>>>
>>> On 03/30/2012 12:33 PM, Lei Wen wrote:
>>>
>>>> Hi Jaehoon,
>>>>
>>>> On Fri, Mar 30, 2012 at 10:39 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>> When response type is R1b, mask value is added the SDHCI_INT_DAT_END.
>>>>> but in while(), didn't check that flag.
>>>>> So sdhci controller didn't work fine.
>>>>> CMD6 didn't always complete.
>>>>
>>>> Could you elaborate it more in details?
>>>> ? ? ? ? do {
>>>> ? ? ? ? ? ? ? ? stat = sdhci_readl(host, SDHCI_INT_STATUS);
>>>> ? ? ? ? ? ? ? ? if (stat & SDHCI_INT_ERROR)
>>>> ? ? ? ? ? ? ? ? ? ? ? ? break;
>>>> ? ? ? ? } while ((stat & mask) != mask);
>>>> Here in the while condition, if the status read out don't contain all mask,
>>>> then the looping would continue.
>>>> Do you mean you just need a retry max time set here?
>>>
>>> I found that didn't initialize the eMMC card.
>>> Because when send CMD6, running infinite loop in there.
>>> CMD6's mask is set to SDHCI_INT_RESPONSE and SDHCI_INT_DATA_END.
>>> (Because response type is R1B).
>>> Then mask value maybe is 0x3...
>>> stat = sdhci_readl(host, SDHCI_INT_STATUS);
>>> stat is 0x1.(cmd is done response).
>>> but in while(), stat & mask is 0x1, and mask is 0x3.
>>> ...doing while().
>>> If just add the timeout, then always produced the timeout error.
>>
>> I see your problems. Your silicon only provide SDHCI_INT_RESPONSE for
>> r1b, while the standard
>> sdhci spec require both the flag is set when the r1b command is finished.
>> So my point is you could add a quirk condition check in the previously
>> endless loop checking.
>> If the quirk is true, and timeout count is met, then you could jump
>> out of original looping.
>> And for later checking, this timeout could be regarded as no error,
>> since the controller would never
>> return the SDHCI_INT_DATA_END in your case.
>
>
> Anyway it's need that use the timeout value. right?
Yes, for the timeout patch here, I am ok with that.

> And in our case, It's means that use the quirks?
Using quirks, or set a timeout value large enough? Maybe latter is
more generic. :)

> I didn't find that the sdhci spec require the flag is set when the r1b command is finished.
> where is specified?
The spec I mention is
https://www.sdcard.org/developers/overview/host_controller/simple_spec/Simplified_SD_Host_Controller_Spec.pdf
You could look at page 64, the bit description for "Transfer Complete":
(3) In the case of a command with busy
This bit is set when busy is de-asserted. Refer to DAT Line Active
and Command Inhibit (DAT) in the Present State register

> I just understand that the transfer complete bit should be set to 1,
> when send the command with busy flag or read/write with data.
> Is this means?

Yes, that is what I means, if you send a command with r1b flag, then
you should expect both
transfer complete and command complete bit is set, only command
complete bit set doesn't mean
your command is safely treated, and you could do the next step further.

>
> If you want to use the quirks, i will test with applied your opinion.
I think you only need to add timeout into this while looping, and not
touching other code.
        do {
                stat = sdhci_readl(host, SDHCI_INT_STATUS);
                if (stat & SDHCI_INT_ERROR)
                        break;
        } while ((stat & mask) != mask);

>
> Best Regards,
> Jaehoon Chung

Thanks,
Lei

[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[Jae hoon Chung]

Hi Lei.

I will try to  test with  your opinion.
Just wondering..If apply with my patch, is it something problem?
Anyway i will test and share the result. then resend the patch.

I want to use the generic sdhci code.

Best Regards,
Jaehoon Chung

2012/3/31 Lei Wen <adrian.wenl@gmail.com>:
> Hi Jaehoon,
>
> On Fri, Mar 30, 2012 at 2:23 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> On 03/30/2012 02:24 PM, Lei Wen wrote:
>>
>>> Hi Jaehoon,
>>>
>>> On Fri, Mar 30, 2012 at 12:36 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>> Hi Lei.
>>>>
>>>> First, thanks for implemented the generic sdhci controller.
>>>
>>> It is my pleasure to share this common code, and I'm glad that it is
>>> used for other platforms now. :)
>>>
>>>>
>>>> On 03/30/2012 12:33 PM, Lei Wen wrote:
>>>>
>>>>> Hi Jaehoon,
>>>>>
>>>>> On Fri, Mar 30, 2012 at 10:39 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>>> When response type is R1b, mask value is added the SDHCI_INT_DAT_END.
>>>>>> but in while(), didn't check that flag.
>>>>>> So sdhci controller didn't work fine.
>>>>>> CMD6 didn't always complete.
>>>>>
>>>>> Could you elaborate it more in details?
>>>>> ? ? ? ? do {
>>>>> ? ? ? ? ? ? ? ? stat = sdhci_readl(host, SDHCI_INT_STATUS);
>>>>> ? ? ? ? ? ? ? ? if (stat & SDHCI_INT_ERROR)
>>>>> ? ? ? ? ? ? ? ? ? ? ? ? break;
>>>>> ? ? ? ? } while ((stat & mask) != mask);
>>>>> Here in the while condition, if the status read out don't contain all mask,
>>>>> then the looping would continue.
>>>>> Do you mean you just need a retry max time set here?
>>>>
>>>> I found that didn't initialize the eMMC card.
>>>> Because when send CMD6, running infinite loop in there.
>>>> CMD6's mask is set to SDHCI_INT_RESPONSE and SDHCI_INT_DATA_END.
>>>> (Because response type is R1B).
>>>> Then mask value maybe is 0x3...
>>>> stat = sdhci_readl(host, SDHCI_INT_STATUS);
>>>> stat is 0x1.(cmd is done response).
>>>> but in while(), stat & mask is 0x1, and mask is 0x3.
>>>> ...doing while().
>>>> If just add the timeout, then always produced the timeout error.
>>>
>>> I see your problems. Your silicon only provide SDHCI_INT_RESPONSE for
>>> r1b, while the standard
>>> sdhci spec require both the flag is set when the r1b command is finished.
>>> So my point is you could add a quirk condition check in the previously
>>> endless loop checking.
>>> If the quirk is true, and timeout count is met, then you could jump
>>> out of original looping.
>>> And for later checking, this timeout could be regarded as no error,
>>> since the controller would never
>>> return the SDHCI_INT_DATA_END in your case.
>>
>>
>> Anyway it's need that use the timeout value. right?
> Yes, for the timeout patch here, I am ok with that.
>
>> And in our case, It's means that use the quirks?
> Using quirks, or set a timeout value large enough? Maybe latter is
> more generic. :)
>
>> I didn't find that the sdhci spec require the flag is set when the r1b command is finished.
>> where is specified?
> The spec I mention is
> https://www.sdcard.org/developers/overview/host_controller/simple_spec/Simplified_SD_Host_Controller_Spec.pdf
> You could look at page 64, the bit description for "Transfer Complete":
> (3) In the case of a command with busy
> This bit is set when busy is de-asserted. Refer to DAT Line Active
> and Command Inhibit (DAT) in the Present State register
>
>> I just understand that the transfer complete bit should be set to 1,
>> when send the command with busy flag or read/write with data.
>> Is this means?
>
> Yes, that is what I means, if you send a command with r1b flag, then
> you should expect both
> transfer complete and command complete bit is set, only command
> complete bit set doesn't mean
> your command is safely treated, and you could do the next step further.
>
>>
>> If you want to use the quirks, i will test with applied your opinion.
> I think you only need to add timeout into this while looping, and not
> touching other code.
> ? ? ? ?do {
> ? ? ? ? ? ? ? ?stat = sdhci_readl(host, SDHCI_INT_STATUS);
> ? ? ? ? ? ? ? ?if (stat & SDHCI_INT_ERROR)
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ?} while ((stat & mask) != mask);
>
>>
>> Best Regards,
>> Jaehoon Chung
>
> Thanks,
> Lei
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v2 1/3] mmc: sdhci: fix the wrong operation when response type is R1b

[Lei Wen]

Hi Jaehoon,

On Sat, Mar 31, 2012 at 2:55 PM, Jae hoon Chung <jh80.chung@gmail.com> wrote:
> Hi Lei.
>
> I will try to ?test with ?your opinion.
> Just wondering..If apply with my patch, is it something problem?

First, you patch limit the original mask which could be extend to
other flag to only
SDHCI_INT_RESPONSE | SDHCI_INT_DATA_END, second, obviously you just need
a timeout, so only two or three lines changes of code to implement it
to keep the patch
simple.

> Anyway i will test and share the result. then resend the patch.
>
> I want to use the generic sdhci code.
That is also my glad to more people to use it.

>
> Best Regards,
> Jaehoon Chung

Thanks,
Lei