Re: [PATCH v2] arm: Add Kconfig entry to select CONFIG_DTB_FILE

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 08.03.2021 15:31, Julien Grall wrote:
> Hi,
> 
> On 08/03/2021 13:59, Michal Orzel wrote:
>> Currently in order to link existing DTB into Xen image
>> we need to either specify option CONFIG_DTB_FILE on the
>> command line or manually add it into .config.
>> Add Kconfig entry: CONFIG_DTB_FILE to be able to
>> provide the path to DTB we want to embed into Xen image.
>> If no path provided - the dtb will not be embedded.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>>   xen/arch/arm/Makefile | 4 +---
>>   xen/common/Kconfig    | 8 ++++++++
>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 16e6523e2c..0f3e99d075 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -68,7 +68,7 @@ extra-y += $(TARGET_SUBARCH)/head.o
>>     #obj-bin-y += ....o
>>   -ifdef CONFIG_DTB_FILE
>> +ifneq ($(CONFIG_DTB_FILE),"")
>>   obj-y += dtb.o
>>   AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
>>   endif
>> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
>>   xen.lds: xen.lds.S
>>       $(CPP) -P $(a_flags) -MQ $@ -o $@ $<
>>   -dtb.o: $(CONFIG_DTB_FILE)
>> -
> 
> Why is this dropped?
1)This line is not needed as it has no impact on creating dtb.o
2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as 
string within quotes.
> 
>>   .PHONY: clean
>>   clean::
>>       rm -f asm-offsets.s xen.lds
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index eb953d171e..a4c8d09edf 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -400,6 +400,14 @@ config DOM0_MEM
>>           Leave empty if you are not sure what to specify.
>>   +config DTB_FILE
> 
> May I ask why is this add in common/Kconfig rather than arm/Kconfig?
> 
I wanted to have it in common features rather than architecture features.
Maybe it could be later on used by other architectures.
>> +    string "Absolute path to device tree blob"
>> +    depends on ARM
> 
> If this stay in common Kconfig, shouldn't this be gated with HAS_DEVICE_TREE?
No it shouldn't as  CONFIG_DTB_FILE depends on CONFIG_ARM which selects 
CONFIG_HAS_DEVICE_TREE
> 
>> +    ---help---
>> +      When using a bootloader that has no device tree support or when there
>> +      is no bootloader at all, use this option to specify the absolute path
>> +      to a device tree that will be linked directly inside Xen binary.
>> +
>>   config TRACEBUFFER
>>       bool "Enable tracing infrastructure" if EXPERT
>>       default y
>>
> 
> Cheers,
> 
Cheers,
Michal