Re: [PATCH v3 32/71] ARC: [DeviceTree] Basic support

[Vineet Gupta <Vineet.Gupta1@synopsys.com>]

Hi James,

On Monday 28 January 2013 03:51 PM, James Hogan wrote:
> Hi Vineet,
> 
> On 24/01/13 10:50, Vineet Gupta wrote:
>> diff --git a/arch/arc/Makefile b/arch/arc/Makefile
>> index 4d52a3b..90570f9 100644
>> --- a/arch/arc/Makefile
>> +++ b/arch/arc/Makefile
>> @@ -83,6 +83,9 @@ head-y		:= arch/arc/kernel/head.o
>>  # See arch/arc/Kbuild for content of core part of the kernel
>>  core-y		+= arch/arc/
>>  
>> +# w/o this dtb won't embed into kernel binary
>> +core-y		+= arch/arc/boot/dts/
>> +
>>  # w/o this ifneq, make ARCH=arc clean was crapping out
>>  ifneq ($(platform-y),)
>>  core-y		+= arch/arc/plat-$(PLATFORM)/
>> @@ -101,6 +104,12 @@ bootpImage: vmlinux
>>  uImage: vmlinux
>>  	$(Q)$(MAKE) $(build)=$(boot) $(boot)/$@
>>  
>> +%.dtb %.dtb.S %.dtb.o:
>> +	$(Q)$(MAKE) $(build)=$(boot)/dts $(boot)/dts/$@
> 
> arm64 also depends on scripts here, presumably for a reason. I've copied
> this in metag too.

It probably has to do with forcing rebuild of DTs, in case scripts/dtc/dtc changes.

I'll add that as well.


>> +
>> +dtbs:
>> +	$(Q)$(MAKE) $(build)=$(boot)/dts $(boot)/dts/$@
> 
> arm64 does this like below which I think is slightly nicer than how
> metag did it:
> 
> dtbs: scripts
> 	$(Q)$(MAKE) $(build)=$(boot)/dts dtbs

OK !

>> +
>>  archclean:
>>  	$(Q)$(MAKE) $(clean)=$(boot)
>>  
>> diff --git a/arch/arc/boot/dts/Makefile b/arch/arc/boot/dts/Makefile
>> new file mode 100644
>> index 0000000..4a972a3
>> --- /dev/null
>> +++ b/arch/arc/boot/dts/Makefile
>> @@ -0,0 +1,14 @@
>> +ifeq ($(CONFIG_OF),y)
>> +
>> +# Built-in dtb
>> +builtindtb-y		:= skeleton
>> +
>> +ifneq ($(CONFIG_ARC_BUILTIN_DTB_NAME),"")
>> +	builtindtb-y	:= $(CONFIG_ARC_BUILTIN_DTB_NAME)
>> +endif
>> +
>> +obj-y	+= $(patsubst "%",%,$(builtindtb-y)).dtb.o
>> +
>> +clean-files := *.dtb
> 
> You don't seem to have a rule for dtbs, but still redirect it in the
> main Makefile. I suppose dtbs may want adding to your archhelp if you
> use it.

The dtbs rule doesn't seem to be working for me here. I'll need to investigate a bit.

> 
> arm64 also adds dtbs and $(dtb-y) to targets (at least in linux-next). I
> think this makes sure that they don't get rebuilt unless they've
> actually changed. I've also copied this for metag.

Did you verify that it makes a difference - at least at my end it's not - the
default dtb is getting rebuilt everytime. I need to debug this some more.


>> --- a/arch/arc/kernel/Makefile
>> +++ b/arch/arc/kernel/Makefile
>> @@ -8,6 +8,8 @@
>>  obj-y	:= arcksyms.o setup.o irq.o time.o reset.o ptrace.o entry.o process.o
>>  obj-y	+= signal.o traps.o sys.o troubleshoot.o stacktrace.o clk.o
>>  
>> +obj-$(CONFIG_OF)			+= devtree.o
>> +
> 
> You always select CONFIG_OF, so this could be obj-y. I'm not sure this
> particularly matters though.

Right, for consistency we might add this unconditionally.

Thanks for your review
-Vineet