Adding DTB to architecture independent vmlinux

From: Dirk Brandewie <[email protected]>

This patch adds support for linking device tree blobs into vmlinux. The blobs linked into the image is controlled via kernel config variables

Signed-off-by Dirk Brandewie <[email protected]>

---
 arch/x86/Kconfig          |    6 +++++-
 arch/x86/boot/dts/Kconfig |    8 ++++++++
 arch/x86/kernel/Makefile  |   11 +++++++++++
 3 files changed, 24 insertions(+), 1 deletions(-)
 create mode 100644 arch/x86/boot/dts/Kconfig

arch/x86/Kconfig +5 -1

index 5904f38..62c195d 100644
@@ -299,13 +299,17 @@ config X86_BIGSMP
 	---help---
 	  This option is needed for the systems that have more than 8 CPUs
 
-config X86_OF
+menuconfig X86_OF
 	bool "Support for device tree"
 	select OF
 	select OF_FLATTREE
 	---help---
 	  Device tree support on X86.
 
+if X86_OF
+source "arch/x86/boot/dts/Kconfig
+endif
+
 if X86_32
 config X86_EXTENDED_PLATFORM
 	bool "Support for extended (non-PC) x86 platforms"

arch/x86/boot/dts/Kconfig new file +0 -0

new file mode 100644
index 0000000..5380b6b

From: Dirk Brandewie <[email protected]>

This patch adds support for linking device tree blobs into vmlinux. The device tree blobs are placed in the init.data section.

Signed-off-by Dirk Brandewie <[email protected]>

---
 include/asm-generic/vmlinux.lds.h |   13 ++++++++++++-
 init/Kconfig                      |    7 +++++++
 scripts/Makefile.lib              |   14 ++++++++++++++
 3 files changed, 33 insertions(+), 1 deletions(-)

include/asm-generic/vmlinux.lds.h +0 -0

index bd69d79..c8f600e 100644

include/asm-generic/vmlinux.lds.h +2 -1

 /* .data section */
 #define DATA_DATA							\
 	*(.data)							\

@@ -468,7 +478,8 @@
 	MCOUNT_REC()							\
 	DEV_DISCARD(init.rodata)					\
 	CPU_DISCARD(init.rodata)					\
-	MEM_DISCARD(init.rodata)
+	MEM_DISCARD(init.rodata)					\
+	KERNEL_DTB
 
 #define INIT_TEXT							\
 	*(.init.text)							\

init/Kconfig +0 -0

index 88c1046..fddfc0f 100644

init/Kconfig +1 -0

+
 config SLUB_DEBUG
 	default y
 	bool "Enable SLUB debugging support" if EMBEDDED

scripts/Makefile.lib +4 -0

index 4c72c11..c4487b2 100644
@@ -200,6 +200,20 @@ quiet_cmd_gzip = GZIP    $@
 cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
 	(rm -f $@ ; false)
 
+# DTC
+#  ---------------------------------------------------------------------------
+$(obj)/%.dtb.S: $(obj)/%.dtb FORCE
+	@echo '.section .dtb,"a"' > $@

Quoted 18 lines · On Thu, Nov 11, 2010 at 04:03:48PM -0800…

From: Dirk Brandewie<[email protected]>

This patch adds support for linking device tree blobs into vmlinux. The device tree blobs are placed in the init.data section.

Signed-off-by Dirk Brandewie<[email protected]>

---
  include/asm-generic/vmlinux.lds.h |   13 ++++++++++++-
  init/Kconfig                      |    7 +++++++
  scripts/Makefile.lib              |   14 ++++++++++++++
  3 files changed, 33 insertions(+), 1 deletions(-)

include/asm-generic/vmlinux.lds.h +0 -0

index bd69d79..c8f600e 100644

include/asm-generic/vmlinux.lds.h line 150

Quoted 5 lines

include/asm-generic/vmlinux.lds.h +2 -0

@@ -146,6 +146,16 @@
  #define TRACE_SYSCALLS()
  #endif
 
+#ifdef CONFIG_KERNEL_DTB
+#define KERNEL_DTB							\

To match other definitions in this file, this should be defined with
parentheses: #define KERNEL_DTB()

include/asm-generic/vmlinux.lds.h line 153

Quoted 3 lines

include/asm-generic/vmlinux.lds.h +3 -0

+	. = ALIGN(32);							\
+	VMLINUX_SYMBOL(__dtb_start) = .;				\
+	*(.dtb)			       					\

I wonder if .meminit.rodata.dtb be a better section name. Could use some input from more experienced kernel hackers here. hpa, what say you?

