[PATCH v3 1/2] arm/cpu/start.c: Distil some common code in __start().

[PATCH v3 1/2] arm/cpu/start.c: Distil some common code in __start().

[Andrey Smirnov]

Both barebox_boarddata and barebox_boot_dtb perform essentially the
same function -- hold a pointer to a chunk of private data. Since only
one variable is ever used at any given time we may as well merge those
two variable into one. This also allows us to share more code between
two boot paths (board data vs. device tree)

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Changes since v2:

	- Restored original function semantics for
          barebox_arm_machine() and barebox_arm_boot_dtb()

	- Removed now unnecessary swith statement and auxiliary
          function that was needed to support its usage


 arch/arm/cpu/start.c | 65 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
index 8e5097b..bfe08cc 100644
--- a/arch/arm/cpu/start.c
+++ b/arch/arm/cpu/start.c
@@ -32,25 +32,37 @@
 #include "mmu-early.h"

 unsigned long arm_stack_top;
-static void *barebox_boarddata;
+static void *barebox_private_data;

-u32 barebox_arm_machine(void)
+static bool blob_is_fdt(const void *blob)
 {
-	struct barebox_arm_boarddata *bd;
-
-	if (!barebox_boarddata)
-		return 0;
-
-	bd = barebox_boarddata;
+	return get_unaligned_be32(blob) == FDT_MAGIC;
+}

-	return bd->machine;
+static bool blob_is_arm_boarddata(const void *blob)
+{
+	const struct barebox_arm_boarddata *bd = blob;
+	return bd->magic == BAREBOX_ARM_BOARDDATA_MAGIC;
 }

