[U-Boot] [PATCH] cmd_boot: cleanup for 'go' command
From: Kuo-Jung Su dantesu@faraday-tech.com
With MMU/D-Cache enabled, data might be retained at cache rather than at DRAM when we execute 'go' command, and some of the bare-metal softwares would always invalidate the entire data cache at start-up, and causes data lost issue.
This patch is designed to fixed this issue. It has been verified at ARM based systems, and should also work at other platforms.
Signed-off-by: Kuo-Jung Su dantesu@faraday-tech.com --- common/cmd_boot.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/common/cmd_boot.c b/common/cmd_boot.c index d3836fd..1a1a532 100644 --- a/common/cmd_boot.c +++ b/common/cmd_boot.c @@ -50,6 +50,16 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
printf ("## Starting application at 0x%08lX ...\n", addr);
+#ifdef CONFIG_USE_IRQ + disable_interrupts(); +#endif +#ifndef CONFIG_SYS_DCACHE_OFF + flush_dcache_all(); +#endif +#ifndef CONFIG_SYS_ICACHE_OFF + invalidate_icache_all(); +#endif + /* * pass address parameter as argv[0] (aka command name), * and all remaining args
Dear Kuo-Jung Su,
In message 1366963312-2901-1-git-send-email-dantesu@gmail.com you wrote:
From: Kuo-Jung Su dantesu@faraday-tech.com
With MMU/D-Cache enabled, data might be retained at cache rather than at DRAM when we execute 'go' command, and some of the bare-metal softwares would always invalidate the entire data cache at start-up, and causes data lost issue.
This patch is designed to fixed this issue. It has been verified at ARM based systems, and should also work at other platforms.
Your patch desription does not match the actual code.
--- a/common/cmd_boot.c +++ b/common/cmd_boot.c @@ -50,6 +50,16 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
printf ("## Starting application at 0x%08lX ...\n", addr);
+#ifdef CONFIG_USE_IRQ
- disable_interrupts();
+#endif
You do not mention in the commit message that you disable interrupts. This must not be done. There are many cases where interrupts are actually needed by the running code.
NAK for this part.
Best regards,
Wolfgang Denk
2013/4/26 Wolfgang Denk wd@denx.de:
Dear Kuo-Jung Su,
In message 1366963312-2901-1-git-send-email-dantesu@gmail.com you wrote:
From: Kuo-Jung Su dantesu@faraday-tech.com
With MMU/D-Cache enabled, data might be retained at cache rather than at DRAM when we execute 'go' command, and some of the bare-metal softwares would always invalidate the entire data cache at start-up, and causes data lost issue.
This patch is designed to fixed this issue. It has been verified at ARM based systems, and should also work at other platforms.
Your patch desription does not match the actual code.
Got it, thanks
--- a/common/cmd_boot.c +++ b/common/cmd_boot.c @@ -50,6 +50,16 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
printf ("## Starting application at 0x%08lX ...\n", addr);+#ifdef CONFIG_USE_IRQ
disable_interrupts();+#endif
You do not mention in the commit message that you disable interrupts. This must not be done. There are many cases where interrupts are actually needed by the running code.
Got it, thanks
NAK for this part.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de A Perl script is correct if it's halfway readable and gets the job done before your boss fires you. - L. Wall & R. L. Schwartz, _Programming Perl_
-- Best wishes, Kuo-Jung Su
From: Kuo-Jung Su dantesu@faraday-tech.com
With MMU/D-Cache enabled, data might be retained at d-cache rather than at DRAM when we execute 'go' command, and some of the bare-metal softwares would always invalidate the entire data cache at start-up, and then leads to a data lost issue. Furthermore, the U-Boot not only relocates itself but also updates symbol table at start-up, which means the i-cache might also be dirty, so it would be better to invalidates the i-cache alone with d-cache flush.
This patch is designed to fix the issues above. It has been verified at ARM based systems, and should also work at other platforms.
Signed-off-by: Kuo-Jung Su dantesu@faraday-tech.com CC: Wolfgang Denk wd@denx.de --- Changes for v2: - Update the commit log to state that i-cache is also invalidated. - DO NOT disable interrupts.
common/cmd_boot.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/common/cmd_boot.c b/common/cmd_boot.c index d3836fd..9e66364 100644 --- a/common/cmd_boot.c +++ b/common/cmd_boot.c @@ -50,6 +50,13 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
printf ("## Starting application at 0x%08lX ...\n", addr);
+#ifndef CONFIG_SYS_DCACHE_OFF + flush_dcache_all(); +#endif +#ifndef CONFIG_SYS_ICACHE_OFF + invalidate_icache_all(); +#endif + /* * pass address parameter as argv[0] (aka command name), * and all remaining args -- 1.7.9.5
On Sun, Apr 28, 2013 at 04:53:08PM -0000, Kuo-Jung Su wrote:
From: Kuo-Jung Su dantesu@faraday-tech.com
With MMU/D-Cache enabled, data might be retained at d-cache rather than at DRAM when we execute 'go' command, and some of the bare-metal softwares would always invalidate the entire data cache at start-up, and then leads to a data lost issue. Furthermore, the U-Boot not only relocates itself but also updates symbol table at start-up, which means the i-cache might also be dirty, so it would be better to invalidates the i-cache alone with d-cache flush.
This patch is designed to fix the issues above. It has been verified at ARM based systems, and should also work at other platforms.
This patch has a few not trivially solvable problems. First, the only weak version of invalidate_icache_all is in common/cmd_cache.c. The only defined version of the function is for ARMv7 CPUs. So, everyone that is not ARMv7 and does not have CONFIG_CMD_CACHE will not build now.
And I'm not seeing a globally defined CONFIG to say "Yes, we are ARMv7" or "Yes, we have icache to invalidate".
This actually seems related to the cache flush issue that Freescale folks brought up a few weeks ago. Or maybe that's my pre-coffee brain talking.
On 05/10/2013 07:06:47 AM, Tom Rini wrote:
On Sun, Apr 28, 2013 at 04:53:08PM -0000, Kuo-Jung Su wrote:
From: Kuo-Jung Su dantesu@faraday-tech.com
With MMU/D-Cache enabled, data might be retained at d-cache rather than at DRAM when we execute 'go' command, and some of the bare-metal softwares would always invalidate the entire data cache at start-up, and then leads to a data lost issue. Furthermore, the U-Boot not only relocates itself but also updates symbol table at start-up, which means the i-cache might also be dirty, so it would be better to invalidates the i-cache alone with d-cache flush.
This patch is designed to fix the issues above. It has been verified at ARM based systems, and should also work at other platforms.
This patch has a few not trivially solvable problems. First, the only weak version of invalidate_icache_all is in common/cmd_cache.c. The only defined version of the function is for ARMv7 CPUs. So, everyone that is not ARMv7 and does not have CONFIG_CMD_CACHE will not build now.
And I'm not seeing a globally defined CONFIG to say "Yes, we are ARMv7" or "Yes, we have icache to invalidate".
This actually seems related to the cache flush issue that Freescale folks brought up a few weeks ago. Or maybe that's my pre-coffee brain talking.
Yes, it's the same issue that we were talking about. mpc85xx does have flush_dcache() and invalidate_icache() which could be used for this, though they're currently missing the "_all" suffix.
-Scott
participants (4)
-
Kuo-Jung Su -
Scott Wood -
Tom Rini -
Wolfgang Denk