Also, inconsistent indentation (mixed tabs and spaces).

include/asm-generic/vmlinux.lds.h line 158

Quoted 5 lines

include/asm-generic/vmlinux.lds.h +5 -0

+	VMLINUX_SYMBOL(__dtb_end) = .;
+#else
+#define KERNEL_DTB
+#endif
+

Does this need to be wrapped with CONFIG_KERNEL_DTB? Is there any downside to including these sections unconditionally?

Quoted 18 lines

include/asm-generic/vmlinux.lds.h +2 -1

  /* .data section */
  #define DATA_DATA							\
  	*(.data)							\

@@ -468,7 +478,8 @@
  	MCOUNT_REC()							\
  	DEV_DISCARD(init.rodata)					\
  	CPU_DISCARD(init.rodata)					\
-	MEM_DISCARD(init.rodata)
+	MEM_DISCARD(init.rodata)					\
+	KERNEL_DTB
 
  #define INIT_TEXT							\
  	*(.init.text)							\

init/Kconfig +0 -0

index 88c1046..fddfc0f 100644

init/Kconfig config PCI_QUIRKS · line 1091

Quoted 9 lines

init/Kconfig +6 -0

@@ -1083,6 +1083,13 @@ config PCI_QUIRKS
            bugs/quirks. Disable this only if your target machine is
            unaffected by PCI quirks.
 
+config KERNEL_DTB
+       bool "Support linking a device tree blob into vmlinux"
+       default n
+       help
+         This option provides support for adding a device tree blob(s)
+	 directly to vmlinux

Inconsistent indentation. I'm also not convinced that this Kconfig symbol is really needed (as commented on above).

Quoted 16 lines

init/Kconfig +1 -0

+
  config SLUB_DEBUG
  	default y
  	bool "Enable SLUB debugging support" if EMBEDDED

scripts/Makefile.lib +4 -0

index 4c72c11..c4487b2 100644
@@ -200,6 +200,20 @@ quiet_cmd_gzip = GZIP    $@
  cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9>  $@) || \
  	(rm -f $@ ; false)
 
+# DTC
+#  ---------------------------------------------------------------------------
+$(obj)/%.dtb.S: $(obj)/%.dtb FORCE
+	@echo '.section .dtb,"a"'>  $@

scripts/Makefile.lib quiet_cmd_gzip = GZIP $@ · line 216

Quoted 10 lines

scripts/Makefile.lib +10 -0

+	@echo '.global __dtb_$(*F)_begin'>>  $@
+	@echo '__dtb_$(*F)_begin:'>>  $@
+	@echo '.incbin "$<" '>>  $@
+	@echo '__dtb_$(*F)_end:'>>  $@
+	@echo '.global __dtb_$(*F)_end'>>  $@
+
+DTC = $(objtree)/scripts/dtc/dtc
+
+quiet_cmd_dtc = DTC	$@
+      cmd_dtc = $(DTC) -O dtb -o $(obj)/$*.dtb -b 0  $(dtstree)/$*.dts

As already mentioned; need to rationalized these rule additions with the changes made in patch #1.

g.

Quoted 11 lines

To match other definitions in this file, this should be defined with
parentheses: #define KERNEL_DTB()

include/asm-generic/vmlinux.lds.h line 153

Quoted 3 lines

include/asm-generic/vmlinux.lds.h +3 -0

+	. = ALIGN(32);							\
+	VMLINUX_SYMBOL(__dtb_start) = .;				\
+	*(.dtb)			       					\

I wonder if .meminit.rodata.dtb be a better section name. Could use some input from more experienced kernel hackers here. hpa, what say you?

