Re: [RFC 0/2] kbuild: Add support to build overlays (%.dtbo)

[Masahiro Yamada]

On Tue, Jan 12, 2021 at 1:13 AM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
>
> +David Gibson
>
> On Mon, Jan 11, 2021 at 9:40 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >
> > On Mon, Jan 11, 2021 at 8:17 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > >
> > > On 07-01-21, 14:28, Masahiro Yamada wrote:
> > > > Viresh's patch is not enough.
> > > >
> > > > We will need to change .gitignore
> > > > and scripts/Makefile.dtbinst as well.
> > > >
> > > > In my understanding, the build rule is completely the same
> > > > between .dtb and .dtbo
> > > > As Rob mentioned, I am not sure if we really need/want
> > > > a separate extension.
> > > >
> > > > A counter approach is to use an extension like '.ovl.dtb'
> > > > It clarifies it is an overlay fragment without changing
> > > > anything in our build system or the upstream DTC project.
> > >
> > > By the time you gave feedback, I have already sent the dtbo change for
> > > DTC to the device-tree-compiler list (based on Rob's suggestion).
> > >
> > > And it got merged today by David:
> > >
> > > https://github.com/dgibson/dtc/commit/163f0469bf2ed8b2fe5aa15bc796b93c70243ddc
> > >
> > > Can we please finalize what we need to do with naming here and be done
> > > with it, so I can rework my patches and get going ?
> > >
> > > Thanks.
> > >
> > > --
> > > viresh
> >
> >
> >
> > It is unfortunate to see such a patch merged
> > before getting agreement about how it should work
> > as a whole.
>
> Given the feedback that dtbo is already a standard, I'd suggest we
> just stick with dts->dtbo.

OK.
dtbo seems a stanrdard already...



> > >+# enable creation of __symbols__ node
> > >+ifneq ($(dtbo-y),)
> > >+DTC_FLAGS += -@
> > >+endif
> >
> > I am not convinced with this code.
> >
> > A single user of the dtbo-y syntax gives -@ to all
> > device trees in the same directory.
> >
> > This is not a solution since Rob already stated -@ should be
> > given per board (or per platform, at least).
>
> Agreed.
>
> > I still do not understand why adding the new syntax dtbo-y
> > is helpful.
>
> I think we should stick with 'dtb-y' here.
>
>
> > Have we already decided to use separate ".dtb" and ".dtbo" for blobs?
> >
> > Will we use ".dts" for all source files?
> > Or, will we use ".dtso" for overlay source files?
> >
> > How should the build system determine the targets
> > that should have -@ option?
>
> The way it does already. Either:
>
> DTC_FLAGS += -@
>
> in a directory of dts files. Or on a per file basis like:
>
> DTC_FLAGS_foo_base += -@


Ah yes.  I like this.




We do not need the dtbo-y syntax.


We can simply use dtb-y for
base boards and overlay fragments:



dtb-$(CONFIG_ARCH_FOO) += \
     foo-base.dtb \
     foo-overlay1.dtbo \
     foo-overlay2.dtbo

DTB_FLAGS_foo-base += -@







>
> > For consistency, will we need a patch like follows?
> >
> >
> > diff --git a/dtc.c b/dtc.c
> > index bdb3f59..474401e 100644
> > --- a/dtc.c
> > +++ b/dtc.c
> > @@ -120,6 +120,8 @@ static const char *guess_type_by_name(const char
> > *fname, const char *fallback)
> >                 return fallback;
> >         if (!strcasecmp(s, ".dts"))
> >                 return "dts";
> > +       if (!strcasecmp(s, ".dtso"))
> > +               return "dts";
> >         if (!strcasecmp(s, ".yaml"))
> >                 return "yaml";
> >         if (!strcasecmp(s, ".dtb"))
> > @@ -349,6 +351,8 @@ int main(int argc, char *argv[])
> >
> >         if (streq(outform, "dts")) {
> >                 dt_to_source(outf, dti);
> > +       else if (streq(outform, "dtso")) {
> > +               dt_to_source(outf, dti);
> >  #ifndef NO_YAML
> >         } else if (streq(outform, "yaml")) {
> >                 if (!streq(inform, "dts"))
> >
> >
> >
> > Overall solution looks unclear to me.
> >
> >
> > Again, it is unfortunate that we did not take enough time
> > (in spite of the RFC prefix) before proceeding.
>
> I should have added David here from the start. Honestly, I expected
> some discussion there.
>
> Rob




-- 
Best Regards
Masahiro Yamada