[U-Boot-Users] [patch] disable caches before booting an app for Blackfin apps

[U-Boot-Users] [patch] disable caches before booting an app for Blackfin apps

[Mike Frysinger]

It isn't generally save to execute applications outside of U-Boot with caches
enabled due to the way the Blackfin processor handles caches (requires
software assistance).  This patch disables caches before booting an ELF or
just booting raw code.  The previous discussion on the patch was that we
wanted to use weaks instead, but that proved to not be feasible when multiple
symbols are involved, which puts us back at the ifdef solution.  I've
minimized the ugliness by moving the setup step outside of the main function.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
diff --git a/common/cmd_boot.c b/common/cmd_boot.c
index e68f16f..9d4f026 100644
--- a/common/cmd_boot.c
+++ b/common/cmd_boot.c
@@ -32,6 +32,23 @@
 DECLARE_GLOBAL_DATA_PTR;
 #endif
 
+static inline void go_setup(int argc, char *argv[])
+{
+#if defined(CONFIG_I386)
+	/*
+	 * x86 does not use a dedicated register to pass the pointer
+	 * to the global_data
+	 */
+	argv[0] = (char *)gd;
+
+#elif defined(CONFIG_BLACKFIN)
+	if (dcache_status ())
+		dcache_disable ();
+	if (icache_status ())
+		icache_disable ();
+#endif
+}
+
 int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 {
 	ulong	addr, rc;
@@ -46,25 +63,20 @@ int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 
 	printf ("## Starting application at 0x%08lX ...\n", addr);
 
+	go_setup(argc, argv);
+
+#if defined(CONFIG_NIOS)
 	/*
-	 * pass address parameter as argv[0] (aka command name),
-	 * and all remaining args
-	 */
-#if defined(CONFIG_I386)
-	/*
-	 * x86 does not use a dedicated register to pass the pointer
-	 * to the global_data
+	 * Nios function pointers are address >> 1
 	 */
-	argv[0] = (char *)gd;
+	addr >>= 1;
 #endif
-#if !defined(CONFIG_NIOS)
-	rc = ((ulong (*)(int, char *[]))addr) (--argc, &argv[1]);
-#else
+
 	/*
-	 * Nios function pointers are address >> 1
+	 * pass address parameter as argv[0] (aka command name),
+	 * and all remaining args
 	 */
-	rc = ((ulong (*)(int, char *[]))(addr>>1)) (--argc, &argv[1]);
-#endif
+	rc = ((ulong (*)(int, char *[]))addr) (--argc, &argv[1]);
 	if (rc != 0) rcode = 1;
 
 	printf ("## Application terminated, rc = 0x%lX\n", rc);
diff --git a/common/cmd_elf.c b/common/cmd_elf.c
index 2eb7453..4683554 100644
--- a/common/cmd_elf.c
+++ b/common/cmd_elf.c
@@ -27,6 +27,21 @@ DECLARE_GLOBAL_DATA_PTR;
 #define MAX(a,b) ((a) > (b) ? (a) : (b))
 #endif
 
+static inline void bootelf_setup(int argc, char *argv[])
+{
+	/*
+	 * QNX images require the data cache is disabled.
+	 * Data cache is already flushed, so just turn it off.
+	 */
+	if (dcache_status ())
+		dcache_disable ();
+
+#ifdef CONFIG_BLACKFIN
+	if (icache_status ())
+		icache_disable ();
+#endif
+}
+
 int valid_elf_image (unsigned long addr);
 unsigned long load_elf_image (unsigned long addr);
 
@@ -53,12 +68,7 @@ int do_bootelf (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 
 	printf ("## Starting application at 0x%08lx ...\n", addr);
 
-	/*
-	 * QNX images require the data cache is disabled.
-	 * Data cache is already flushed, so just turn it off.
-	 */
-	if (dcache_status ())
-		dcache_disable ();
+	bootelf_setup(argc, argv);
 
 	/*
 	 * pass address parameter as argv[0] (aka command name),

[U-Boot-Users] [patch] disable caches before booting an app for Blackfin apps

[Rafal Jaworowski]

Hi Mike,

> It isn't generally save to execute applications outside of U-Boot with caches
> enabled due to the way the Blackfin processor handles caches (requires
> software assistance).  This patch disables caches before booting an ELF or
> just booting raw code.  The previous discussion on the patch was that we
> wanted to use weaks instead, but that proved to not be feasible when multiple
> symbols are involved, which puts us back at the ifdef solution.  I've
> minimized the ugliness by moving the setup step outside of the main function.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> diff --git a/common/cmd_boot.c b/common/cmd_boot.c
> index e68f16f..9d4f026 100644
> --- a/common/cmd_boot.c
> +++ b/common/cmd_boot.c
> @@ -32,6 +32,23 @@
>  DECLARE_GLOBAL_DATA_PTR;
>  #endif
>  
> +static inline void go_setup(int argc, char *argv[])
> +{
> +#if defined(CONFIG_I386)
> +	/*
> +	 * x86 does not use a dedicated register to pass the pointer
> +	 * to the global_data
> +	 */
> +	argv[0] = (char *)gd;
> +
> +#elif defined(CONFIG_BLACKFIN)
> +	if (dcache_status ())
> +		dcache_disable ();
> +	if (icache_status ())
> +		icache_disable ();
> +#endif
> +}
> +
>  int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  {
>  	ulong	addr, rc;
> @@ -46,25 +63,20 @@ int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  
>  	printf ("## Starting application at 0x%08lX ...\n", addr);
>  
> +	go_setup(argc, argv);
> +
> +#if defined(CONFIG_NIOS)
>  	/*
> -	 * pass address parameter as argv[0] (aka command name),
> -	 * and all remaining args
> -	 */
> -#if defined(CONFIG_I386)
> -	/*
> -	 * x86 does not use a dedicated register to pass the pointer
> -	 * to the global_data
> +	 * Nios function pointers are address >> 1
>  	 */
> -	argv[0] = (char *)gd;
> +	addr >>= 1;
>  #endif
> -#if !defined(CONFIG_NIOS)
> -	rc = ((ulong (*)(int, char *[]))addr) (--argc, &argv[1]);
> -#else
> +
>  	/*
> -	 * Nios function pointers are address >> 1
> +	 * pass address parameter as argv[0] (aka command name),
> +	 * and all remaining args
>  	 */
> -	rc = ((ulong (*)(int, char *[]))(addr>>1)) (--argc, &argv[1]);
> -#endif
> +	rc = ((ulong (*)(int, char *[]))addr) (--argc, &argv[1]);
>  	if (rc != 0) rcode = 1;
>  
>  	printf ("## Application terminated, rc = 0x%lX\n", rc);
> diff --git a/common/cmd_elf.c b/common/cmd_elf.c
> index 2eb7453..4683554 100644
> --- a/common/cmd_elf.c
> +++ b/common/cmd_elf.c
> @@ -27,6 +27,21 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define MAX(a,b) ((a) > (b) ? (a) : (b))
>  #endif
>  
> +static inline void bootelf_setup(int argc, char *argv[])
> +{
> +	/*
> +	 * QNX images require the data cache is disabled.
> +	 * Data cache is already flushed, so just turn it off.
> +	 */
> +	if (dcache_status ())
> +		dcache_disable ();
> +

Please re-formulate the comment, so it doesn't only refer to QNX, as per our
recent discussion disabling d-cache before running an external app is
considered a generic U-Boot operation, not platform specific, see this thread:

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/34599

Also, I'm wondering if we shouldn't re-enable the cache after returning from
an external app (as they are allowed to do so)? Without doing it we end up
with degraded performance without any notice, require explicit reset to return
to previous state etc.

kind regards,
Rafal

[U-Boot-Users] [patch] disable caches before booting an app for Blackfin apps

[Mike Frysinger]

On Wednesday 30 January 2008, Rafal Jaworowski wrote:
> Hi Mike,
>
> > It isn't generally save to execute applications outside of U-Boot with
> > caches enabled due to the way the Blackfin processor handles caches
> > (requires software assistance).  This patch disables caches before
> > booting an ELF or just booting raw code.  The previous discussion on the
> > patch was that we wanted to use weaks instead, but that proved to not be
> > feasible when multiple symbols are involved, which puts us back at the
> > ifdef solution.  I've minimized the ugliness by moving the setup step
> > outside of the main function.
> >
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > ---
> > diff --git a/common/cmd_boot.c b/common/cmd_boot.c
> > index e68f16f..9d4f026 100644
> > --- a/common/cmd_boot.c
> > +++ b/common/cmd_boot.c
> > @@ -32,6 +32,23 @@
> >  DECLARE_GLOBAL_DATA_PTR;
> >  #endif
> >
> > +static inline void go_setup(int argc, char *argv[])
> > +{
> > +#if defined(CONFIG_I386)
> > +	/*
> > +	 * x86 does not use a dedicated register to pass the pointer
> > +	 * to the global_data
> > +	 */
> > +	argv[0] = (char *)gd;
> > +
> > +#elif defined(CONFIG_BLACKFIN)
> > +	if (dcache_status ())
> > +		dcache_disable ();
> > +	if (icache_status ())
> > +		icache_disable ();
> > +#endif
> > +}
> > +
> >  int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> >  {
> >  	ulong	addr, rc;
> > @@ -46,25 +63,20 @@ int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char
> > *argv[])
> >
> >  	printf ("## Starting application at 0x%08lX ...\n", addr);
> >
> > +	go_setup(argc, argv);
> > +
> > +#if defined(CONFIG_NIOS)
> >  	/*
> > -	 * pass address parameter as argv[0] (aka command name),
> > -	 * and all remaining args
> > -	 */
> > -#if defined(CONFIG_I386)
> > -	/*
> > -	 * x86 does not use a dedicated register to pass the pointer
> > -	 * to the global_data
> > +	 * Nios function pointers are address >> 1
> >  	 */
> > -	argv[0] = (char *)gd;
> > +	addr >>= 1;
> >  #endif
> > -#if !defined(CONFIG_NIOS)
> > -	rc = ((ulong (*)(int, char *[]))addr) (--argc, &argv[1]);
> > -#else
> > +
> >  	/*
> > -	 * Nios function pointers are address >> 1
> > +	 * pass address parameter as argv[0] (aka command name),
> > +	 * and all remaining args
> >  	 */
> > -	rc = ((ulong (*)(int, char *[]))(addr>>1)) (--argc, &argv[1]);
> > -#endif
> > +	rc = ((ulong (*)(int, char *[]))addr) (--argc, &argv[1]);
> >  	if (rc != 0) rcode = 1;
> >
> >  	printf ("## Application terminated, rc = 0x%lX\n", rc);
> > diff --git a/common/cmd_elf.c b/common/cmd_elf.c
> > index 2eb7453..4683554 100644
> > --- a/common/cmd_elf.c
> > +++ b/common/cmd_elf.c
> > @@ -27,6 +27,21 @@ DECLARE_GLOBAL_DATA_PTR;
> >  #define MAX(a,b) ((a) > (b) ? (a) : (b))
> >  #endif
> >
> > +static inline void bootelf_setup(int argc, char *argv[])
> > +{
> > +	/*
> > +	 * QNX images require the data cache is disabled.
> > +	 * Data cache is already flushed, so just turn it off.
> > +	 */
> > +	if (dcache_status ())
> > +		dcache_disable ();
> > +
>
> Please re-formulate the comment, so it doesn't only refer to QNX, as per
> our recent discussion disabling d-cache before running an external app is
> considered a generic U-Boot operation, not platform specific, see this
> thread:

i dont know anything about the topic.  feel free to submit a follow up patch.  
i merely relocated the existing comment.

> Also, I'm wondering if we shouldn't re-enable the cache after returning
> from an external app (as they are allowed to do so)? Without doing it we
> end up with degraded performance without any notice, require explicit reset
> to return to previous state etc.

i did see that deficiency in the code ... that'd require the code to stay 
inline in the function :/
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080130/f77249d4/attachment.pgp 

[U-Boot-Users] [patch] disable caches before booting an app for Blackfin apps

[Wolfgang Denk]

In message <200801291821.05927.vapier@gentoo.org> you wrote:
> It isn't generally save to execute applications outside of U-Boot with caches
> enabled due to the way the Blackfin processor handles caches (requires
> software assistance).  This patch disables caches before booting an ELF or
> just booting raw code.  The previous discussion on the patch was that we
> wanted to use weaks instead, but that proved to not be feasible when multiple
> symbols are involved, which puts us back at the ifdef solution.  I've
> minimized the ugliness by moving the setup step outside of the main function.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Applied. Thanks.

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 at denx.de
One does not thank logic.
	-- Sarek, "Journey to Babel", stardate 3842.4