diff mbox

[U-Boot,1/3] mmc: sdhci: increase the timeout value for data transfer

Message ID 505C09DA.8030102@samsung.com
State Accepted
Delegated to: Andy Fleming
Headers show

Commit Message

Jaehoon Chung Sept. 21, 2012, 6:31 a.m. UTC 
Timeout value is tunable.
When run read/write operation, sometime returned the timeout error.
Because the timeout value is too short.
So increased the enough timeout value.
(This timeout value is used to prevent the infinite loop.)

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

Comments

Rommel Custodio Sept. 21, 2012, 9:48 a.m. UTC | #1 
Jaehoon Chung <jh80.chung <at> samsung.com> writes:

> 
> Timeout value is tunable.
> When run read/write operation, sometime returned the timeout error.
> Because the timeout value is too short.

Hello,

I think it is better to fine tune CONFIG_SYS_MMC_MAX_BLK_COUNT.
This gets assigned to mmc->b_max, unless you specifically set
mmc->b_max value during mmc_register().

b_max is important since when you adjust b_max properly,
mmc_bread() and mmc_bwrite() will properly  partition the
read/write operation (in b_max blocks) so that the timeout
does not occur.

All the best,
Rommel

(replying via gmane since i just joined the list)
Jaehoon Chung Sept. 24, 2012, 1:32 a.m. UTC | #2 
Hi Rommel,

I didn't think so..Our environment is support the CONFIG_SYS_MMC_MAX_BLK_COUNT.
Did you know how get the timeout value "1000"?

If the timeout value "1000" is reasonable, i want to know what basis.
Well, i don't think that my timeout value is reasonable.

Actually i want to remove the timeout value in that function.
But then we should be prevent the infinite loop.

Anyway, thanks for your comment. I will check the your opinion.

Best Regards,
Jaehoon Chung

On 09/21/2012 06:48 PM, Rommel Custodio wrote:
> 
> Jaehoon Chung <jh80.chung <at> samsung.com> writes:
> 
>>
>> Timeout value is tunable.
>> When run read/write operation, sometime returned the timeout error.
>> Because the timeout value is too short.
> 
> Hello,
> 
> I think it is better to fine tune CONFIG_SYS_MMC_MAX_BLK_COUNT.
> This gets assigned to mmc->b_max, unless you specifically set
> mmc->b_max value during mmc_register().
> 
> b_max is important since when you adjust b_max properly,
> mmc_bread() and mmc_bwrite() will properly  partition the
> read/write operation (in b_max blocks) so that the timeout
> does not occur.
> 
> All the best,
> Rommel
> 
> (replying via gmane since i just joined the list)
> 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Rommel Custodio Sept. 24, 2012, 2:34 a.m. UTC | #3 
Hello Jaehoon,

> I didn't think so..Our environment is support the
> CONFIG_SYS_MMC_MAX_BLK_COUNT.

This is defined in mmc.c right after the include definitions.
The comment says that:
  Set block count limit because of 16 bit
  register limit on some hardware

So my use of this define is a bit of a hack too.


> Did you know how get the timeout value "1000"?
> 
> If the timeout value "1000" is reasonable, i want
> to know what basis. Well, i don't think that my
> timeout value is reasonable.

I think timeout value was from original creator/maintainer
of SD/MMC code.

When trying to read big data off of the SD card, I get the
timeout too on my platform (ml507). My test consist of reading
a 26Mb file from the SD card in PIO and DMA mode.

When the timeout is displayed in the serial console, the block
count register is not yet zero but there is no error in the
interrupt status register. The block count value shows the
same value when I perform the same test.

For my use, CONFIG_SYS_MMC_MAX_BLK_COUNT = 0x1000 in my
environment. I did not modify any of the timeout values.


> 
> Actually i want to remove the timeout value in that function.
> But then we should be prevent the infinite loop.

The infinite loop that you mention does not occur in my
situation.

> 
> Anyway, thanks for your comment. I will check the your opinion.

HTH

