| Message ID | 20190814083819.5784-1-kever.yang@rock-chips.com |
|---|---|
| State | Accepted |
| Delegated to: | Peng Fan |
| Headers | show |
| Series | [U-Boot] mmc: dw_mmc: fix timeout calculate method | expand |
> Subject: [PATCH] mmc: dw_mmc: fix timeout calculate method > > There are two cases not been considered: > - use uint for timeout, it will overflow when size bigger than 512KB for > it *8*1000 at the beginning, but we may use size up to 32MB; > - The timeout is using clock speed for data rate, but the device may not > have such high speed, eg. clock is 52MHz while the device write speed may > be less than 10MB/s. > > Fix them in this patch, the max timeout is about 6500 when size is 32MB after > fix. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> Applied to mmc/master. Thanks, Peng.
Hi Kever, > -----Original Message----- > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Peng Fan > Sent: 2019年8月27日 15:40 > To: Kever Yang <kever.yang@rock-chips.com>; u-boot@lists.denx.de > Cc: trini@konsulko.com > Subject: Re: [U-Boot] [PATCH] mmc: dw_mmc: fix timeout calculate method > > > Subject: [PATCH] mmc: dw_mmc: fix timeout calculate method > > > > There are two cases not been considered: > > - use uint for timeout, it will overflow when size bigger than 512KB for > > it *8*1000 at the beginning, but we may use size up to 32MB; > > - The timeout is using clock speed for data rate, but the device may not > > have such high speed, eg. clock is 52MHz while the device write speed > may > > be less than 10MB/s. > > > > Fix them in this patch, the max timeout is about 6500 when size is > > 32MB after fix. > > > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> This patch breaks build, I need drop it. See build: https://travis-ci.org/MrVan/u-boot/jobs/577210464 Regards, Peng. > > Applied to mmc/master. > > Thanks, > Peng. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.d > enx.de%2Flistinfo%2Fu-boot&data=02%7C01%7CPeng.Fan%40nxp.com > %7Cec929026da5f42951b6108d72ac1c446%7C686ea1d3bc2b4c6fa92cd99c > 5c301635%7C0%7C0%7C637024884027193189&sdata=SPBna8IBqgpo9 > %2F6ab1YO5sbpX6NGgkCowt0oLclRHEk%3D&reserved=0
Add ARC maintainers. On 2019/8/27 下午5:58, Peng Fan wrote: > Hi Kever, > >> -----Original Message----- >> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Peng Fan >> Sent: 2019年8月27日 15:40 >> To: Kever Yang <kever.yang@rock-chips.com>; u-boot@lists.denx.de >> Cc: trini@konsulko.com >> Subject: Re: [U-Boot] [PATCH] mmc: dw_mmc: fix timeout calculate method >> >>> Subject: [PATCH] mmc: dw_mmc: fix timeout calculate method >>> >>> There are two cases not been considered: >>> - use uint for timeout, it will overflow when size bigger than 512KB for >>> it *8*1000 at the beginning, but we may use size up to 32MB; >>> - The timeout is using clock speed for data rate, but the device may not >>> have such high speed, eg. clock is 52MHz while the device write speed >> may >>> be less than 10MB/s. >>> >>> Fix them in this patch, the max timeout is about 6500 when size is >>> 32MB after fix. >>> >>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > This patch breaks build, I need drop it. > See build: > https://travis-ci.org/MrVan/u-boot/jobs/577210464 The error happens only in ARC architecture, and seems like the arc compiler not support 64bit div? Any idea on how to fix this? Thanks, - Kever > > Regards, > Peng. > >> Applied to mmc/master. >> >> Thanks, >> Peng. >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.d >> enx.de%2Flistinfo%2Fu-boot&data=02%7C01%7CPeng.Fan%40nxp.com >> %7Cec929026da5f42951b6108d72ac1c446%7C686ea1d3bc2b4c6fa92cd99c >> 5c301635%7C0%7C0%7C637024884027193189&sdata=SPBna8IBqgpo9 >> %2F6ab1YO5sbpX6NGgkCowt0oLclRHEk%3D&reserved=0
Hi Kever, > -----Original Message----- > From: Kever Yang <kever.yang@rock-chips.com> > Sent: Thursday, August 29, 2019 10:17 AM > To: Peng Fan <peng.fan@nxp.com>; u-boot@lists.denx.de > Cc: trini@konsulko.com; Eugeniy.Paltsev@synopsys.com; Alexey Brodkin <abrodkin@synopsys.com>; Simon > Glass <sjg@chromium.org> > Subject: Re: [PATCH] mmc: dw_mmc: fix timeout calculate method > > Add ARC maintainers. Thanks for adding us in the loop. > On 2019/8/27 下午5:58, Peng Fan wrote: > > Hi Kever, > > > >> -----Original Message----- > >> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Peng Fan > >> Sent: 2019年8月27日 15:40 > >> To: Kever Yang <kever.yang@rock-chips.com>; u-boot@lists.denx.de > >> Cc: trini@konsulko.com > >> Subject: Re: [U-Boot] [PATCH] mmc: dw_mmc: fix timeout calculate method > >> > >>> Subject: [PATCH] mmc: dw_mmc: fix timeout calculate method > >>> > >>> There are two cases not been considered: > >>> - use uint for timeout, it will overflow when size bigger than 512KB for > >>> it *8*1000 at the beginning, but we may use size up to 32MB; > >>> - The timeout is using clock speed for data rate, but the device may not > >>> have such high speed, eg. clock is 52MHz while the device write speed > >> may > >>> be less than 10MB/s. > >>> > >>> Fix them in this patch, the max timeout is about 6500 when size is > >>> 32MB after fix. > >>> > >>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > > This patch breaks build, I need drop it. > > See build: > > https://travis-ci.org/MrVan/u-boot/jobs/577210464 > > The error happens only in ARC architecture, and seems like the arc > compiler not support 64bit div? It's not a compiler problem but a subtle issue with libgcc. To be independent from toolchain type (and we have quite a few flavors like little- and big-endian, ARCv1 or ARCv2 etc) decision was made to move libgcc into U-Boot but given full libgcc is quite large only primitives required by up-to-date U-Boot were copied from GCC to U-Boot. > Any idea on how to fix this? Add missing primitives like __udivdi3() and their dependencies, see for example: https://github.com/gcc-mirror/gcc/blob/master/libgcc/libgcc2.c#L1318. And I'll happily do it myself but for some reason I cannot reproduce that problem locally. I use the same pre-built arc-2018.09 toolchain: ---------------------------->8----------------------- # arc-linux-gcc -v gcc version 8.2.1 20180814 (ARCv2 ISA Linux uClibc toolchain 2018.09) ---------------------------->8----------------------- And may successfully build U-Boot for HSDK from https://github.com/MrVan/u-boot/commits/mmc-8-28-2019: ---------------------------->8----------------------- # git l 76c0bf2873 - (HEAD -> mrvan, github-mrvan/mmc-8-28-2019) ARM: dts: ast2500: Add SDHCI nodes (27 hours ago) <Eddie James> 07b000cd32 - configs: AST2500 EVB: Enable SD controller (27 hours ago) <Eddie James> 5f6a033f10 - mmc: Add Aspeed SD controller driver (27 hours ago) <Eddie James> a6503eaa66 - clk: aspeed: Add support for SD clock (27 hours ago) <Eddie James> c54e328452 - mmc: Rename timeout parameters for clarification (27 hours ago) <Sam Protsenko> be949bb19c - mmc: Fix timeout values passed to mmc_wait_dat0() (27 hours ago) <Sam Protsenko> 49842f0edf - dm: mmc: remove unused U_BOOT_DRIVER(mmc) (27 hours ago) <Andy Yan> 7b91b47f8f - Prepare v2019.10-rc3 (27 hours ago) <Tom Rini> ---------------------------->8----------------------- That way: ---------------------------->8----------------------- ./tools/buildman/buildman hsdk boards.cfg is up to date. Nothing to do. Building current source for 1 boards (1 thread, 48 jobs per thread) arc: w+ hsdk 0 1 0 /1 hsdk ---------------------------->8----------------------- What do I do wrong? -Alexey
Hi Alexey, On 2019/8/29 下午5:21, Alexey Brodkin wrote: > Hi Kever, > >> -----Original Message----- >> From: Kever Yang <kever.yang@rock-chips.com> >> Sent: Thursday, August 29, 2019 10:17 AM >> To: Peng Fan <peng.fan@nxp.com>; u-boot@lists.denx.de >> Cc: trini@konsulko.com; Eugeniy.Paltsev@synopsys.com; Alexey Brodkin <abrodkin@synopsys.com>; Simon >> Glass <sjg@chromium.org> >> Subject: Re: [PATCH] mmc: dw_mmc: fix timeout calculate method >> >> Add ARC maintainers. > Thanks for adding us in the loop. > >> On 2019/8/27 下午5:58, Peng Fan wrote: >>> Hi Kever, >>> >>>> -----Original Message----- >>>> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Peng Fan >>>> Sent: 2019年8月27日 15:40 >>>> To: Kever Yang <kever.yang@rock-chips.com>; u-boot@lists.denx.de >>>> Cc: trini@konsulko.com >>>> Subject: Re: [U-Boot] [PATCH] mmc: dw_mmc: fix timeout calculate method >>>> >>>>> Subject: [PATCH] mmc: dw_mmc: fix timeout calculate method >>>>> >>>>> There are two cases not been considered: >>>>> - use uint for timeout, it will overflow when size bigger than 512KB for >>>>> it *8*1000 at the beginning, but we may use size up to 32MB; >>>>> - The timeout is using clock speed for data rate, but the device may not >>>>> have such high speed, eg. clock is 52MHz while the device write speed >>>> may >>>>> be less than 10MB/s. >>>>> >>>>> Fix them in this patch, the max timeout is about 6500 when size is >>>>> 32MB after fix. >>>>> >>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >>> This patch breaks build, I need drop it. >>> See build: >>> https://travis-ci.org/MrVan/u-boot/jobs/577210464 >> The error happens only in ARC architecture, and seems like the arc >> compiler not support 64bit div? > It's not a compiler problem but a subtle issue with libgcc. > To be independent from toolchain type (and we have quite a few flavors like > little- and big-endian, ARCv1 or ARCv2 etc) decision was made to move > libgcc into U-Boot but given full libgcc is quite large only primitives required > by up-to-date U-Boot were copied from GCC to U-Boot. > >> Any idea on how to fix this? > Add missing primitives like __udivdi3() and their dependencies, see for example: > https://github.com/gcc-mirror/gcc/blob/master/libgcc/libgcc2.c#L1318. > > And I'll happily do it myself but for some reason I cannot reproduce that problem locally. > I use the same pre-built arc-2018.09 toolchain: > ---------------------------->8----------------------- > # arc-linux-gcc -v > gcc version 8.2.1 20180814 (ARCv2 ISA Linux uClibc toolchain 2018.09) > ---------------------------->8----------------------- > > And may successfully build U-Boot for HSDK from > https://github.com/MrVan/u-boot/commits/mmc-8-28-2019: > ---------------------------->8----------------------- > # git l > 76c0bf2873 - (HEAD -> mrvan, github-mrvan/mmc-8-28-2019) ARM: dts: ast2500: Add SDHCI nodes (27 hours ago) <Eddie James> > 07b000cd32 - configs: AST2500 EVB: Enable SD controller (27 hours ago) <Eddie James> > 5f6a033f10 - mmc: Add Aspeed SD controller driver (27 hours ago) <Eddie James> > a6503eaa66 - clk: aspeed: Add support for SD clock (27 hours ago) <Eddie James> > c54e328452 - mmc: Rename timeout parameters for clarification (27 hours ago) <Sam Protsenko> > be949bb19c - mmc: Fix timeout values passed to mmc_wait_dat0() (27 hours ago) <Sam Protsenko> > 49842f0edf - dm: mmc: remove unused U_BOOT_DRIVER(mmc) (27 hours ago) <Andy Yan> > 7b91b47f8f - Prepare v2019.10-rc3 (27 hours ago) <Tom Rini> > ---------------------------->8----------------------- I think this tree does not including this patch, Peng drop it because of this issue, so you need to apply this patch in your branch to reproduce the problem. I have send out V2 patch for this fix with only using 32bit variable instead of 64bit. Thanks, - Kever > > That way: > ---------------------------->8----------------------- > ./tools/buildman/buildman hsdk > boards.cfg is up to date. Nothing to do. > Building current source for 1 boards (1 thread, 48 jobs per thread) > arc: w+ hsdk > 0 1 0 /1 hsdk > ---------------------------->8----------------------- > > What do I do wrong? > > -Alexey > >
Hi Kever, [snip] > I think this tree does not including this patch, Peng drop it because of > this issue, > so you need to apply this patch in your branch to reproduce the problem. > I have send out V2 patch for this fix with only using 32bit variable Could you please refer me to the problematic patch so I may try it? -Alexey
Hi Alexey, On 2019/8/30 下午9:28, Alexey Brodkin wrote: > Hi Kever, > > [snip] > >> I think this tree does not including this patch, Peng drop it because of >> this issue, >> so you need to apply this patch in your branch to reproduce the problem. >> I have send out V2 patch for this fix with only using 32bit variable > Could you please refer me to the problematic patch so I may try it? This is the patch with problem, and here is the link on patchwork: https://patchwork.ozlabs.org/patch/1146845/ Thanks, - Kever > > -Alexey
Hi Kever, > -----Original Message----- > From: Kever Yang <kever.yang@rock-chips.com> > Sent: Monday, September 2, 2019 11:05 AM > To: Alexey Brodkin <abrodkin@synopsys.com> > Cc: trini@konsulko.com; Eugeniy.Paltsev@synopsys.com; Simon Glass <sjg@chromium.org>; Peng Fan > <peng.fan@nxp.com>; u-boot@lists.denx.de > Subject: Re: [PATCH] mmc: dw_mmc: fix timeout calculate method > > Hi Alexey, > > On 2019/8/30 下午9:28, Alexey Brodkin wrote: > > Hi Kever, > > > > [snip] > > > >> I think this tree does not including this patch, Peng drop it because of > >> this issue, > >> so you need to apply this patch in your branch to reproduce the problem. > >> I have send out V2 patch for this fix with only using 32bit variable > > Could you please refer me to the problematic patch so I may try it? > > This is the patch with problem, and here is the link on patchwork: > https://patchwork.ozlabs.org/patch/1146845/ Please find my fixes here: https://patchwork.ozlabs.org/patch/1156541/ https://patchwork.ozlabs.org/patch/1156617/ Tom do we want https://patchwork.ozlabs.org/patch/1146845/ and fixes for it (see 2 items above) to become a part of upcoming v2019.10 release or it will be slated for the next one? -Alexey
On Mon, Sep 02, 2019 at 12:24:42PM +0000, Alexey Brodkin wrote: > Hi Kever, > > > -----Original Message----- > > From: Kever Yang <kever.yang@rock-chips.com> > > Sent: Monday, September 2, 2019 11:05 AM > > To: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: trini@konsulko.com; Eugeniy.Paltsev@synopsys.com; Simon Glass <sjg@chromium.org>; Peng Fan > > <peng.fan@nxp.com>; u-boot@lists.denx.de > > Subject: Re: [PATCH] mmc: dw_mmc: fix timeout calculate method > > > > Hi Alexey, > > > > On 2019/8/30 下午9:28, Alexey Brodkin wrote: > > > Hi Kever, > > > > > > [snip] > > > > > >> I think this tree does not including this patch, Peng drop it because of > > >> this issue, > > >> so you need to apply this patch in your branch to reproduce the problem. > > >> I have send out V2 patch for this fix with only using 32bit variable > > > Could you please refer me to the problematic patch so I may try it? > > > > This is the patch with problem, and here is the link on patchwork: > > https://patchwork.ozlabs.org/patch/1146845/ > > Please find my fixes here: > https://patchwork.ozlabs.org/patch/1156541/ > https://patchwork.ozlabs.org/patch/1156617/ > > Tom do we want https://patchwork.ozlabs.org/patch/1146845/ and fixes for it > (see 2 items above) to become a part of upcoming v2019.10 release or > it will be slated for the next one? I think we should aim to get all the fixes in for this release.
H Tom, [snip] > > > This is the patch with problem, and here is the link on patchwork: > > > https://patchwork.ozlabs.org/patch/1146845/ > > > > Please find my fixes here: > > https://patchwork.ozlabs.org/patch/1156541/ > > https://patchwork.ozlabs.org/patch/1156617/ > > > > Tom do we want https://patchwork.ozlabs.org/patch/1146845/ and fixes for it > > (see 2 items above) to become a part of upcoming v2019.10 release or > > it will be slated for the next one? > > I think we should aim to get all the fixes in for this release. Done, see https://lists.denx.de/pipermail/u-boot/2019-September/382628.html -Alexey
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 22f6c7eefd..2cb61ba184 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -117,10 +117,10 @@ static int dwmci_fifo_ready(struct dwmci_host *host, u32 bit, u32 *len) static unsigned int dwmci_get_timeout(struct mmc *mmc, const unsigned int size) { - unsigned int timeout; + u64 timeout; - timeout = size * 8 * 1000; /* counting in bits and msec */ - timeout *= 2; /* wait twice as long */ + timeout = (u64)size * 8 * 1000; /* counting in bits and msec */ + timeout *= 10; /* wait 10 times as long */ timeout /= mmc->clock; timeout /= mmc->bus_width; timeout /= mmc->ddr_mode ? 2 : 1;
There are two cases not been considered: - use uint for timeout, it will overflow when size bigger than 512KB for it *8*1000 at the beginning, but we may use size up to 32MB; - The timeout is using clock speed for data rate, but the device may not have such high speed, eg. clock is 52MHz while the device write speed may be less than 10MB/s. Fix them in this patch, the max timeout is about 6500 when size is 32MB after fix. Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- drivers/mmc/dw_mmc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)