Hi,
On 09.03.2021 11:20, Julien Grall wrote:
>
>
> On 09/03/2021 07:34, Michal Orzel wrote:
>> Hi Julien,
>
> Hi,
>
>> 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.or...@arm.com>
>>>> ---
>>>> 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.
>
> Because of 1), this should have ideally be part of a separate patch. But I am
> OK to keep it in this patch so long it is explained in the commit message.
Ok I will explain it in the commit msg in v3
>
>>>
>>>> .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.
>
> The same can be argued for a few CONFIG in arch/.../Kconfig. What I want to
> avoid is spreading depends on <ARCH> in the common/Kconfig.
>
>>>> + 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
> I think you misunderstood my point, what I suggested is replacing "depends on
> Arm" by "depends on HAS_DEVICE_TREE".
>
> This is for two reasons:
> 1) This avoids spreading depend on <ARCH> in common/kconfig
> 2) This avoids the assumption that Arm is always using DT
>
> If you would rather not use "depends on HAS_DEVICE_TREE", then I think this
> config should go in arch/arm/Kconfig until we see another users.
>
Ok I will keep it in common/Kconfig but switch to depends on HAS_DEVICE_TREE
> Cheers,
>
Cheers
