Re: [PATCH 2/4] of: Add support for linking device tree blobs into vmlinux

[Dirk Brandewie <dirk.brandewie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 11/13/2010 09:25 PM, Grant Likely wrote:
> On Thu, Nov 11, 2010 at 04:03:48PM -0800, dirk.brandewie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Dirk Brandewie<dirk.brandewie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> 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<dirk.brandewie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>   include/asm-generic/vmlinux.lds.h |   13 ++++++++++++-
>>   init/Kconfig                      |    7 +++++++
>>   scripts/Makefile.lib              |   14 ++++++++++++++
>>   3 files changed, 33 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index bd69d79..c8f600e 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -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()
>
>> +	. = 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.

>
> Also, inconsistent indentation (mixed tabs and spaces).
>

will fix

>> +	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?
>

Not sure I was just following the convention set by TRACE_SYSCALLS(), 
BRANCH_PROFILE(), ...


>>   /* .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)							\
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 88c1046..fddfc0f 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -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).
>
>> +
>>   config SLUB_DEBUG
>>   	default y
>>   	bool "Enable SLUB debugging support" if EMBEDDED
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 4c72c11..c4487b2 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -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"'>  $@
>> +	@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.
>