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

[Julien Grall <julien@xxxxxxx>]

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.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.
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.
> > >    .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.
Cheers,

--
Julien Grall