(replying via gmane since i just joined the list)
Jaehoon Chung Sept. 24, 2012, 5:23 a.m. UTC | #4 
Hi Rommel,

On 09/24/2012 11:34 AM, Rommel Custodio wrote:
> Hello Jaehoon,
> 
>> I didn't think so..Our environment is support the
>> CONFIG_SYS_MMC_MAX_BLK_COUNT.
> 
> This is defined in mmc.c right after the include definitions.
> The comment says that:
>   Set block count limit because of 16 bit
>   register limit on some hardware
I known that defined into mmc.c.
After changed the MMC_MAX_BLK_COUNT, it also produced "Transfer data timeout".
When MMC_MAX_BLOCK_COUNT is set to 1, didn't produce the error log.
But it's too late.
> 
> So my use of this define is a bit of a hack too.
> 
> 
>> Did you know how get the timeout value "1000"?
>>
>> If the timeout value "1000" is reasonable, i want
>> to know what basis. Well, i don't think that my
>> timeout value is reasonable.
> 
> I think timeout value was from original creator/maintainer
> of SD/MMC code.
Right, the timeout value has set from original code.
> 
> When trying to read big data off of the SD card, I get the
> timeout too on my platform (ml507). My test consist of reading
> a 26Mb file from the SD card in PIO and DMA mode.
> 
> When the timeout is displayed in the serial console, the block
> count register is not yet zero but there is no error in the
> interrupt status register. The block count value shows the
> same value when I perform the same test.
This timeout value isn't timeout value related with TIMEOUT control register.
Your comment seems to mention the TIMEOUT control register.
(As my understanding..if my understanding is wrong, sorry..)
> 
> For my use, CONFIG_SYS_MMC_MAX_BLK_COUNT = 0x1000 in my
> environment. I did not modify any of the timeout values.
> 
> 
>>
>> Actually i want to remove the timeout value in that function.
>> But then we should be prevent the infinite loop.
> 
> The infinite loop that you mention does not occur in my
> situation.
I also didn't occur the infinite loop.
So i think that we can remove the timeout value in that function.

Best Regards,
Jaehoon Chung
> 
>>
>> Anyway, thanks for your comment. I will check the your opinion.
> 
> HTH
> 
> (replying via gmane since i just joined the list)
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Mela Custodio Sept. 24, 2012, 7:57 p.m. UTC | #5 
On Mon, Sep 24, 2012 at 2:23 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> The infinite loop that you mention does not occur in my
>> situation.
> I also didn't occur the infinite loop.
> So i think that we can remove the timeout value in that function.
Hi,

I have no comment about that. In your local environment you could remove it but
I doubt that that will be good in mainline u-boot though.

All the best,
RgC
Jaehoon Chung Sept. 25, 2012, 1:33 a.m. UTC | #6 
Hi,

On 09/25/2012 04:57 AM, Mela Custodio wrote:
> On Mon, Sep 24, 2012 at 2:23 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> The infinite loop that you mention does not occur in my
>>> situation.
>> I also didn't occur the infinite loop.
>> So i think that we can remove the timeout value in that function.
> Hi,
> 
> I have no comment about that. In your local environment you could remove it but
> I doubt that that will be good in mainline u-boot though.
Yes, In local environment, i can remove it. but in mainline, i agreed your opinion.
As your mention, it would be not good that remove the timeout.
So i increased the timeout value from 1000 to 100000.
then we can get more chance to check the interrupt status register.
My problem is that host is returned the timeout error before changing the interrupt status register.

Best Regards,
Jaehoon Chung
> 
> All the best,
> RgC
>
diff mbox

Patch

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 2e3c408..9329874 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -83,7 +83,7 @@  static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
 {
 	unsigned int stat, rdy, mask, timeout, block = 0;
 
-	timeout = 10000;
+	timeout = 1000000;
 	rdy = SDHCI_INT_SPACE_AVAIL | SDHCI_INT_DATA_AVAIL;
 	mask = SDHCI_DATA_AVAILABLE | SDHCI_SPACE_AVAILABLE;
 	do {