I will use what ever name seems reasonable, It turns out that the sections that are named *.init.rodata are really in a R/W section :-( so that name is really a misnomer. I will check with HPA on name again.

From: Dirk Brandewie <[email protected]>

This patch forces the size of the DTB to be modulo 32 bytes. This is needed to support linking multiple DTB's into a single section in the image. GCC wants structures to be 32 byte aligned without this change DTB's after the first in the section may not be properly aligned so the flat tree parsing code will fall over.

Signed-off-by Dirk Brandewie <[email protected]>

Quoted 9 lines · On 11/11/2010 04:03 PM, [email protected]…

From: Dirk Brandewie<[email protected]>

This patch forces the size of the DTB to be modulo 32 bytes. This is needed to support linking multiple DTB's into a single section in the image. GCC wants structures to be 32 byte aligned without this change DTB's after the first in the section may not be properly aligned so the flat tree parsing code will fall over.

Signed-off-by Dirk Brandewie<[email protected]>

I don't think 32 is a universal number; it should depend on the ABI. On x86, for one thing, I'm pretty sure that there is no particular alignment requirements beyond the natural alignment of the data items, for example.

Although 32 is probably conservative on any platform, please flag the origin of this with a comment, or make it a constant defined in a header file... otherwise, if this breaks for whatever reason it'll be near impossible to find.

Quoted 16 lines · H. Peter Anvin

Quoted 9 lines · On 11/11/2010 04:03 PM, [email protected]…

From: Dirk Brandewie<[email protected]>

This patch forces the size of the DTB to be modulo 32 bytes. This is needed to support linking multiple DTB's into a single section in the image. GCC wants structures to be 32 byte aligned without this change DTB's after the first in the section may not be properly aligned so the flat tree parsing code will fall over.

Signed-off-by Dirk Brandewie<[email protected]>

I don't think 32 is a universal number; it should depend on the ABI. On x86, for one thing, I'm pretty sure that there is no particular alignment requirements beyond the natural alignment of the data items, for example.

Quoted 5 lines

Although 32 is probably conservative on any platform, please flag the origin of this with a comment, or make it a constant defined in a header file... otherwise, if this breaks for whatever reason it'll be near impossible to find.

I will change . = ALIGN(32); to STRUCT_ALIGN();

from vmlinux.lds.h:
/*
* Align to a 32 byte boundary equal to the
* alignment gcc 4.5 uses for a struct
*/
#define STRUCT_ALIGN() . = ALIGN(32)

Quoted 23 lines · Dirk Brandewie

Quoted 16 lines · H. Peter Anvin

Quoted 9 lines · On 11/11/2010 04:03 PM, [email protected]…

From: Dirk Brandewie<[email protected]>

This patch forces the size of the DTB to be modulo 32 bytes. This is needed to support linking multiple DTB's into a single section in the image. GCC wants structures to be 32 byte aligned without this change DTB's after the first in the section may not be properly aligned so the flat tree parsing code will fall over.

Signed-off-by Dirk Brandewie<[email protected]>

I don't think 32 is a universal number; it should depend on the ABI. On x86, for one thing, I'm pretty sure that there is no particular alignment requirements beyond the natural alignment of the data items, for example.

Quoted 5 lines

Although 32 is probably conservative on any platform, please flag the origin of this with a comment, or make it a constant defined in a header file... otherwise, if this breaks for whatever reason it'll be near impossible to find.

Quoted 8 lines

I will change . = ALIGN(32); to STRUCT_ALIGN();

from vmlinux.lds.h:
/*
* Align to a 32 byte boundary equal to the
* alignment gcc 4.5 uses for a struct
*/
#define STRUCT_ALIGN() . = ALIGN(32)

I also think this is the wrong place to do this. scripts/dtc is a mirror of copy of upstream dtc, designed for making dtb images for general purposes. I think the alignment should instead go into the linker script fragments you generate to incbin the dtbs.

The second issue is being able to parse the section in the kernel image to find each of the blobs that have been concatenated together. If the DTB size is modulo 32 bytes I can use blob = blob+be32_to_cpu(blob->totalsize) to find the next blob in the section and use that address directly to have fdt.c parse the blob. Otherwise I would need to search for the signature of the next blob somewhere past the end of the current blob which is knid of messy IMHO.

Where is the DTC git tree I should be delivering patches against? is
git://www.jdl.com/software/dtc.git the correct tree?

Hi David,

Quoted 1 line · David Gibson

On Thu, Nov 11, 2010 at 05:01:16PM -0800, Dirk Brandewie wrote:

Quoted 4 lines

I also think this is the wrong place to do this. scripts/dtc is a mirror of copy of upstream dtc, designed for making dtb images for general purposes. I think the alignment should instead go into the linker script fragments you generate to incbin the dtbs.

I was sleep deprived when I responded to HPA I thought he had replied to the patch modifying vmlinux.lds.h since we had talked about that file a fair amount. So my first response had little bearing on patch in question.

I am trying to solve two issues with this change.

The structure needs to be 32 byte aligned for the code in fdt.c to be able parse the blob. You are right I can force this alignment in the assembly fragment.

The second issue is being able to parse the section in the kernel image to find each of the blobs that have been concatenated together. If the DTB size is modulo 32 bytes I can use blob = blob+be32_to_cpu(blob->totalsize) to find the next blob in the section and use that address directly to have fdt.c parse the blob. Otherwise I would need to search for the signature of the next blob somewhere past the end of the current blob which is knid of messy IMHO.

I have two questions,
Do you think this acceptable if I made forcing the alignment a command line argument?

Where is the DTC git tree I should be delivering patches against? is git://www.jdl.com/software/dtc.git the correct tree?

On Fri, Nov 12, 2010 at 08:24:53AM -0800, Dirk Brandewie wrote:

Quoted 6 lines

I am trying to solve two issues with this change.

The structure needs to be 32 byte aligned for the code in fdt.c to be able parse the blob. You are right I can force this alignment in the assembly fragment.

Quoted 8 lines

The second issue is being able to parse the section in the kernel image to find each of the blobs that have been concatenated together. If the DTB size is modulo 32 bytes I can use blob = blob+be32_to_cpu(blob->totalsize) to find the next blob in the section and use that address directly to have fdt.c parse the blob. Otherwise I would need to search for the signature of the next blob somewhere past the end of the current blob which is knid of messy IMHO.

As hpa says you can just align your increment too, to find the next blob.

Quoted 4 lines

I have two questions,
Do you think this acceptable if I made forcing the alignment a command line argument?

Um, sure. It's not actually necessary for what you're doing here, but it might have some use, I guess, and it's pretty non-invasive. Your posted patch for this, however, is broken, see my comments in reply,

From: Dirk Brandewie <[email protected]>

Add support for specifying a "compatible" string from the kernel command line and functions for the platform to find the compatible blob present in the kernel image if any.

Signed-off-by Dirk Brandewie <[email protected]>

---
 drivers/of/fdt.c       |   52 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_fdt.h |    9 ++++++++
 2 files changed, 61 insertions(+), 0 deletions(-)

drivers/of/fdt.c +3 -0

index c1360e0..07fe4c6 100644
@@ -604,3 +604,55 @@ void __init unflatten_device_tree(void)
 
 	pr_debug(" <- unflatten_device_tree()\n");
 }
