Re: [RFC PATCH 5/5] doc: Add a document for non-compliant DT node/property removal

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5434 bytes --]

On Wed, Aug 30, 2023 at 10:24:39AM +0300, Ilias Apalodimas wrote:
> Hi Tom
> 
> On Mon, 28 Aug 2023 at 21:39, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Aug 29, 2023 at 12:04:53AM +0530, Sughosh Ganu wrote:
> > > hi Simon,
> > >
> > > On Mon, 28 Aug 2023 at 23:25, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > Add a document explaining the need for removal of non-compliant
> > > > > devicetree nodes and properties. Also describe in brief, the macros
> > > > > that can be used for this removal.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >  .../devicetree/dt_non_compliant_purge.rst     | 64 +++++++++++++++++++
> > > > >  1 file changed, 64 insertions(+)
> > > > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > >
> > > > > diff --git a/doc/develop/devicetree/dt_non_compliant_purge.rst b/doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > > new file mode 100644
> > > > > index 0000000000..c3a8feab5b
> > > > > --- /dev/null
> > > > > +++ b/doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > > @@ -0,0 +1,64 @@
> > > > > +.. SPDX-License-Identifier: GPL-2.0+
> > > > > +
> > > > > +Removal of non-compliant nodes and properties
> > > > > +=============================================
> > > > > +
> > > > > +The devicetree used in U-Boot might contain nodes and properties which
> > > > > +are specific only to U-Boot, and are not necessarily being used to
> > > > > +describe hardware but to pass information to U-Boot. An example of
> > > > > +such a property would be the public key being passed to U-Boot for
> > > > > +verification.
> > > >
> > > > It has nothing to do with describing hardware. The DT can describe
> > > > other things too. See the /options node, for example.
> > > >
> > > > Please don't bring this highly misleading language into U-Boot.
> > >
> > > Please point out what is misleading in the above paragraph. What is
> > > being emphasised in the above paragraph is that certain nodes and
> > > properties in the devicetree are relevant only in u-boot, and not the
> > > kernel. And this is precisely what the devicetree maintainers are
> > > saying [1].
> > >
> > > >
> > > > > +
> > > > > +This devicetree can then be passed to the OS. Since certain nodes and
> > > > > +properties are not really describing hardware, and more importantly,
> > > > > +these are only relevant to U-Boot, bindings for these cannot be
> > > > > +upstreamed into the devicetree repository. There have been instances
> > > > > +of attempts being made to upstream such bindings, and these deemed not
> > > > > +fit for upstreaming.
> > > >
> > > > Then either they should not be in U-Boot, or there is a problem with
> > > > the process.
> > > >
> > > > > Not having a binding for these nodes and
> > > > > +properties means that the devicetree fails the schema compliance tests
> > > > > +[1]. This also means that the platform cannot get certifications like
> > > > > +SystemReady [2] which, among other things require a devicetree which
> > > > > +passes the schema compliance tests.
> > > > > +
> > > > > +For such nodes and properties, it has been suggested by the devicetree
> > > > > +maintainers that the right thing to do is to remove them from the
> > > > > +devicetree before it gets passed on to the OS [3].
> > > >
> > > > Hard NAK. If we go this way, then no one will ever have an incentive
> > > > to do the right thing.
> > > >
> > > > Please send bindings for Linaro's work, instead. If something is
> > > > entirely U-Boot-specific, then it can go in /options/u-boot but it
> > > > still must be in the dt-schema.
> > >
> > > Please re-read the document including the last link [1]. If you go
> > > through that entire thread, you will notice that this is precisely
> > > what Linaro was trying to do here -- upstream the binding for the
> > > fwu-mdata node. It is only based on the feedback of the devicetree
> > > maintainers that this patchset was required.
> > >
> > > -sughosh
> > >
> > > [1] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/#t
> >
> > Please note that this right here, that the explanation of why we need to
> > delete this node, not being a bright shiny visible object is one of the
> > big problems with this patchset and implementation. It cannot be
> > footnotes in email threads as to why such-and-such node/property isn't
> > upstream, it needs to be documented and visible in the code base /
> > documentation and an obvious you must do this for future cases.
> 
> I thought we agreed that deleting nodes that won't be accepted
> upstream is the right approach since that would break the systemready
> 2.0 compatibility.
> 
> Yes, it can't be footnotes or hidden links, but this is totally
> different than what I am reading on this thread.

An issue is that the functionality got posted without clear links as to
why the initial nodes to be deleted had been rejected, in the patchset
itself (and so not preserved long term). Being able to show that yes,
really, it was attempted to upstream the nodes, and not "delete first
upstream later (never)" is critical.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]