-static void *barebox_boot_dtb;
+u32 barebox_arm_machine(void)
+{
+	if (barebox_private_data &&
+	    blob_is_arm_boarddata(barebox_private_data)) {
+		const struct barebox_arm_boarddata *bd = barebox_private_data;
+		return bd->machine;
+	} else {
+		return 0;
+	}
+}

 void *barebox_arm_boot_dtb(void)
 {
-	return barebox_boot_dtb;
+	if (barebox_private_data &&
+	    blob_is_fdt(barebox_private_data))
+		return barebox_private_data;
+	else
+		return NULL;
 }

 static noinline __noreturn void __start(unsigned long membase,
@@ -70,7 +82,6 @@ static noinline __noreturn void __start(unsigned long membase,

 	pr_debug("memory at 0x%08lx, size 0x%08lx\n", membase, memsize);

-	barebox_boarddata = boarddata;
 	arm_stack_top = endmem;
 	endmem -= STACK_SIZE; /* Stack */

@@ -89,21 +100,23 @@ static noinline __noreturn void __start(unsigned long membase,
 	}

 	if (boarddata) {
-		if (get_unaligned_be32(boarddata) == FDT_MAGIC) {
-			uint32_t totalsize = get_unaligned_be32(boarddata + 4);
+		uint32_t totalsize = 0;
+		const char *name;
+
+		if (blob_is_fdt(boarddata)) {
+			totalsize = get_unaligned_be32(boarddata + 4);
+			name = "DTB";
+		} else if (blob_is_arm_boarddata(boarddata)) {
+			totalsize = sizeof(struct barebox_arm_boarddata);
+			name = "machine type";
+		}
+
+		if (totalsize) {
 			endmem -= ALIGN(totalsize, 64);
-			barebox_boot_dtb = (void *)endmem;
-			pr_debug("found DTB in boarddata, copying to 0x%p\n",
-					barebox_boot_dtb);
-			memcpy(barebox_boot_dtb, boarddata, totalsize);
-		} else if (((struct barebox_arm_boarddata *)boarddata)->magic ==
-				BAREBOX_ARM_BOARDDATA_MAGIC) {
-			endmem -= ALIGN(sizeof(struct barebox_arm_boarddata), 64);
-			barebox_boarddata = (void *)endmem;
-			pr_debug("found machine type in boarddata, copying to 0x%p\n",
-					barebox_boarddata);
-			memcpy(barebox_boarddata, boarddata,
-					sizeof(struct barebox_arm_boarddata));
+			pr_debug("found %s in boarddata, copying to 0x%lu\n",
+				 name, endmem);
+			barebox_private_data = memcpy((void *)endmem,
+						      boarddata, totalsize);
 		}
 	}

--
2.1.4

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v3 2/2] arm/cpu: Avoid multiple definitions of barebox_arm_entry

[Andrey Smirnov]

All versions of barebox_arm_entry (in uncompress.c, start.c and
start-pbl.c) appear to be doing exacty the same thing. So move the
definition into a separate file and use IS_ENABLED macro to avoid
re-definition.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
Changes since v2:
	- None

Changes since v1:
	- Hopefully more meaningful names for former internal symbols


 arch/arm/cpu/Makefile     |  4 ++--
 arch/arm/cpu/entry.c      | 38 +++++++++++++++++++++++++++++++++++++
 arch/arm/cpu/entry.h      | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 arch/arm/cpu/start-pbl.c  | 29 +---------------------------
 arch/arm/cpu/start.c      | 29 ++--------------------------
 arch/arm/cpu/uncompress.c | 16 +---------------
 6 files changed, 92 insertions(+), 72 deletions(-)
 create mode 100644 arch/arm/cpu/entry.c
 create mode 100644 arch/arm/cpu/entry.h

diff --git a/arch/arm/cpu/Makefile b/arch/arm/cpu/Makefile
index fb3929c..418bcab 100644
--- a/arch/arm/cpu/Makefile
+++ b/arch/arm/cpu/Makefile
@@ -1,7 +1,7 @@
 obj-y += cpu.o
 obj-$(CONFIG_ARM_EXCEPTIONS) += exceptions.o
 obj-$(CONFIG_ARM_EXCEPTIONS) += interrupts.o
-obj-y += start.o setupc.o
+obj-y += start.o setupc.o entry.o

 #
 # Any variants can be called as start-armxyz.S
@@ -23,7 +23,7 @@ AFLAGS_pbl-cache-armv7.o       :=-Wa,-march=armv7-a
 pbl-$(CONFIG_CPU_32v7) += cache-armv7.o
 obj-$(CONFIG_CACHE_L2X0) += cache-l2x0.o

-pbl-y += setupc.o
+pbl-y += setupc.o entry.o
 pbl-$(CONFIG_PBL_SINGLE_IMAGE) += start-pbl.o
 pbl-$(CONFIG_PBL_MULTI_IMAGES) += uncompress.o

diff --git a/arch/arm/cpu/entry.c b/arch/arm/cpu/entry.c
new file mode 100644
index 0000000..3b74c6a
--- /dev/null
+++ b/arch/arm/cpu/entry.c
@@ -0,0 +1,38 @@
+#include <types.h>
+
+#include <asm/cache.h>
+
+#include "entry.h"
+
+/*
+ * Main ARM entry point. Call this with the memory region you can
+ * spare for barebox. This doesn't necessarily have to be the full
+ * SDRAM. The currently running binary can be inside or outside of
+ * this region. TEXT_BASE can be inside or outside of this
+ * region. boarddata will be preserved and can be accessed later with
+ * barebox_arm_boarddata().
+ *
+ * -> membase + memsize
+ *   STACK_SIZE              - stack
+ *   16KiB, aligned to 16KiB - First level page table if early MMU support
+ *                             is enabled
+ *   128KiB                  - early memory space
+ * -> maximum end of barebox binary
+ *
+ * Usually a TEXT_BASE of 1MiB below your lowest possible end of memory should
+ * be fine.
+ */
+
+void __naked __noreturn barebox_arm_entry(unsigned long membase,
+					  unsigned long memsize, void *boarddata)
+{
+	arm_setup_stack(membase + memsize - 16);
+	arm_early_mmu_cache_invalidate();
+
+	if (IS_ENABLED(CONFIG_PBL_MULTI_IMAGES))
+		barebox_multi_pbl_start(membase, memsize, boarddata);
+	else if (IS_ENABLED(CONFIG_PBL_SINGLE_IMAGE))
+		barebox_single_pbl_start(membase, memsize, boarddata);
+	else
+		barebox_non_pbl_start(membase, memsize, boarddata);
+}
diff --git a/arch/arm/cpu/entry.h b/arch/arm/cpu/entry.h
new file mode 100644
index 0000000..80fee57
--- /dev/null
+++ b/arch/arm/cpu/entry.h
@@ -0,0 +1,48 @@
+#ifndef __ENTRY_H__
+#define __ENTRY_H__
+
+#include <common.h>
+
+#if !defined (__PBL__)
+void __noreturn barebox_non_pbl_start(unsigned long membase,
+				      unsigned long memsize,
+				      void *boarddata);
+#else
+static inline
+void __noreturn barebox_non_pbl_start(unsigned long membase,
+				      unsigned long memsize,
+				      void *boarddata)
+{
+	hang();
+};
+#endif
+
+#if defined (__PBL__) && defined(CONFIG_PBL_MULTI_IMAGES)
+void __noreturn barebox_multi_pbl_start(unsigned long membase,
+					unsigned long memsize,
+					void *boarddata);
+#else
+static inline
+void __noreturn barebox_multi_pbl_start(unsigned long membase,
+					unsigned long memsize,
+					void *boarddata)
+{
+	hang();
+}
+#endif
+
+#if defined (__PBL__) && defined(CONFIG_PBL_SINGLE_IMAGE)
+void __noreturn barebox_single_pbl_start(unsigned long membase,
+					 unsigned long memsize,
+					 void *boarddata);
+#else
+static inline
+void __noreturn barebox_single_pbl_start(unsigned long membase,
+					 unsigned long memsize,
+					 void *boarddata)
+{
+	hang();
+}
+#endif
+
+#endif
diff --git a/arch/arm/cpu/start-pbl.c b/arch/arm/cpu/start-pbl.c
index f2490fd..2075ffe 100644
--- a/arch/arm/cpu/start-pbl.c
+++ b/arch/arm/cpu/start-pbl.c
@@ -45,7 +45,7 @@ void __naked __section(.text_head_entry) pbl_start(void)
 extern void *input_data;
 extern void *input_data_end;

-static noinline __noreturn void __barebox_arm_entry(unsigned long membase,
+__noreturn void barebox_single_pbl_start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
 	uint32_t offset;
@@ -56,8 +56,6 @@ static noinline __noreturn void __barebox_arm_entry(unsigned long membase,

 	endmem -= STACK_SIZE; /* stack */

-	arm_early_mmu_cache_invalidate();
-
 	if (IS_ENABLED(CONFIG_PBL_RELOCATABLE))
 		relocate_to_current_adr();

@@ -106,28 +104,3 @@ static noinline __noreturn void __barebox_arm_entry(unsigned long membase,

 	barebox(membase, memsize, boarddata);
 }
-
-/*
- * Main ARM entry point in the compressed image. Call this with the memory
- * region you can spare for barebox. This doesn't necessarily have to be the
- * full SDRAM. The currently running binary can be inside or outside of this
- * region. TEXT_BASE can be inside or outside of this region. boarddata will
- * be preserved and can be accessed later with barebox_arm_boarddata().
- *
- * -> membase + memsize
- *   STACK_SIZE              - stack
- *   16KiB, aligned to 16KiB - First level page table if early MMU support
- *                             is enabled
- *   128KiB                  - early memory space
- * -> maximum end of barebox binary
- *
- * Usually a TEXT_BASE of 1MiB below your lowest possible end of memory should
- * be fine.
- */
-void __naked __noreturn barebox_arm_entry(unsigned long membase,
-		unsigned long memsize, void *boarddata)
-{
-	arm_setup_stack(membase + memsize - 16);
-
-	__barebox_arm_entry(membase, memsize, boarddata);
-}
diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
index bfe08cc..8e0103b 100644
--- a/arch/arm/cpu/start.c
+++ b/arch/arm/cpu/start.c
@@ -65,7 +65,7 @@ void *barebox_arm_boot_dtb(void)
 		return NULL;
 }

-static noinline __noreturn void __start(unsigned long membase,
+__noreturn void barebox_non_pbl_start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
 	unsigned long endmem = membase + memsize;
@@ -162,31 +162,6 @@ void __naked __section(.text_entry) start(void)
 	barebox_arm_head();
 }

-/*
- * Main ARM entry point in the uncompressed image. Call this with the memory
- * region you can spare for barebox. This doesn't necessarily have to be the
- * full SDRAM. The currently running binary can be inside or outside of this
- * region. TEXT_BASE can be inside or outside of this region. boarddata will
- * be preserved and can be accessed later with barebox_arm_boarddata().
- *
- * -> membase + memsize
- *   STACK_SIZE              - stack
- *   16KiB, aligned to 16KiB - First level page table if early MMU support
- *                             is enabled
- * -> maximum end of barebox binary
- *
- * Usually a TEXT_BASE of 1MiB below your lowest possible end of memory should
- * be fine.
- */
-void __naked __noreturn barebox_arm_entry(unsigned long membase,
-		unsigned long memsize, void *boarddata)
-{
-	arm_setup_stack(membase + memsize - 16);
-
-	arm_early_mmu_cache_invalidate();
-
-	__start(membase, memsize, boarddata);
-}
 #else
 /*
  * First function in the uncompressed image. We get here from
@@ -195,6 +170,6 @@ void __naked __noreturn barebox_arm_entry(unsigned long membase,
 void __naked __section(.text_entry) start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
-	__start(membase, memsize, boarddata);
+	barebox_non_pbl_start(membase, memsize, boarddata);
 }
 #endif
diff --git a/arch/arm/cpu/uncompress.c b/arch/arm/cpu/uncompress.c
index b0b7c6d..dbf6b1e 100644
--- a/arch/arm/cpu/uncompress.c
+++ b/arch/arm/cpu/uncompress.c
@@ -41,7 +41,7 @@ static int __attribute__((__used__))
 	__attribute__((__section__(".image_end")))
 	__image_end_dummy = 0xdeadbeef;

-static void __noreturn noinline uncompress_start_payload(unsigned long membase,
+void __noreturn barebox_multi_pbl_start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
 	uint32_t pg_len;
@@ -52,8 +52,6 @@ static void __noreturn noinline uncompress_start_payload(unsigned long membase,
 	void *pg_start;
 	unsigned long pc = get_pc();

-	arm_early_mmu_cache_invalidate();
-
 	endmem -= STACK_SIZE; /* stack */

 	image_end = (void *)ld_var(__image_end) - get_runtime_offset();
@@ -114,15 +112,3 @@ static void __noreturn noinline uncompress_start_payload(unsigned long membase,

 	barebox(membase, memsize, boarddata);
 }
-
-/*
- * For the multi images startup process board code jumps here. We will uncompress
- * the attached barebox image and start it.
- */
-void __naked __noreturn barebox_arm_entry(unsigned long membase,
-		unsigned long memsize, void *boarddata)
-{
-	arm_setup_stack(membase + memsize - 16);
-
-	uncompress_start_payload(membase, memsize, boarddata);
-}
--
2.1.4

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v3 2/2] arm/cpu: Avoid multiple definitions of barebox_arm_entry

[Sascha Hauer]

On Fri, Oct 30, 2015 at 08:12:23PM -0700, Andrey Smirnov wrote:
> All versions of barebox_arm_entry (in uncompress.c, start.c and
> start-pbl.c) appear to be doing exacty the same thing. So move the
> definition into a separate file and use IS_ENABLED macro to avoid
> re-definition.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> Changes since v2:
> 	- None
> 
> Changes since v1:
> 	- Hopefully more meaningful names for former internal symbols
> 
> 
>  arch/arm/cpu/Makefile     |  4 ++--
>  arch/arm/cpu/entry.c      | 38 +++++++++++++++++++++++++++++++++++++
>  arch/arm/cpu/entry.h      | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/cpu/start-pbl.c  | 29 +---------------------------
>  arch/arm/cpu/start.c      | 29 ++--------------------------
>  arch/arm/cpu/uncompress.c | 16 +---------------
>  6 files changed, 92 insertions(+), 72 deletions(-)
>  create mode 100644 arch/arm/cpu/entry.c
>  create mode 100644 arch/arm/cpu/entry.h
> 
> diff --git a/arch/arm/cpu/Makefile b/arch/arm/cpu/Makefile
> index fb3929c..418bcab 100644
> --- a/arch/arm/cpu/Makefile
> +++ b/arch/arm/cpu/Makefile
> @@ -1,7 +1,7 @@
>  obj-y += cpu.o
>  obj-$(CONFIG_ARM_EXCEPTIONS) += exceptions.o
>  obj-$(CONFIG_ARM_EXCEPTIONS) += interrupts.o
> -obj-y += start.o setupc.o
> +obj-y += start.o setupc.o entry.o
> 
>  #
>  # Any variants can be called as start-armxyz.S
> @@ -23,7 +23,7 @@ AFLAGS_pbl-cache-armv7.o       :=-Wa,-march=armv7-a
>  pbl-$(CONFIG_CPU_32v7) += cache-armv7.o
>  obj-$(CONFIG_CACHE_L2X0) += cache-l2x0.o
> 
> -pbl-y += setupc.o
> +pbl-y += setupc.o entry.o
>  pbl-$(CONFIG_PBL_SINGLE_IMAGE) += start-pbl.o
>  pbl-$(CONFIG_PBL_MULTI_IMAGES) += uncompress.o
> 
> diff --git a/arch/arm/cpu/entry.c b/arch/arm/cpu/entry.c
> new file mode 100644
> index 0000000..3b74c6a
> --- /dev/null
> +++ b/arch/arm/cpu/entry.c
> @@ -0,0 +1,38 @@
> +#include <types.h>
> +
> +#include <asm/cache.h>
> +
> +#include "entry.h"
> +
> +/*
> + * Main ARM entry point. Call this with the memory region you can
> + * spare for barebox. This doesn't necessarily have to be the full
> + * SDRAM. The currently running binary can be inside or outside of
> + * this region. TEXT_BASE can be inside or outside of this
> + * region. boarddata will be preserved and can be accessed later with
> + * barebox_arm_boarddata().
> + *
> + * -> membase + memsize
> + *   STACK_SIZE              - stack
> + *   16KiB, aligned to 16KiB - First level page table if early MMU support
> + *                             is enabled
> + *   128KiB                  - early memory space
> + * -> maximum end of barebox binary
> + *
> + * Usually a TEXT_BASE of 1MiB below your lowest possible end of memory should
> + * be fine.
> + */
> +
> +void __naked __noreturn barebox_arm_entry(unsigned long membase,
> +					  unsigned long memsize, void *boarddata)
> +{
> +	arm_setup_stack(membase + memsize - 16);
> +	arm_early_mmu_cache_invalidate();
> +
> +	if (IS_ENABLED(CONFIG_PBL_MULTI_IMAGES))
> +		barebox_multi_pbl_start(membase, memsize, boarddata);
> +	else if (IS_ENABLED(CONFIG_PBL_SINGLE_IMAGE))
> +		barebox_single_pbl_start(membase, memsize, boarddata);
> +	else
> +		barebox_non_pbl_start(membase, memsize, boarddata);
> +}
> diff --git a/arch/arm/cpu/entry.h b/arch/arm/cpu/entry.h
> new file mode 100644
> index 0000000..80fee57
> --- /dev/null
> +++ b/arch/arm/cpu/entry.h
> @@ -0,0 +1,48 @@
> +#ifndef __ENTRY_H__
> +#define __ENTRY_H__
> +
> +#include <common.h>
> +
> +#if !defined (__PBL__)
> +void __noreturn barebox_non_pbl_start(unsigned long membase,
> +				      unsigned long memsize,
> +				      void *boarddata);
> +#else
> +static inline
> +void __noreturn barebox_non_pbl_start(unsigned long membase,
> +				      unsigned long memsize,
> +				      void *boarddata)
> +{
> +	hang();
> +};
> +#endif
> +
> +#if defined (__PBL__) && defined(CONFIG_PBL_MULTI_IMAGES)
> +void __noreturn barebox_multi_pbl_start(unsigned long membase,
> +					unsigned long memsize,
> +					void *boarddata);
> +#else
> +static inline
> +void __noreturn barebox_multi_pbl_start(unsigned long membase,
> +					unsigned long memsize,
> +					void *boarddata)
> +{
> +	hang();
> +}
> +#endif
> +
> +#if defined (__PBL__) && defined(CONFIG_PBL_SINGLE_IMAGE)
> +void __noreturn barebox_single_pbl_start(unsigned long membase,
> +					 unsigned long memsize,
> +					 void *boarddata);
> +#else
> +static inline
> +void __noreturn barebox_single_pbl_start(unsigned long membase,
> +					 unsigned long memsize,
> +					 void *boarddata)
> +{
> +	hang();
> +}
> +#endif

Do we need these static inline stubs? From a first test we don't and
without them this patch looks much nicer.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v3 1/2] arm/cpu/start.c: Distil some common code in __start().

[Sascha Hauer]

On Fri, Oct 30, 2015 at 08:12:22PM -0700, Andrey Smirnov wrote:
> Both barebox_boarddata and barebox_boot_dtb perform essentially the
> same function -- hold a pointer to a chunk of private data. Since only
> one variable is ever used at any given time we may as well merge those
> two variable into one. This also allows us to share more code between
> two boot paths (board data vs. device tree)
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Applied with a small change.

> ---
> 
> Changes since v2:
> 
> 	- Restored original function semantics for
>           barebox_arm_machine() and barebox_arm_boot_dtb()
> 
> 	- Removed now unnecessary swith statement and auxiliary
>           function that was needed to support its usage
> 
> 
>  arch/arm/cpu/start.c | 65 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
> index 8e5097b..bfe08cc 100644
> --- a/arch/arm/cpu/start.c
> +++ b/arch/arm/cpu/start.c
> @@ -32,25 +32,37 @@
>  #include "mmu-early.h"
> 
>  unsigned long arm_stack_top;
> -static void *barebox_boarddata;
> +static void *barebox_private_data;

I renamed this back to barebox_boarddata since it's data that comes
from the board code.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox