| Message ID | 20200212122151.28591-1-sr@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | cmd: go: Flush cache before jumping to app/image | expand |
On Wed, Feb 12, 2020 at 1:21 PM Stefan Roese <sr at denx.de> wrote: > > It has been noticed on MT7628/88 platforms, that booting the RAM image > does not work reliably. Sometimes it works and sometimes not. Debugging > showed that this "might" be a cache related issue as very strange > errors occured (output corrupted etc). > > This patch adds a cache flush for the complete SDRAM area to the go cmd > before jumping to the entry point. The complete area is flushed as we > don't know at this point, how big the area of the "application" really > is. > > Signed-off-by: Stefan Roese <sr at denx.de> > Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com> > Cc: Mauro Condarelli <mc5686 at mclink.it> > Cc: Weijie Gao <weijie.gao at mediatek.com> > --- > cmd/boot.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/cmd/boot.c b/cmd/boot.c > index 9150fce80b..968522face 100644 > --- a/cmd/boot.c > +++ b/cmd/boot.c > @@ -9,10 +9,13 @@ > */ > #include <common.h> > #include <command.h> > +#include <cpu_func.h> > #include <net.h> > > #ifdef CONFIG_CMD_GO > > +DECLARE_GLOBAL_DATA_PTR; > + > /* Allow ports to override the default behavior */ > __attribute__((weak)) > unsigned long do_go_exec(ulong (*entry)(int, char * const []), int argc, > @@ -33,6 +36,13 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > printf ("## Starting application at 0x%08lX ...\n", addr); > > + /* > + * Flush cache before jumping to application. Let's flush the > + * whole SDRAM area, since we don't know the size of the image > + * that was loaded. > + */ > + flush_cache(gd->bd->bi_memstart, gd->bd->bi_memsize); > + > /* > * pass address parameter as argv[0] (aka command name), > * and all remaining args > -- > 2.25.0 > I think that's not right for all architectures or cache-coherent systems. It would be better to implement do_go_exec() in arch/mips/libs/bootm.c because this function is already annotated as weak.
Hi Daniel, Hi Stefan, I'll test this ASAP, but, in the meantime, please have a look at the strange (and perhaps related) code in arch/mips/mach-mtmips/cpu.c Regards Mauro On 2/12/20 2:00 PM, Daniel Schwierzeck wrote: > On Wed, Feb 12, 2020 at 1:21 PM Stefan Roese <sr at denx.de> wrote: >> It has been noticed on MT7628/88 platforms, that booting the RAM image >> does not work reliably. Sometimes it works and sometimes not. Debugging >> showed that this "might" be a cache related issue as very strange >> errors occured (output corrupted etc). >> >> This patch adds a cache flush for the complete SDRAM area to the go cmd >> before jumping to the entry point. The complete area is flushed as we >> don't know at this point, how big the area of the "application" really >> is. >> >> Signed-off-by: Stefan Roese <sr at denx.de> >> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com> >> Cc: Mauro Condarelli <mc5686 at mclink.it> >> Cc: Weijie Gao <weijie.gao at mediatek.com> >> --- >> cmd/boot.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/cmd/boot.c b/cmd/boot.c >> index 9150fce80b..968522face 100644 >> --- a/cmd/boot.c >> +++ b/cmd/boot.c >> @@ -9,10 +9,13 @@ >> */ >> #include <common.h> >> #include <command.h> >> +#include <cpu_func.h> >> #include <net.h> >> >> #ifdef CONFIG_CMD_GO >> >> +DECLARE_GLOBAL_DATA_PTR; >> + >> /* Allow ports to override the default behavior */ >> __attribute__((weak)) >> unsigned long do_go_exec(ulong (*entry)(int, char * const []), int argc, >> @@ -33,6 +36,13 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> >> printf ("## Starting application at 0x%08lX ...\n", addr); >> >> + /* >> + * Flush cache before jumping to application. Let's flush the >> + * whole SDRAM area, since we don't know the size of the image >> + * that was loaded. >> + */ >> + flush_cache(gd->bd->bi_memstart, gd->bd->bi_memsize); >> + >> /* >> * pass address parameter as argv[0] (aka command name), >> * and all remaining args >> -- >> 2.25.0 >> > I think that's not right for all architectures or cache-coherent > systems. It would be better > to implement do_go_exec() in arch/mips/libs/bootm.c because this function is > already annotated as weak. >
Hi Daniel, On 12.02.20 14:00, Daniel Schwierzeck wrote: > On Wed, Feb 12, 2020 at 1:21 PM Stefan Roese <sr at denx.de> wrote: >> >> It has been noticed on MT7628/88 platforms, that booting the RAM image >> does not work reliably. Sometimes it works and sometimes not. Debugging >> showed that this "might" be a cache related issue as very strange >> errors occured (output corrupted etc). >> >> This patch adds a cache flush for the complete SDRAM area to the go cmd >> before jumping to the entry point. The complete area is flushed as we >> don't know at this point, how big the area of the "application" really >> is. >> >> Signed-off-by: Stefan Roese <sr at denx.de> >> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com> >> Cc: Mauro Condarelli <mc5686 at mclink.it> >> Cc: Weijie Gao <weijie.gao at mediatek.com> >> --- >> cmd/boot.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/cmd/boot.c b/cmd/boot.c >> index 9150fce80b..968522face 100644 >> --- a/cmd/boot.c >> +++ b/cmd/boot.c >> @@ -9,10 +9,13 @@ >> */ >> #include <common.h> >> #include <command.h> >> +#include <cpu_func.h> >> #include <net.h> >> >> #ifdef CONFIG_CMD_GO >> >> +DECLARE_GLOBAL_DATA_PTR; >> + >> /* Allow ports to override the default behavior */ >> __attribute__((weak)) >> unsigned long do_go_exec(ulong (*entry)(int, char * const []), int argc, >> @@ -33,6 +36,13 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> >> printf ("## Starting application at 0x%08lX ...\n", addr); >> >> + /* >> + * Flush cache before jumping to application. Let's flush the >> + * whole SDRAM area, since we don't know the size of the image >> + * that was loaded. >> + */ >> + flush_cache(gd->bd->bi_memstart, gd->bd->bi_memsize); >> + >> /* >> * pass address parameter as argv[0] (aka command name), >> * and all remaining args >> -- >> 2.25.0 >> > > I think that's not right for all architectures or cache-coherent > systems. It would be better > to implement do_go_exec() in arch/mips/libs/bootm.c because this function is > already annotated as weak. Good idea. I'll change it accordingly in v2 once its been tested and confirmed by Mauro. Thanks, Stefan
Hi Mauro, On 12.02.20 14:09, Mauro Condarelli wrote: > Hi Daniel, > Hi Stefan, > > I'll test this ASAP, but, in the meantime, please have a look at the > strange (and perhaps related) code in arch/mips/mach-mtmips/cpu.c You are most likely referring to the code in last_stage_init() that also deals with a potential cache issue. Its still unresolved and this "hack" is still needed for a working U-Boot image (e.g. no timeouts in tftp). Its only partly related to the current issue though as it also deals with "cache" but addresses a different issue. Please test and report back. Thanks, Stefan
Hi Stefan, On 2/12/20 2:22 PM, Stefan wrote: > Hi Mauro, > > On 12.02.20 14:09, Mauro Condarelli wrote: >> Hi Daniel, >> Hi Stefan, >> >> I'll test this ASAP, but, in the meantime, please have a look at the >> strange (and perhaps related) code in arch/mips/mach-mtmips/cpu.c > > You are most likely referring to the code in last_stage_init() that > also deals with a potential cache issue. Its still unresolved and > this "hack" is still needed for a working U-Boot image (e.g. no timeouts > in tftp). Yes, I was referring to that strange "D-cache" cleanup. Why not just a cache-flush()? > Its only partly related to the current issue though as it also deals > with "cache" but addresses a different issue. Please test and report > back. Done. Confirmed. With new version in SPI NOR I could load (and execute successfully) all versions I had saved on my tftp server. If some further testing is useful don't hesitate to ask. > Thanks, > Stefan > Many thanks Mauro
Hi Mauro, On 12.02.20 14:50, Mauro Condarelli wrote: > Hi Stefan, > > On 2/12/20 2:22 PM, Stefan wrote: >> Hi Mauro, >> >> On 12.02.20 14:09, Mauro Condarelli wrote: >>> Hi Daniel, >>> Hi Stefan, >>> >>> I'll test this ASAP, but, in the meantime, please have a look at the >>> strange (and perhaps related) code in arch/mips/mach-mtmips/cpu.c >> >> You are most likely referring to the code in last_stage_init() that >> also deals with a potential cache issue. Its still unresolved and >> this "hack" is still needed for a working U-Boot image (e.g. no timeouts >> in tftp). > Yes, I was referring to that strange "D-cache" cleanup. > Why not just a cache-flush()? Frankly, I don't remember the exact details. I definitely tested also with cache_flush() but either it introduced too much boot delay or it simply didn't work. This d-cache issue was (is) pretty nasty. >> Its only partly related to the current issue though as it also deals >> with "cache" but addresses a different issue. Please test and report >> back. > Done. > Confirmed. > With new version in SPI NOR I could load (and execute successfully) > all versions I had saved on my tftp server. Good. :) > If some further testing is useful don't hesitate to ask. I'll send v2 in a minute. Please re-test it and send your tested-by, if this also works for you. Thanks, Stefan
diff --git a/cmd/boot.c b/cmd/boot.c index 9150fce80b..968522face 100644 --- a/cmd/boot.c +++ b/cmd/boot.c @@ -9,10 +9,13 @@ */ #include <common.h> #include <command.h> +#include <cpu_func.h> #include <net.h> #ifdef CONFIG_CMD_GO +DECLARE_GLOBAL_DATA_PTR; + /* Allow ports to override the default behavior */ __attribute__((weak)) unsigned long do_go_exec(ulong (*entry)(int, char * const []), int argc, @@ -33,6 +36,13 @@ static int do_go(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) printf ("## Starting application at 0x%08lX ...\n", addr); + /* + * Flush cache before jumping to application. Let's flush the + * whole SDRAM area, since we don't know the size of the image + * that was loaded. + */ + flush_cache(gd->bd->bi_memstart, gd->bd->bi_memsize); + /* * pass address parameter as argv[0] (aka command name), * and all remaining args
It has been noticed on MT7628/88 platforms, that booting the RAM image does not work reliably. Sometimes it works and sometimes not. Debugging showed that this "might" be a cache related issue as very strange errors occured (output corrupted etc). This patch adds a cache flush for the complete SDRAM area to the go cmd before jumping to the entry point. The complete area is flushed as we don't know at this point, how big the area of the "application" really is. Signed-off-by: Stefan Roese <sr at denx.de> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com> Cc: Mauro Condarelli <mc5686 at mclink.it> Cc: Weijie Gao <weijie.gao at mediatek.com> --- cmd/boot.c | 10 ++++++++++ 1 file changed, 10 insertions(+)