[PATCH] cmd: go: Flush cache before jumping to app/image

[PATCH] cmd: go: Flush cache before jumping to app/image

[Stefan Roese]

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@denx.de>
Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
Cc: Mauro Condarelli <mc5686@mclink.it>
Cc: Weijie Gao <weijie.gao@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

[PATCH] cmd: go: Flush cache before jumping to app/image

[Daniel Schwierzeck]

On Wed, Feb 12, 2020 at 1:21 PM Stefan Roese <sr@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@denx.de>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> Cc: Mauro Condarelli <mc5686@mclink.it>
> Cc: Weijie Gao <weijie.gao@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.

-- 
- Daniel

[PATCH] cmd: go: Flush cache before jumping to app/image

[Mauro Condarelli]

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@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@denx.de>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>> Cc: Mauro Condarelli <mc5686@mclink.it>
>> Cc: Weijie Gao <weijie.gao@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.
>

[PATCH] cmd: go: Flush cache before jumping to app/image

[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

[PATCH] cmd: go: Flush cache before jumping to app/image

[Mauro Condarelli]

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

[PATCH] cmd: go: Flush cache before jumping to app/image

[Stefan]

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

[PATCH] cmd: go: Flush cache before jumping to app/image

[Stefan]

Hi Daniel,

On 12.02.20 14:00, Daniel Schwierzeck wrote:
> On Wed, Feb 12, 2020 at 1:21 PM Stefan Roese <sr@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@denx.de>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>> Cc: Mauro Condarelli <mc5686@mclink.it>
>> Cc: Weijie Gao <weijie.gao@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