[PATCH] Makefile: Fix include directory for OF_UPSTREAM

[Sumit Garg]

On Thu, 30 May 2024 at 14:43, 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.

DT headers backwards compatibility is something that upstream DT
maintainers assure us about. And we try to closely monitor any DT ABI
breakage while syncing upstream DT. Till now after we added
OF_UPSTREAM support, fortunately there hasn't been any DT ABI breakage
reports. So we should switch to upstream DT headers wherever we can
and drop any redundant local DT headers. Migration to OF_UPSTREAM can
surely be the next step but we want to encourage more and more code
reuse from upstream DT sources.

>
> 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 redundant local DT headers should be dropped wherever we can.
Adding further defines or new clocks in your case isn't going to break
backwards compatibility but rather dropping any existing clock defines
would be a DT ABI breakage.

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

That's a leftover from migration, feel free to propose a patch to drop it.

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

The additional complexity added by your patch is just needed to keep
local DT headers but that goes against using upstream DT as much as we
can and thereby reducing maintenance burden.

-Sumit