[PATCH] Makefile: Fix include directory for OF_UPSTREAM

[Patrick Barsanti]

Hi Sumit,

On Wed, 29 May 2024 at 14:00, Sumit Garg <sumit.garg at linaro.org> wrote:

> On Wed, 29 May 2024 at 14:45, Patrick Barsanti
> <patrick.barsanti at amarulasolutions.com> wrote:
> >
> > Hi Sumit,
> >
> > On Wed, 29 May 2024 at 08:57, Sumit Garg <sumit.garg at linaro.org> wrote:
> >>
> >> Hi Patrick,
> >>
> >> On Tue, 28 May 2024 at 14:16, Patrick Barsanti
> >> <patrick.barsanti at amarulasolutions.com> wrote:
> >> >
> >> > Always prioritizing u-boot includes causes problems when trying to
> >> > migrate boards to OF_UPSTREAM that have divergent devicetree files
> with
> >> > respect to the upstream ones.
> >> >
> >> > For example, migrating a board based on `imx6ul.dtsi` to OF_UPSTREAM
> >> > breaks it, as there are some missing defines in the local dtsi file;
> >> > the solutions would be to either patch it, which defeats the purpose
> of
> >> > OF_UPSTREAM, or delete it entirely. This last option would then break
> all
> >> > the other boards which have not yet been migrated to OF_UPSTREAM.
> >>
> >> Can you elaborate more here regarding which dt-bindings headers
> >> conflict? Also, is it only the DTS files consumer for those headers or
> >> there are U-Boot drivers depending on them too?
> >>
> >> -Sumit
> >
> >
> > Sorry, I think I have worded my commit message wrong. I should
> > have used differ instead of diverge, which is slightly misleading.
> >
> > The specific case I am talking about can be found in:
> > include/dt-bindings/clock/imx6ul-clock.h
> > dts/upstream/include/dt-bindings/clock/imx6ul-clock.h
> >
> > The local header is missing the last commit from the kernel, which is
> > 4e197ee880c2 ("clk: imx6ul: add ethernet refclock mux support").
> > This added some new defines, which are not present in the u-boot
> > header.
> > Following this commit, the `imx6ul.dtsi` was patched in the kernel to
> > use one of the new defines.
> >
> > Because of this, at the current state, migrating a board which is
> > somehow based on `imx6ul.dtsi` will give a dtc error given by a value
> > being used in the upstream dtsi which is not defined in the local
> > header, because local includes always have priority with respect
> > to upstream ones even when setting OF_UPSTREAM.
>
> So you should just drop the local DT bindings header:
> include/dt-bindings/clock/imx6ul-clock.h and that should resolve the
> problem for you, right?


Yes, that indeed solves my problem.
But if I drop it, I will force all other boards which are not yet
migrated to OF_UPSTREAM and include `imx6ul.dtsi` to
use the upstream header instead of the local one.
I did not feel comfortable in doing so, because if any change is made
to the upstream header in the future which is somehow not backwards
compatible, then all boards which are still not using OF_UPSTREAM
would not compile.

I also thought this would not be limited to the single header file which
caused my problem. I imagine there would be other cases of local
headers which are missing one or more commits from mainline kernel
and cause the same type of problem when moving to OF_UPSTREAM.

The opposite problem also exists.
For example:
675575880f ("phycore-am64x: Migrate to OF_UPSTREAM")
does not drop include/dt-bindings/net/ti-dp83867.h, and I think the
migration worked properly only because at the moment there is no
difference between local and upstream headers.
If and when the upstream headers and devicetrees will be patched,
this will cause problems, because the local include will be used
even if it's out-of-date.
I have tested this: by simulating a kernel patch to the upstream files,
which adds an extra define in ti-dp83867.h and updates
k3-am64-phycore-som.dtsi to use this new define, current state
u-boot will fail to build because that define is not present in the local
include header.
By including my patch, the build is successful.

This is the reason why I proposed this Makefile patch, but of course I
am completely open to suggestions and ideas better than mine, which
I suspect are fairly easy to come by :)

Thank you,
Regards,
Patrick


>
> -Sumit


> >
> > Regards,
> > Patrick
> >
> >>
> >> >
> >> > The opposite problem also exists: by always prioritizing upstream
> >> > includes, if changes are made in the kernel headers and devicetree
> >> > files that are not backwards compatible, again all boards which have
> not
> >> > been migrated to OF_UPSTREAM will break.
> >> >
> >> > This patch fixes this problem by prioritizing upstream includes when
> >> > `CONFIG_OF_UPSTREAM=y`, while keeping current prioritization when
> >> > it is not.
> >> >
> >> > Signed-off-by: Patrick Barsanti <
> patrick.barsanti at amarulasolutions.com>
> >> > ---
> >> >  Makefile | 14 ++++++++++++++
> >> >  1 file changed, 14 insertions(+)
> >> >
> >> > diff --git a/Makefile b/Makefile
> >> > index 79b28c2d81..899ae664ca 100644
> >> > --- a/Makefile
> >> > +++ b/Makefile
> >> > @@ -826,6 +826,19 @@ KBUILD_HOSTCFLAGS += $(if
> $(CONFIG_TOOLS_DEBUG),-g)
> >> >
> >> >  # Use UBOOTINCLUDE when you must reference the include/ directory.
> >> >  # Needed to be compatible with the O= option
> >> > +ifeq ($(CONFIG_OF_UPSTREAM),y)
> >> > +UBOOTINCLUDE    := \
> >> > +       -I$(srctree)/dts/upstream/include \
> >> > +       -Iinclude \
> >> > +       $(if $(KBUILD_SRC), -I$(srctree)/include) \
> >> > +       $(if $(CONFIG_$(SPL_)SYS_THUMB_BUILD), \
> >> > +               $(if $(CONFIG_HAS_THUMB2), \
> >> > +                       $(if $(CONFIG_CPU_V7M), \
> >> > +
>  -I$(srctree)/arch/arm/thumb1/include), \
> >> > +                       -I$(srctree)/arch/arm/thumb1/include)) \
> >> > +       -I$(srctree)/arch/$(ARCH)/include \
> >> > +       -include $(srctree)/include/linux/kconfig.h
> >> > +else
> >> >  UBOOTINCLUDE    := \
> >> >         -Iinclude \
> >> >         $(if $(KBUILD_SRC), -I$(srctree)/include) \
> >> > @@ -837,6 +850,7 @@ UBOOTINCLUDE    := \
> >> >         -I$(srctree)/arch/$(ARCH)/include \
> >> >         -include $(srctree)/include/linux/kconfig.h \
> >> >         -I$(srctree)/dts/upstream/include
> >> > +endif
> >> >
> >> >  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> -print-file-name=include)
> >> >
> >> > --
> >> > 2.43.0
> >> >
>