Re: [U-Boot] [PATCH v3 4/4] mmc: sdhci: increase the timeout and udelay value

2012-09-02 Thread Jaehoon Chung 
Hi Andy,

I understood your comment,
I will try to solve the problem.
(didn't change the udelay and loop count)
Then Could you merge the patch [1/4~3/4]?
I will debug more and resend the patch related with this problem.

If you can merge the patches[1/4~3/4], I think that other people can also debug 
this problem.
(if produce the problem.)

Best Regards,
Jaehoon Chung

On 09/01/2012 06:16 AM, Andy Fleming wrote:
 On Thu, Aug 30, 2012 at 7:24 PM, Jaehoon Chung jh80.ch...@samsung.com wrote:
 Samsung-SoC is taken the too late to changing the interrupt status register.
 This patch is ensure to check the interrupt status register for Samsung-SoC.

 Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
 
 
 You should write, here, what you changed in v3.
 
 
  drivers/mmc/sdhci.c |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
 index ac39e48..d0b8d24 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 = 1;
 +   timeout = 10;
 rdy = SDHCI_INT_SPACE_AVAIL | SDHCI_INT_DATA_AVAIL;
 mask = SDHCI_DATA_AVAILABLE | SDHCI_SPACE_AVAILABLE;
 do {
 @@ -110,7 +110,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, 
 struct mmc_data *data,
 }
  #endif
 if (timeout--  0)
 -   udelay(10);
 +   udelay(20);
 
 
 ... This change makes no sense.
 
 Actually, this whole *function* makes no sense. It seems to me that if
 you attempt to transfer more than 10 blocks, it will fail. A
 timeout variable should only be used to control the number of
 iterations through a wait loop. This loop does more than wait, it also
 executes an ongoing, multiblock transfer.
 
 I'm not exactly sure what issue this change is solving, but extending
 the delay, and increasing the number of iterations is *not* the
 solution. You're just hiding the problem.
 
 You need to figure out why your transfer failed, and then modify the
 code to actually solve that problem. And I strongly think you need to
 refactor this transfer loop so that it properly transfers all desired
 blocks, and times out only when timeouts happen.
 
 Andy
 

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 4/4] mmc: sdhci: increase the timeout and udelay value

2012-09-02 Thread Jaehoon Chung 
Hi Andy and Lei,

I have some question.
We used the timeout value at sdhci_transfer_data().
How did we get the timeout value 1? and must set the timeout value?
I think that used to prevent the infinite loop. right?
i want to know how did we get that timeout value?

Before set the interrupt status register, timeout value is set to zero.
So returned error with Data transfer timeout message.

Best Regards,
Jaehoon Chung

On 09/03/2012 11:44 AM, Jaehoon Chung wrote:
 Hi Andy,
 
 I understood your comment,
 I will try to solve the problem.
 (didn't change the udelay and loop count)
 Then Could you merge the patch [1/4~3/4]?
 I will debug more and resend the patch related with this problem.
 
 If you can merge the patches[1/4~3/4], I think that other people can also 
 debug this problem.
 (if produce the problem.)
 
 Best Regards,
 Jaehoon Chung
 
 On 09/01/2012 06:16 AM, Andy Fleming wrote:
 On Thu, Aug 30, 2012 at 7:24 PM, Jaehoon Chung jh80.ch...@samsung.com 
 wrote:
 Samsung-SoC is taken the too late to changing the interrupt status register.
 This patch is ensure to check the interrupt status register for Samsung-SoC.

 Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---


 You should write, here, what you changed in v3.


  drivers/mmc/sdhci.c |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
 index ac39e48..d0b8d24 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 = 1;
 +   timeout = 10;
 rdy = SDHCI_INT_SPACE_AVAIL | SDHCI_INT_DATA_AVAIL;
 mask = SDHCI_DATA_AVAILABLE | SDHCI_SPACE_AVAILABLE;
 do {
 @@ -110,7 +110,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, 
 struct mmc_data *data,
 }
  #endif
 if (timeout--  0)
 -   udelay(10);
 +   udelay(20);


 ... This change makes no sense.

 Actually, this whole *function* makes no sense. It seems to me that if
 you attempt to transfer more than 10 blocks, it will fail. A
 timeout variable should only be used to control the number of
 iterations through a wait loop. This loop does more than wait, it also
 executes an ongoing, multiblock transfer.

 I'm not exactly sure what issue this change is solving, but extending
 the delay, and increasing the number of iterations is *not* the
 solution. You're just hiding the problem.

 You need to figure out why your transfer failed, and then modify the
 code to actually solve that problem. And I strongly think you need to
 refactor this transfer loop so that it properly transfers all desired
 blocks, and times out only when timeouts happen.

 Andy

 
 ___
 U-Boot mailing list
 U-Boot@lists.denx.de
 http://lists.denx.de/mailman/listinfo/u-boot
 

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 4/4] mmc: sdhci: increase the timeout and udelay value

2012-08-31 Thread Andy Fleming 
On Thu, Aug 30, 2012 at 7:24 PM, Jaehoon Chung jh80.ch...@samsung.com wrote:
 Samsung-SoC is taken the too late to changing the interrupt status register.
 This patch is ensure to check the interrupt status register for Samsung-SoC.

 Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---


You should write, here, what you changed in v3.


  drivers/mmc/sdhci.c |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
 index ac39e48..d0b8d24 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 = 1;
 +   timeout = 10;
 rdy = SDHCI_INT_SPACE_AVAIL | SDHCI_INT_DATA_AVAIL;
 mask = SDHCI_DATA_AVAILABLE | SDHCI_SPACE_AVAILABLE;
 do {
 @@ -110,7 +110,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, 
 struct mmc_data *data,
 }
  #endif
 if (timeout--  0)
 -   udelay(10);
 +   udelay(20);


... This change makes no sense.

Actually, this whole *function* makes no sense. It seems to me that if
you attempt to transfer more than 10 blocks, it will fail. A
timeout variable should only be used to control the number of
iterations through a wait loop. This loop does more than wait, it also
executes an ongoing, multiblock transfer.

I'm not exactly sure what issue this change is solving, but extending
the delay, and increasing the number of iterations is *not* the
solution. You're just hiding the problem.

You need to figure out why your transfer failed, and then modify the
code to actually solve that problem. And I strongly think you need to
refactor this transfer loop so that it properly transfers all desired
blocks, and times out only when timeouts happen.

Andy
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v3 4/4] mmc: sdhci: increase the timeout and udelay value

2012-08-30 Thread Jaehoon Chung 
Samsung-SoC is taken the too late to changing the interrupt status register.
This patch is ensure to check the interrupt status register for Samsung-SoC.

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

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index ac39e48..d0b8d24 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 = 1;
+   timeout = 10;
rdy = SDHCI_INT_SPACE_AVAIL | SDHCI_INT_DATA_AVAIL;
mask = SDHCI_DATA_AVAILABLE | SDHCI_SPACE_AVAILABLE;
do {
@@ -110,7 +110,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, 
struct mmc_data *data,
}
 #endif
if (timeout--  0)
-   udelay(10);
+   udelay(20);
else {
printf(Transfer data timeout\n);
return -1;
-- 
1.7.4.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot