| Message ID | 1902435786.2282282.1344626175261.JavaMail.root@advansee.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Friday 10 August 2012 15:16:15 Benoît Thébaudeau wrote: > mtest is supposed to test many types of memory accesses in many different > conditions. If dcache is enabled, memory accesses are likely bursts, and > some memory accesses are simply skipped. Hence, dcache should be disabled > during mtest operation so that what mtest actually tests is not masked by > dcache. if you want dcache disabled, then why don't you run `dcache off` first ? i think it's useful to be able to do both, and forcing it one way is wrong. thus, NAK from me. -mike
Hi Mike, On 08/11/2012 05:18 AM, Mike Frysinger wrote: > On Friday 10 August 2012 15:16:15 Benoît Thébaudeau wrote: > > mtest is supposed to test many types of memory accesses in many > > different > > conditions. If dcache is enabled, memory accesses are likely > > bursts, and > > some memory accesses are simply skipped. Hence, dcache should be > > disabled > > during mtest operation so that what mtest actually tests is not > > masked by > > dcache. > > if you want dcache disabled, then why don't you run `dcache off` > first ? i > think it's useful to be able to do both, and forcing it one way is > wrong. > > thus, NAK from me. > -mike Because you will very likely trust mtest and forget about running `dcache off` first, so you may then be happy about falsely positive mtest results. Moreover, I can't find any sense or usefulness in running mtest with dcache enabled. If there is really a need for running mtest with dcache enabled, an option (disabled by default) could be added to the command line to tell mtest not to disable dcache. Best regards, Benoît
On Saturday 11 August 2012 10:17:04 Benoît Thébaudeau wrote: > On 08/11/2012 05:18 AM, Mike Frysinger wrote: > > On Friday 10 August 2012 15:16:15 Benoît Thébaudeau wrote: > > > mtest is supposed to test many types of memory accesses in many > > > different > > > conditions. If dcache is enabled, memory accesses are likely > > > bursts, and > > > some memory accesses are simply skipped. Hence, dcache should be > > > disabled > > > during mtest operation so that what mtest actually tests is not > > > masked by > > > dcache. > > > > if you want dcache disabled, then why don't you run `dcache off` > > first ? i > > think it's useful to be able to do both, and forcing it one way is > > wrong. > > > > thus, NAK from me. > > Because you will very likely trust mtest and forget about running `dcache > off` first, so you may then be happy about falsely positive mtest results. there are a lot of commands in u-boot that people could "easily forget" to tweak and have it do the wrong thing. imo, mtest running in the active cache/memory settings is the *expected* behavior rather than having it silently reconfigure the system behind the back of the user. > Moreover, I can't find any sense or usefulness in running mtest with > dcache enabled. it can be useful to stress test the system with cache enabled. i recall during some early porting work that being able to test both caught bugs that we might have missed otherwise. > If there is really a need for running mtest with dcache enabled, an option > (disabled by default) could be added to the command line to tell mtest not > to disable dcache. we already have such an option: dcache off; mtest; dcache on -mike
Hi Mike, On 08/11/2012 19:50, Mike Frysinger wrote: > On Saturday 11 August 2012 10:17:04 Benoît Thébaudeau wrote: > > On 08/11/2012 05:18 AM, Mike Frysinger wrote: > > > On Friday 10 August 2012 15:16:15 Benoît Thébaudeau wrote: > > > > mtest is supposed to test many types of memory accesses in many > > > > different > > > > conditions. If dcache is enabled, memory accesses are likely > > > > bursts, and > > > > some memory accesses are simply skipped. Hence, dcache should > > > > be > > > > disabled > > > > during mtest operation so that what mtest actually tests is not > > > > masked by > > > > dcache. > > > > > > if you want dcache disabled, then why don't you run `dcache off` > > > first ? i > > > think it's useful to be able to do both, and forcing it one way > > > is > > > wrong. > > > > > > thus, NAK from me. > > > > Because you will very likely trust mtest and forget about running > > `dcache > > off` first, so you may then be happy about falsely positive mtest > > results. > > there are a lot of commands in u-boot that people could "easily > forget" to > tweak and have it do the wrong thing. imo, mtest running in the > active > cache/memory settings is the *expected* behavior rather than having > it > silently reconfigure the system behind the back of the user. IMO, mtest is supposed to test the underlying memory as it advertises, not the dcache system, so this is what people will expect from it, and this should be the default behavior. If dcache is needed in some cases, it should be an explicit choice, not the default one. People should not have to think about dcache being supported or enabled on their platform when running mtest. If dcache is added as a command line option to mtest, it will also appear in its help message, which will be a reminder, so there will be nothing made really silently. On the contrary, if dcache is left enabled by default, this will be completely unknown from the user, and there will not even be a reminder that disabling dcache manually should be considered. > > Moreover, I can't find any sense or usefulness in running mtest > > with > > dcache enabled. > > it can be useful to stress test the system with cache enabled. i > recall > during some early porting work that being able to test both caught > bugs that > we might have missed otherwise. OK, but this is not the main purpose of mtest. Here, you used it as a system stress test tool, not really as a memory stress test tool. So I agree that leaving dcache enabled should be an option, but not the default behavior. > > If there is really a need for running mtest with dcache enabled, an > > option > > (disabled by default) could be added to the command line to tell > > mtest not > > to disable dcache. > > we already have such an option: dcache off; mtest; dcache on > -mike See my comment above. IMO, dcache off should be the default for mtest. If you really don't want that, the least should be to have a big warning printed both in mtest help message and when running mtest with dcache enabled. Best regards, Benoît
Dear Benoît Thébaudeau, In message <1725235724.2300239.1344694624384.JavaMail.root@advansee.com> you wrote: > > On 08/11/2012 05:18 AM, Mike Frysinger wrote: ... > > if you want dcache disabled, then why don't you run `dcache off` > > first ? i > > think it's useful to be able to do both, and forcing it one way is > > wrong. > > > > thus, NAK from me. > > -mike > > Because you will very likely trust mtest and forget about running `dcache off` > first, so you may then be happy about falsely positive mtest results. Moreover, > I can't find any sense or usefulness in running mtest with dcache enabled. I agree with Mike. "UNIX was not designed to stop you from doing stupid things, because that would also stop you from doing clever things." - Doug Gwyn So NAK from me, too. Best regards, Wolfgang Denk
Dear Wolfgang Denk, On Sunday, September 2, 2012 6:30:23 PM, Wolfgang Denk wrote: > Dear Benoît Thébaudeau, > > In message > <1725235724.2300239.1344694624384.JavaMail.root@advansee.com> you > wrote: > > > > On 08/11/2012 05:18 AM, Mike Frysinger wrote: > ... > > > if you want dcache disabled, then why don't you run `dcache off` > > > first ? i > > > think it's useful to be able to do both, and forcing it one way > > > is > > > wrong. > > > > > > thus, NAK from me. > > > -mike > > > > Because you will very likely trust mtest and forget about running > > `dcache off` > > first, so you may then be happy about falsely positive mtest > > results. Moreover, > > I can't find any sense or usefulness in running mtest with dcache > > enabled. > > I agree with Mike. > > "UNIX was not designed to stop you from doing stupid things, because > that would also stop you from doing clever things." - Doug Gwyn > > So NAK from me, too. OK, but do you agree with the following that Mike and me agreed on after that? http://lists.denx.de/pipermail/u-boot/2012-August/130650.html http://lists.denx.de/pipermail/u-boot/2012-August/130726.html http://patchwork.ozlabs.org/patch/176909/ Best regards, Benoît
Hi Benoît, On Mon, 3 Sep 2012 16:25:15 +0200 (CEST), Benoît Thébaudeau <benoit.thebaudeau@advansee.com> wrote: > Dear Wolfgang Denk, > > On Sunday, September 2, 2012 6:30:23 PM, Wolfgang Denk wrote: > > Dear Benoît Thébaudeau, > > > > In message > > <1725235724.2300239.1344694624384.JavaMail.root@advansee.com> you > > wrote: > > > > > > On 08/11/2012 05:18 AM, Mike Frysinger wrote: > > ... > > > > if you want dcache disabled, then why don't you run `dcache off` > > > > first ? i > > > > think it's useful to be able to do both, and forcing it one way > > > > is > > > > wrong. > > > > > > > > thus, NAK from me. > > > > -mike > > > > > > Because you will very likely trust mtest and forget about running > > > `dcache off` > > > first, so you may then be happy about falsely positive mtest > > > results. Moreover, > > > I can't find any sense or usefulness in running mtest with dcache > > > enabled. > > > > I agree with Mike. > > > > "UNIX was not designed to stop you from doing stupid things, > > because that would also stop you from doing clever things." - > > Doug Gwyn > > > > So NAK from me, too. > > OK, but do you agree with the following that Mike and me agreed on > after that? > http://lists.denx.de/pipermail/u-boot/2012-August/130650.html > http://lists.denx.de/pipermail/u-boot/2012-August/130726.html > http://patchwork.ozlabs.org/patch/176909/ I did already reply to this, but since the agreement is brought back, I think I should re-state my opinion: such a warning line will most likely be overlooked, thus has little value. People using mtest should know that they must check/set dcache state before running mtest. > Best regards, > Benoît Amicalement,
Hi Albert, On Monday, September 3, 2012 6:50:14 PM, Albert ARIBAUD wrote: > Hi Benoît, > > On Mon, 3 Sep 2012 16:25:15 +0200 (CEST), Benoît Thébaudeau > <benoit.thebaudeau@advansee.com> wrote: > > > Dear Wolfgang Denk, > > > > On Sunday, September 2, 2012 6:30:23 PM, Wolfgang Denk wrote: > > > Dear Benoît Thébaudeau, > > > > > > In message > > > <1725235724.2300239.1344694624384.JavaMail.root@advansee.com> you > > > wrote: > > > > > > > > On 08/11/2012 05:18 AM, Mike Frysinger wrote: > > > ... > > > > > if you want dcache disabled, then why don't you run `dcache > > > > > off` > > > > > first ? i > > > > > think it's useful to be able to do both, and forcing it one > > > > > way > > > > > is > > > > > wrong. > > > > > > > > > > thus, NAK from me. > > > > > -mike > > > > > > > > Because you will very likely trust mtest and forget about > > > > running > > > > `dcache off` > > > > first, so you may then be happy about falsely positive mtest > > > > results. Moreover, > > > > I can't find any sense or usefulness in running mtest with > > > > dcache > > > > enabled. > > > > > > I agree with Mike. > > > > > > "UNIX was not designed to stop you from doing stupid things, > > > because that would also stop you from doing clever things." > > > - > > > Doug Gwyn > > > > > > So NAK from me, too. > > > > OK, but do you agree with the following that Mike and me agreed on > > after that? > > http://lists.denx.de/pipermail/u-boot/2012-August/130650.html > > http://lists.denx.de/pipermail/u-boot/2012-August/130726.html > > http://patchwork.ozlabs.org/patch/176909/ > > I did already reply to this, but since the agreement is brought back, > I > think I should re-state my opinion: such a warning line will most > likely > be overlooked, thus has little value. People using mtest should know > that they must check/set dcache state before running mtest. If this line is overlooked, it's the same as not having it. If it is not overlooked, it is useful both to detail the test conditions and as a reminder not to do stupid things. This line does not prevent users from doing any manual dcache check/enable/disable operation they want before running mtest. All in all, adding this line can only be beneficial. Best regards, Benoît
Hi Benoît, On Tue, Sep 4, 2012 at 7:14 AM, Benoît Thébaudeau <benoit.thebaudeau@advansee.com> wrote: > Hi Albert, > > On Monday, September 3, 2012 6:50:14 PM, Albert ARIBAUD wrote: >> Hi Benoît, >> >> On Mon, 3 Sep 2012 16:25:15 +0200 (CEST), Benoît Thébaudeau >> <benoit.thebaudeau@advansee.com> wrote: >> >> > Dear Wolfgang Denk, >> > >> > On Sunday, September 2, 2012 6:30:23 PM, Wolfgang Denk wrote: >> > > Dear Benoît Thébaudeau, >> > > >> > > In message >> > > <1725235724.2300239.1344694624384.JavaMail.root@advansee.com> you >> > > wrote: >> > > > >> > > > On 08/11/2012 05:18 AM, Mike Frysinger wrote: >> > > ... >> > > > > if you want dcache disabled, then why don't you run `dcache >> > > > > off` >> > > > > first ? i >> > > > > think it's useful to be able to do both, and forcing it one >> > > > > way >> > > > > is >> > > > > wrong. >> > > > > >> > > > > thus, NAK from me. >> > > > > -mike >> > > > >> > > > Because you will very likely trust mtest and forget about >> > > > running >> > > > `dcache off` >> > > > first, so you may then be happy about falsely positive mtest >> > > > results. Moreover, >> > > > I can't find any sense or usefulness in running mtest with >> > > > dcache >> > > > enabled. >> > > >> > > I agree with Mike. >> > > >> > > "UNIX was not designed to stop you from doing stupid things, >> > > because that would also stop you from doing clever things." >> > > - >> > > Doug Gwyn >> > > >> > > So NAK from me, too. >> > >> > OK, but do you agree with the following that Mike and me agreed on >> > after that? >> > http://lists.denx.de/pipermail/u-boot/2012-August/130650.html >> > http://lists.denx.de/pipermail/u-boot/2012-August/130726.html >> > http://patchwork.ozlabs.org/patch/176909/ >> >> I did already reply to this, but since the agreement is brought back, >> I >> think I should re-state my opinion: such a warning line will most >> likely >> be overlooked, thus has little value. People using mtest should know >> that they must check/set dcache state before running mtest. > > If this line is overlooked, it's the same as not having it. If it is not > overlooked, it is useful both to detail the test conditions and as a reminder > not to do stupid things. This line does not prevent users from doing any manual > dcache check/enable/disable operation they want before running mtest. All in > all, adding this line can only be beneficial. I agree - particularly when somebody else is looking at the output (on the ML for example) and notices it. I'm also inclined to not 100% trust dcache operations (we all know that cache support is in a state of flux) so something like: # dcache on # mtest 0x80100000 0x90000000 0xaabb If you get back: Testing 0x80100000 ... 0x90000000 (dcache: off): You know something is wrong with the dcache on implementation Regards, Graeme
Dear Graeme Russ, In message <CALButCLVW0Vv9vc+MZ0wpau2xw7FPv+ZqpD7MZpxn=K_400HVA@mail.gmail.com> you wrote: > > > If this line is overlooked, it's the same as not having it. If it is not > > overlooked, it is useful both to detail the test conditions and as a reminder > > not to do stupid things. This line does not prevent users from doing any manual > > dcache check/enable/disable operation they want before running mtest. All in > > all, adding this line can only be beneficial. I disagree. It add line noise, printing information that is available otherwise as well. If you are interested in the state of the data cache support, just run the "dcache" command. > I'm also inclined to not 100% trust dcache operations (we all know > that cache support is in a state of flux) so something like: > > # dcache on > # mtest 0x80100000 0x90000000 0xaabb ... > You know something is wrong with the dcache on implementation Well, it appears awkward to me to use a completely unrelated command for such "testing". The same result should be available (much more straightforward) with: # dcache on # dcache Best regards, Wolfgang Denk
Hi Wolfgang, On Tue, Sep 4, 2012 at 4:41 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Graeme Russ, > > Well, it appears awkward to me to use a completely unrelated command > for such "testing". The same result should be available (much more > straightforward) with: > > # dcache on > # dcache Good point :) Regards, Graeme
Dear Benoît Thébaudeau, In message <356588499.3415554.1346682315106.JavaMail.root@advansee.com> you wrote: > > > So NAK from me, too. > > OK, but do you agree with the following that Mike and me agreed on after that? > http://lists.denx.de/pipermail/u-boot/2012-August/130650.html > http://lists.denx.de/pipermail/u-boot/2012-August/130726.html > http://patchwork.ozlabs.org/patch/176909/ No, I don't. If you anybody is interested in the state of the cache support, he can use the "dcache" (and/or "icache") command(s). Best regards, Wolfgang Denk
diff --git u-boot-4d3c95f.orig/common/cmd_mem.c u-boot-4d3c95f/common/cmd_mem.c index 18f0a3f..5d2b735 100644 --- u-boot-4d3c95f.orig/common/cmd_mem.c +++ u-boot-4d3c95f/common/cmd_mem.c @@ -600,6 +600,8 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) ulong errs = 0; int iterations = 1; int iteration_limit; + int dcache; + int ret = 1; #if defined(CONFIG_SYS_ALT_MEMTEST) vu_long len; @@ -651,6 +653,13 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) else iteration_limit = 0; + /* Perform tests on the underlying memory rather than on the D-cache. */ + dcache = dcache_status(); + if (dcache) { + dcache_disable(); + invalidate_dcache_all(); + } + #if defined(CONFIG_SYS_ALT_MEMTEST) printf ("Testing %08x ... %08x:\n", (uint)start, (uint)end); debug("%s:%d: start 0x%p end 0x%p\n", @@ -659,14 +668,15 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) for (;;) { if (ctrlc()) { putc ('\n'); - return 1; + goto end; } if (iteration_limit && iterations > iteration_limit) { printf("Tested %d iteration(s) with %lu errors.\n", iterations-1, errs); - return errs != 0; + ret = errs != 0; + goto end; } printf("Iteration: %6d\r", iterations); @@ -704,7 +714,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) errs++; if (ctrlc()) { putc ('\n'); - return 1; + goto end; } } *addr = ~val; @@ -717,7 +727,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) errs++; if (ctrlc()) { putc ('\n'); - return 1; + goto end; } } } @@ -787,7 +797,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) errs++; if (ctrlc()) { putc ('\n'); - return 1; + goto end; } } } @@ -809,7 +819,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) errs++; if (ctrlc()) { putc ('\n'); - return 1; + goto end; } } } @@ -851,7 +861,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) errs++; if (ctrlc()) { putc ('\n'); - return 1; + goto end; } } @@ -873,7 +883,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) errs++; if (ctrlc()) { putc ('\n'); - return 1; + goto end; } } start[offset] = 0; @@ -885,13 +895,14 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) for (;;) { if (ctrlc()) { putc ('\n'); - return 1; + goto end; } if (iteration_limit && iterations > iteration_limit) { printf("Tested %d iteration(s) with %lu errors.\n", iterations-1, errs); - return errs != 0; + ret = errs != 0; + goto end; } ++iterations; @@ -918,7 +929,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) errs++; if (ctrlc()) { putc ('\n'); - return 1; + goto end; } } val += incr; @@ -939,7 +950,11 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) incr = -incr; } #endif - return 0; /* not reached */ + +end: + if (dcache) + dcache_enable(); + return ret; }
mtest is supposed to test many types of memory accesses in many different conditions. If dcache is enabled, memory accesses are likely bursts, and some memory accesses are simply skipped. Hence, dcache should be disabled during mtest operation so that what mtest actually tests is not masked by dcache. Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> Cc: Wolfgang Denk <wd@denx.de> --- .../common/cmd_mem.c | 39 ++++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-)