+
+#ifndef MODULE
+#ifdef CONFIG_OF_FLATTREE

drivers/of/fdt.c +4 -0

+	}
+	if (rc == NULL)
+		initial_boot_params = orig_initial_boot_params;	
+	return rc;

include/linux/of_fdt.h +7 -0

 /* TBD: Temporary export of fdt globals - remove when code fully merged */
 extern int __initdata dt_root_addr_cells;
 extern int __initdata dt_root_size_cells;

@@ -82,6 +84,13 @@ extern void early_init_dt_add_memory_arch(u64 base, u64 size);
 extern u64 early_init_dt_alloc_memory_arch(u64 size, u64 align);
 extern u64 dt_mem_next_cell(int s, __be32 **cellp);
 
+extern char *of_get_dtb_compatible_string(void);
+#ifdef CONFIG_KERNEL_DTB
+extern void *of_find_compatible_dtb(char *name);
+#else
+static inline void *of_find_compatible_dtb(char *name) {return 0;}
+
+#endif 
 /*
  * If BLK_DEV_INITRD, the fdt early init code will call this function,
  * to be provided by the arch code. start and end are specified as

Signature · 6 lines

1.7.2.3

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss

drivers/of/fdt.c void __init unflatten_device_tree(void) · line 635

Quoted 3 lines

drivers/of/fdt.c +1 -0

+			initial_boot_params = blob;

Blech! I really need to get Stephen (cc'd) to respin his "of/fdt: Add unflatten_partial_device_tree" patch that decouples the flattree functions from initial_boot_params. Having to do it this way is just plain ugly.

Stephen: nudge, nudge.

Quoted 8 lines

Quoted 3 lines

+                   initial_boot_params = blob;

Blech!  I really need to get Stephen (cc'd) to respin his "of/fdt: Add unflatten_partial_device_tree" patch that decouples the flattree functions from initial_boot_params.  Having to do it this way is just plain ugly.

Quoted 1 line

Stephen: nudge, nudge.

If things have settled down, I'll dust them off again. Don't know if it will happen this week: I'm called for jury duty.

'char' is completely bogus in this context. Either void or struct boot_param_header would be more accurate and useful. I believe you can do:

	extern void __dtb_start;
	extern_void __dtb_end;

And then reference the addresses with &__dtb_start and &__dtb_end. That would eliminate a lot of the casting below.

Quoted 8 lines · Grant Likely

'char' is completely bogus in this context. Either void or struct boot_param_header would be more accurate and useful. I believe you can do:

	extern void __dtb_start;
	extern_void __dtb_end;

Quoted 3 lines

And then reference the addresses with &__dtb_start and &__dtb_end. That would eliminate a lot of the casting below.

Except that doesn't work with gcc 4.5+; you really need to use an array type.