| Message ID | 1467063821-15900-1-git-send-email-steve.rae@raedomain.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Steve, On 27.06.2016 23:43, Steve Rae wrote: > Otherwise, ocassionally see errors like this: > Flashing sparse image at offset 2078720 > Flashing Sparse Image > sdhci_send_command: Timeout for status update! > mmc fail to send stop cmd > write_sparse_image: Write failed, block #2181088 [0] > > This does not affect the actual writing speed, which is controlled by > the default value: > CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT > > It only increases the retries when reading: > SDHCI_INT_STATUS > to avoid the timeout error. > > Signed-off-by: Steve Rae <steve.rae@raedomain.com> > --- > as per the discussion in: > http://lists.denx.de/pipermail/u-boot/2016-June/258966.html > this supercedes: > http://patchwork.ozlabs.org/patch/615994/ IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig and use the old value as default value. So that you can overwrite it for your board / platform via your defconfig. But I have no strong feelings here - your current version also works for me and does not "clutter" the Kconfig subsystem with too many values. So: Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > drivers/mmc/sdhci.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c > index 5c71ab8..aa4cd4f 100644 > --- a/drivers/mmc/sdhci.c > +++ b/drivers/mmc/sdhci.c > @@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data, > #define CONFIG_SDHCI_CMD_MAX_TIMEOUT 3200 > #endif > #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT 100 > +#define CONFIG_SDHCI_READ_STATUS_TIMEOUT 1000 > > static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd, > struct mmc_data *data) > @@ -243,9 +244,9 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd, > if (stat & SDHCI_INT_ERROR) > break; > } while (((stat & mask) != mask) && > - (get_timer(start) < CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT)); > + (get_timer(start) < CONFIG_SDHCI_READ_STATUS_TIMEOUT)); > > - if (get_timer(start) >= CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT) { > + if (get_timer(start) >= CONFIG_SDHCI_READ_STATUS_TIMEOUT) { > if (host->quirks & SDHCI_QUIRK_BROKEN_R1B) > return 0; > else { > Viele Grüße, Stefan
Hi Stefan, On Tue, Jun 28, 2016 at 8:00 AM, Stefan Roese <sr@denx.de> wrote: > Hi Steve, > > On 27.06.2016 23:43, Steve Rae wrote: >> >> Otherwise, ocassionally see errors like this: >> Flashing sparse image at offset 2078720 >> Flashing Sparse Image >> sdhci_send_command: Timeout for status update! >> mmc fail to send stop cmd >> write_sparse_image: Write failed, block #2181088 [0] >> >> This does not affect the actual writing speed, which is controlled by >> the default value: >> CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT >> >> It only increases the retries when reading: >> SDHCI_INT_STATUS >> to avoid the timeout error. >> >> Signed-off-by: Steve Rae <steve.rae@raedomain.com> >> --- >> as per the discussion in: >> http://lists.denx.de/pipermail/u-boot/2016-June/258966.html >> this supercedes: >> http://patchwork.ozlabs.org/patch/615994/ > > > IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig > and use the old value as default value. So that you can overwrite > it for your board / platform via your defconfig. But I have no > strong feelings here - your current version also works for > me and does not "clutter" the Kconfig subsystem with too many > values. So: > > Reviewed-by: Stefan Roese <sr@denx.de> > > Thanks, > Stefan > Thanks for the review... I didn't want to touch the "performance" algorithm related to SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to be in Kconfig -- it is just a define. Thanks, Steve >> drivers/mmc/sdhci.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c >> index 5c71ab8..aa4cd4f 100644 >> --- a/drivers/mmc/sdhci.c >> +++ b/drivers/mmc/sdhci.c >> @@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host >> *host, struct mmc_data *data, >> #define CONFIG_SDHCI_CMD_MAX_TIMEOUT 3200 >> #endif >> #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT 100 >> +#define CONFIG_SDHCI_READ_STATUS_TIMEOUT 1000 >> >> static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd, >> struct mmc_data *data) >> @@ -243,9 +244,9 @@ static int sdhci_send_command(struct mmc *mmc, struct >> mmc_cmd *cmd, >> if (stat & SDHCI_INT_ERROR) >> break; >> } while (((stat & mask) != mask) && >> - (get_timer(start) < CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT)); >> + (get_timer(start) < CONFIG_SDHCI_READ_STATUS_TIMEOUT)); >> >> - if (get_timer(start) >= CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT) { >> + if (get_timer(start) >= CONFIG_SDHCI_READ_STATUS_TIMEOUT) { >> if (host->quirks & SDHCI_QUIRK_BROKEN_R1B) >> return 0; >> else { >> > > Viele Grüße, > Stefan > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
On Tue, Jun 28, 2016 at 01:30:09PM -0700, Steve Rae wrote: > Hi Stefan, > > On Tue, Jun 28, 2016 at 8:00 AM, Stefan Roese <sr@denx.de> wrote: > > Hi Steve, > > > > On 27.06.2016 23:43, Steve Rae wrote: > >> > >> Otherwise, ocassionally see errors like this: > >> Flashing sparse image at offset 2078720 > >> Flashing Sparse Image > >> sdhci_send_command: Timeout for status update! > >> mmc fail to send stop cmd > >> write_sparse_image: Write failed, block #2181088 [0] > >> > >> This does not affect the actual writing speed, which is controlled by > >> the default value: > >> CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT > >> > >> It only increases the retries when reading: > >> SDHCI_INT_STATUS > >> to avoid the timeout error. > >> > >> Signed-off-by: Steve Rae <steve.rae@raedomain.com> > >> --- > >> as per the discussion in: > >> http://lists.denx.de/pipermail/u-boot/2016-June/258966.html > >> this supercedes: > >> http://patchwork.ozlabs.org/patch/615994/ > > > > > > IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig > > and use the old value as default value. So that you can overwrite > > it for your board / platform via your defconfig. But I have no > > strong feelings here - your current version also works for > > me and does not "clutter" the Kconfig subsystem with too many > > values. So: > > > > Reviewed-by: Stefan Roese <sr@denx.de> > > > > Thanks, > > Stefan > > > > Thanks for the review... > I didn't want to touch the "performance" algorithm related to > SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). > However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to > be in Kconfig -- it is just a define. ... so how is this handled in the kernel? I'm assuming some DT property..
Hi Tom, On Tue, Jun 28, 2016 at 1:34 PM, Tom Rini <trini@konsulko.com> wrote: > On Tue, Jun 28, 2016 at 01:30:09PM -0700, Steve Rae wrote: >> Hi Stefan, >> >> On Tue, Jun 28, 2016 at 8:00 AM, Stefan Roese <sr@denx.de> wrote: >> > Hi Steve, >> > >> > On 27.06.2016 23:43, Steve Rae wrote: >> >> >> >> Otherwise, ocassionally see errors like this: >> >> Flashing sparse image at offset 2078720 >> >> Flashing Sparse Image >> >> sdhci_send_command: Timeout for status update! >> >> mmc fail to send stop cmd >> >> write_sparse_image: Write failed, block #2181088 [0] >> >> >> >> This does not affect the actual writing speed, which is controlled by >> >> the default value: >> >> CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT >> >> >> >> It only increases the retries when reading: >> >> SDHCI_INT_STATUS >> >> to avoid the timeout error. >> >> >> >> Signed-off-by: Steve Rae <steve.rae@raedomain.com> >> >> --- >> >> as per the discussion in: >> >> http://lists.denx.de/pipermail/u-boot/2016-June/258966.html >> >> this supercedes: >> >> http://patchwork.ozlabs.org/patch/615994/ >> > >> > >> > IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig >> > and use the old value as default value. So that you can overwrite >> > it for your board / platform via your defconfig. But I have no >> > strong feelings here - your current version also works for >> > me and does not "clutter" the Kconfig subsystem with too many >> > values. So: >> > >> > Reviewed-by: Stefan Roese <sr@denx.de> >> > >> > Thanks, >> > Stefan >> > >> >> Thanks for the review... >> I didn't want to touch the "performance" algorithm related to >> SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). >> However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to >> be in Kconfig -- it is just a define. > > ... so how is this handled in the kernel? I'm assuming some DT > property.. > > -- > Tom ( we discussed this in the other email thread; but I'll copy it here... ) It looks like (v.4.6) the code loops for max_loops=16, and it looks like the loop delay is created by a write (which does not exist in the U-Boot code): sdhci_writel(host, mask, SDHCI_INT_STATUS); Thanks, Steve
On Tue, Jun 28, 2016 at 01:53:52PM -0700, Steve Rae wrote: > Hi Tom, > > On Tue, Jun 28, 2016 at 1:34 PM, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Jun 28, 2016 at 01:30:09PM -0700, Steve Rae wrote: > >> Hi Stefan, > >> > >> On Tue, Jun 28, 2016 at 8:00 AM, Stefan Roese <sr@denx.de> wrote: > >> > Hi Steve, > >> > > >> > On 27.06.2016 23:43, Steve Rae wrote: > >> >> > >> >> Otherwise, ocassionally see errors like this: > >> >> Flashing sparse image at offset 2078720 > >> >> Flashing Sparse Image > >> >> sdhci_send_command: Timeout for status update! > >> >> mmc fail to send stop cmd > >> >> write_sparse_image: Write failed, block #2181088 [0] > >> >> > >> >> This does not affect the actual writing speed, which is controlled by > >> >> the default value: > >> >> CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT > >> >> > >> >> It only increases the retries when reading: > >> >> SDHCI_INT_STATUS > >> >> to avoid the timeout error. > >> >> > >> >> Signed-off-by: Steve Rae <steve.rae@raedomain.com> > >> >> --- > >> >> as per the discussion in: > >> >> http://lists.denx.de/pipermail/u-boot/2016-June/258966.html > >> >> this supercedes: > >> >> http://patchwork.ozlabs.org/patch/615994/ > >> > > >> > > >> > IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig > >> > and use the old value as default value. So that you can overwrite > >> > it for your board / platform via your defconfig. But I have no > >> > strong feelings here - your current version also works for > >> > me and does not "clutter" the Kconfig subsystem with too many > >> > values. So: > >> > > >> > Reviewed-by: Stefan Roese <sr@denx.de> > >> > > >> > Thanks, > >> > Stefan > >> > > >> > >> Thanks for the review... > >> I didn't want to touch the "performance" algorithm related to > >> SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). > >> However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to > >> be in Kconfig -- it is just a define. > > > > ... so how is this handled in the kernel? I'm assuming some DT > > property.. > > > > -- > > Tom > > ( we discussed this in the other email thread; but I'll copy it here... ) I thought I saw this in another thread, thanks. > It looks like (v.4.6) the code loops for max_loops=16, > and it looks like the loop delay is created by a write (which does not > exist in the U-Boot code): > sdhci_writel(host, mask, SDHCI_INT_STATUS); Maybe we should rewrite the area in question, for the next release to be like the kernel?
On Tue, Jun 28, 2016 at 3:21 PM, Tom Rini <trini@konsulko.com> wrote: > On Tue, Jun 28, 2016 at 01:53:52PM -0700, Steve Rae wrote: >> Hi Tom, >> >> On Tue, Jun 28, 2016 at 1:34 PM, Tom Rini <trini@konsulko.com> wrote: >> > On Tue, Jun 28, 2016 at 01:30:09PM -0700, Steve Rae wrote: >> >> Hi Stefan, >> >> >> >> On Tue, Jun 28, 2016 at 8:00 AM, Stefan Roese <sr@denx.de> wrote: >> >> > Hi Steve, >> >> > >> >> > On 27.06.2016 23:43, Steve Rae wrote: >> >> >> >> >> >> Otherwise, ocassionally see errors like this: >> >> >> Flashing sparse image at offset 2078720 >> >> >> Flashing Sparse Image >> >> >> sdhci_send_command: Timeout for status update! >> >> >> mmc fail to send stop cmd >> >> >> write_sparse_image: Write failed, block #2181088 [0] >> >> >> >> >> >> This does not affect the actual writing speed, which is controlled by >> >> >> the default value: >> >> >> CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT >> >> >> >> >> >> It only increases the retries when reading: >> >> >> SDHCI_INT_STATUS >> >> >> to avoid the timeout error. >> >> >> >> >> >> Signed-off-by: Steve Rae <steve.rae@raedomain.com> >> >> >> --- >> >> >> as per the discussion in: >> >> >> http://lists.denx.de/pipermail/u-boot/2016-June/258966.html >> >> >> this supercedes: >> >> >> http://patchwork.ozlabs.org/patch/615994/ >> >> > >> >> > >> >> > IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig >> >> > and use the old value as default value. So that you can overwrite >> >> > it for your board / platform via your defconfig. But I have no >> >> > strong feelings here - your current version also works for >> >> > me and does not "clutter" the Kconfig subsystem with too many >> >> > values. So: >> >> > >> >> > Reviewed-by: Stefan Roese <sr@denx.de> >> >> > >> >> > Thanks, >> >> > Stefan >> >> > >> >> >> >> Thanks for the review... >> >> I didn't want to touch the "performance" algorithm related to >> >> SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). >> >> However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to >> >> be in Kconfig -- it is just a define. >> > >> > ... so how is this handled in the kernel? I'm assuming some DT >> > property.. >> > >> > -- >> > Tom >> >> ( we discussed this in the other email thread; but I'll copy it here... ) > > I thought I saw this in another thread, thanks. > >> It looks like (v.4.6) the code loops for max_loops=16, >> and it looks like the loop delay is created by a write (which does not >> exist in the U-Boot code): >> sdhci_writel(host, mask, SDHCI_INT_STATUS); > > Maybe we should rewrite the area in question, for the next release to be > like the kernel? > Maybe - I don't know enough about it -- it looks like there are interrupts involved.... ( Who could this be assigned to? ) I was just trying to get it to run without hitting the time out.... Thanks, Steve > -- > Tom > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
Hi Steve, > @@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data, > #define CONFIG_SDHCI_CMD_MAX_TIMEOUT 3200 > #endif > #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT 100 > +#define CONFIG_SDHCI_READ_STATUS_TIMEOUT 1000 > > static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd, > struct mmc_data *data) When I see CONFIG_ prefix, I imagine this parameter can be configurable via Kconfig or board header, but this is not actually overridden. Nor do I believe it should be moved to Kconfig, because I doubt it is a CONFIG. Setting such things aside, one important thing is, this patch fixes my problem. BTW, why did you add "arm: " to the git subject? Is this patch related to ARM? Otherwise, Tested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Hi Masahiro, On Wed, Jun 29, 2016 at 4:52 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Steve, > > >> @@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data, >> #define CONFIG_SDHCI_CMD_MAX_TIMEOUT 3200 >> #endif >> #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT 100 >> +#define CONFIG_SDHCI_READ_STATUS_TIMEOUT 1000 >> >> static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd, >> struct mmc_data *data) > > > When I see CONFIG_ prefix, I imagine this parameter can be configurable > via Kconfig or board header, > but this is not actually overridden. > that makes sense.... > Nor do I believe it should be moved to Kconfig, > because I doubt it is a CONFIG. > correct! so maybe the define should be ? : SDHCI_READ_STATUS_TIMEOUT > > Setting such things aside, one important thing is, > this patch fixes my problem. > > > > BTW, why did you add "arm: " to the git subject? > Is this patch related to ARM? > no (sorry) , I don't think so.... > > > Otherwise, > > Tested-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > -- > Best Regards > Masahiro Yamada
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 5c71ab8..aa4cd4f 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data, #define CONFIG_SDHCI_CMD_MAX_TIMEOUT 3200 #endif #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT 100 +#define CONFIG_SDHCI_READ_STATUS_TIMEOUT 1000 static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) @@ -243,9 +244,9 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd, if (stat & SDHCI_INT_ERROR) break; } while (((stat & mask) != mask) && - (get_timer(start) < CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT)); + (get_timer(start) < CONFIG_SDHCI_READ_STATUS_TIMEOUT)); - if (get_timer(start) >= CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT) { + if (get_timer(start) >= CONFIG_SDHCI_READ_STATUS_TIMEOUT) { if (host->quirks & SDHCI_QUIRK_BROKEN_R1B) return 0; else {
Otherwise, ocassionally see errors like this: Flashing sparse image at offset 2078720 Flashing Sparse Image sdhci_send_command: Timeout for status update! mmc fail to send stop cmd write_sparse_image: Write failed, block #2181088 [0] This does not affect the actual writing speed, which is controlled by the default value: CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT It only increases the retries when reading: SDHCI_INT_STATUS to avoid the timeout error. Signed-off-by: Steve Rae <steve.rae@raedomain.com> --- as per the discussion in: http://lists.denx.de/pipermail/u-boot/2016-June/258966.html this supercedes: http://patchwork.ozlabs.org/patch/615994/ drivers/mmc/sdhci.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)