[PATCH] Makefile: Fix include directory for OF_UPSTREAM

[Michael Nazzareno Trimarchi]

Hi Patrick

On Thu, May 30, 2024 at 11:13 AM Patrick Barsanti
<patrick.barsanti at amarulasolutions.com> wrote:
>
> 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 :)
>

Based on the discussion so far, you can extend the commit message so
other people can comment on it

Michael

> 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
>> >> >



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael at amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info at amarulasolutions.com
www.amarulasolutions.com