| Message ID | 5040204D.9050302@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andy Fleming |
| Headers | show |
On Thu, Aug 30, 2012 at 7:24 PM, Jaehoon Chung <jh80.chung@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.chung@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@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 = 10000; > + timeout = 100000; > 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 100000 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
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.chung@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.chung@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@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 = 10000; >> + timeout = 100000; >> 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 100000 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 >
Hi Andy and Lei, I have some question. We used the timeout value at sdhci_transfer_data(). How did we get the timeout value "10000"? 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.chung@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.chung@samsung.com> >>> Signed-off-by: Kyungmin Park <kyungmin.park@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 = 10000; >>> + timeout = 100000; >>> 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 100000 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 >
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 = 10000; + timeout = 100000; 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;