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

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

On 8/26/23 11:06, Sughosh Ganu 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>

Thanks for properly documenting the change.

Warning, treated as error:
doc/develop/devicetree/dt_non_compliant_purge.rst:
document isn't included in any toctree

Please, add the document to doc/develop/devicetree/index.rst

Please, run make htmldocs before resubmitting.

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

but these were not deemed fit

> +fit for upstreaming. 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

%s/require/requires/

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

%s/that the right thing to do is//

> +devicetree before it gets passed on to the OS [3].

%s/on to/to/

> +
> +Removing nodes/properties
> +-------------------------
> +
> +In U-Boot, this is been done through adding information on such nodes

%s/is been done through/is done by/

> +and properties in a list. The entire node can be deleted, or a
> +specific property under a node can be deleted. The list of such nodes
> +and properties is generated at compile time, and the function to purge
> +these can be invoked through a EVT_FT_FIXUP event notify call.
> +
> +For deleting a node, this can be done by declaring a macro::
> +
> +	DT_NON_COMPLIANT_PURGE(fwu_mdata) = {
> +		.node_path      = "/fwu-mdata",
> +	};

Where should such a macro be placed?

> +
> +Similarly, for deleting a property under a node, that can be done by
> +specifying the property name::
> +
> +	DT_NON_COMPLIANT_PURGE(capsule_key) = {
> +		.node_path      = "/signature",
> +		.prop           = "capsule-key",
> +	};

Why is capsule_key needed twice here? What would be the effect of:

DT_NON_COMPLIANT_PURGE(voodoo) = {
	.node_path      = "/signature",
	.prop           = "capsule-key",
};

> +
> +In the first example, the entire node with path /fwu-mdata will be
> +removed. In the second example, the property capsule-key
> +under /signature node will be removed.
> +
> +Similarly, a list of nodes and properties can be specified using the
> +following macro::
> +
> +	DT_NON_COMPLIANT_PURGE_LIST(foo) = {

What does foo refer to here? Can this parameter be eliminated from the
syntax?

Will the application of the removals be sorted alphabetically by this
argument?

What will happen if a property or node does not exist at time of
removal? Hopefully that does not stop the boot process but is silently
ignored.

> +		{ .node_path = "/some_node", .prop = "some_bar" },
> +		{ .node_path = "/some_node" },
> +	};

Why do you need the first element in your example?
Wouldn't deleting /some/node imply that all its children are deleted?

Why do we need separate properties .node_path and .prop. It would be
less effort to write:

DT_NON_COMPLIANT_PURGE_LIST() =
{
	"/some_node_1/some_property_1",
	"/some_node_1/some_property_2",
	"/some_node_2",
}

I assume the macro does its job irrespective of non-compliance.
Why not call it DT_PURGE()?

Best regards

Heinrich

> +
> +[1] - https://github.com/devicetree-org/dt-schema
> +[2] - https://www.arm.com/architecture/system-architectures/systemready-certification-program
> +[3] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/

On Sat, 26 Aug 2023 at 15:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/26/23 11:06, Sughosh Ganu 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>
>
> Thanks for properly documenting the change.
>
> Warning, treated as error:
> doc/develop/devicetree/dt_non_compliant_purge.rst:
> document isn't included in any toctree
>
> Please, add the document to doc/develop/devicetree/index.rst
>
> Please, run make htmldocs before resubmitting.
>
> > ---
> >   .../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.
> > +
> > +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
>
> but these were not deemed fit
>
> > +fit for upstreaming. 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
>
> %s/require/requires/
>
> > +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
>
> %s/that the right thing to do is//
>
> > +devicetree before it gets passed on to the OS [3].
>
> %s/on to/to/
>
> > +
> > +Removing nodes/properties
> > +-------------------------
> > +
> > +In U-Boot, this is been done through adding information on such nodes
>
> %s/is been done through/is done by/
>
> > +and properties in a list. The entire node can be deleted, or a
> > +specific property under a node can be deleted. The list of such nodes
> > +and properties is generated at compile time, and the function to purge
> > +these can be invoked through a EVT_FT_FIXUP event notify call.
> > +
> > +For deleting a node, this can be done by declaring a macro::
> > +
> > +     DT_NON_COMPLIANT_PURGE(fwu_mdata) = {
> > +             .node_path      = "/fwu-mdata",
> > +     };
>
> Where should such a macro be placed?

It should be placed in any file that will get compiled.

>
> > +
> > +Similarly, for deleting a property under a node, that can be done by
> > +specifying the property name::
> > +
> > +     DT_NON_COMPLIANT_PURGE(capsule_key) = {
> > +             .node_path      = "/signature",
> > +             .prop           = "capsule-key",
> > +     };
>
> Why is capsule_key needed twice here? What would be the effect of:
>
> DT_NON_COMPLIANT_PURGE(voodoo) = {
>         .node_path      = "/signature",
>         .prop           = "capsule-key",
> };

In your above example, voodoo just happens to be the name of the entry
that will be created. But the property that will be searched in the
devicetree for removal is "capsule-key".

>
> > +
> > +In the first example, the entire node with path /fwu-mdata will be
> > +removed. In the second example, the property capsule-key
> > +under /signature node will be removed.
> > +
> > +Similarly, a list of nodes and properties can be specified using the
> > +following macro::
> > +
> > +     DT_NON_COMPLIANT_PURGE_LIST(foo) = {
>
> What does foo refer to here? Can this parameter be eliminated from the
> syntax?

foo is the name of the entry being generated through the
ll_entry_declare macro. We do need this parameter for ll_entry_declare
to generate a unique entry. Using a common name results in a linker
error.

>
> Will the application of the removals be sorted alphabetically by this
> argument?

I am not sure how the linker arranges the entries in the list. Why
does it matter?

>
> What will happen if a property or node does not exist at time of
> removal? Hopefully that does not stop the boot process but is silently
> ignored.

Yes, it will not cause the boot to fail.

>
> > +             { .node_path = "/some_node", .prop = "some_bar" },
> > +             { .node_path = "/some_node" },
> > +     };
>
> Why do you need the first element in your example?
> Wouldn't deleting /some/node imply that all its children are deleted?

Ah, this is a bad example. I should be using a different name for the
second entry here. Will fix.

>
> Why do we need separate properties .node_path and .prop. It would be
> less effort to write:
>
> DT_NON_COMPLIANT_PURGE_LIST() =
> {
>         "/some_node_1/some_property_1",
>         "/some_node_1/some_property_2",
>         "/some_node_2",
> }
>
> I assume the macro does its job irrespective of non-compliance.
> Why not call it DT_PURGE()?

Yes, it can be called DT_PURGE. I was using the term non_compliant
simply to highlight the fact that we would need to use this for
removal of non-compliant nodes or properties.

-sughosh

>
> Best regards
>
> Heinrich
>
> > +
> > +[1] - https://github.com/devicetree-org/dt-schema
> > +[2] - https://www.arm.com/architecture/system-architectures/systemready-certification-program
> > +[3] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/

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.

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

> +
> +Removing nodes/properties
> +-------------------------
> +
> +In U-Boot, this is been done through adding information on such nodes
> +and properties in a list. The entire node can be deleted, or a
> +specific property under a node can be deleted. The list of such nodes
> +and properties is generated at compile time, and the function to purge
> +these can be invoked through a EVT_FT_FIXUP event notify call.
> +
> +For deleting a node, this can be done by declaring a macro::
> +
> +       DT_NON_COMPLIANT_PURGE(fwu_mdata) = {
> +               .node_path      = "/fwu-mdata",
> +       };
> +
> +Similarly, for deleting a property under a node, that can be done by
> +specifying the property name::
> +
> +       DT_NON_COMPLIANT_PURGE(capsule_key) = {
> +               .node_path      = "/signature",
> +               .prop           = "capsule-key",
> +       };
> +
> +In the first example, the entire node with path /fwu-mdata will be
> +removed. In the second example, the property capsule-key
> +under /signature node will be removed.
> +
> +Similarly, a list of nodes and properties can be specified using the
> +following macro::
> +
> +       DT_NON_COMPLIANT_PURGE_LIST(foo) = {
> +               { .node_path = "/some_node", .prop = "some_bar" },
> +               { .node_path = "/some_node" },
> +       };
> +
> +[1] - https://github.com/devicetree-org/dt-schema
> +[2] - https://www.arm.com/architecture/system-architectures/systemready-certification-program
> +[3] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/
> --
> 2.34.1
>

Regards,
Simon

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

>
> > +
> > +Removing nodes/properties
> > +-------------------------
> > +
> > +In U-Boot, this is been done through adding information on such nodes
> > +and properties in a list. The entire node can be deleted, or a
> > +specific property under a node can be deleted. The list of such nodes
> > +and properties is generated at compile time, and the function to purge
> > +these can be invoked through a EVT_FT_FIXUP event notify call.
> > +
> > +For deleting a node, this can be done by declaring a macro::
> > +
> > +       DT_NON_COMPLIANT_PURGE(fwu_mdata) = {
> > +               .node_path      = "/fwu-mdata",
> > +       };
> > +
> > +Similarly, for deleting a property under a node, that can be done by
> > +specifying the property name::
> > +
> > +       DT_NON_COMPLIANT_PURGE(capsule_key) = {
> > +               .node_path      = "/signature",
> > +               .prop           = "capsule-key",
> > +       };
> > +
> > +In the first example, the entire node with path /fwu-mdata will be
> > +removed. In the second example, the property capsule-key
> > +under /signature node will be removed.
> > +
> > +Similarly, a list of nodes and properties can be specified using the
> > +following macro::
> > +
> > +       DT_NON_COMPLIANT_PURGE_LIST(foo) = {
> > +               { .node_path = "/some_node", .prop = "some_bar" },
> > +               { .node_path = "/some_node" },
> > +       };
> > +
> > +[1] - https://github.com/devicetree-org/dt-schema
> > +[2] - https://www.arm.com/architecture/system-architectures/systemready-certification-program
> > +[3] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/
> > --
> > 2.34.1
> >
>
> Regards,
> Simon

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.

Hi Sughosh,

On Mon, 28 Aug 2023 at 12:35, Sughosh Ganu <sughosh.ganu@linaro.org> 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].

That is not relevant though...we need to make sure all the nodes are
in the dt schema.

It is misleading because you imply that DT should only describe
hardware. That is not true.

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

It looks like it should go in /options/u-boot ? Can you resubmit it there?

The stripping is a no-go for me, sorry. It is absolutely going to
destroy any chance of tidying up DT in U-Boot.

Please also figure out how to add DT validation to U-Boot, so we can
see what the gap is. Once we have that in, I will be happier to do
workarounds.

Regards,
Simon

>
> -sughosh
>
> [1] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/#t
>
> >
> > > +
> > > +Removing nodes/properties
> > > +-------------------------
> > > +
> > > +In U-Boot, this is been done through adding information on such nodes
> > > +and properties in a list. The entire node can be deleted, or a
> > > +specific property under a node can be deleted. The list of such nodes
> > > +and properties is generated at compile time, and the function to purge
> > > +these can be invoked through a EVT_FT_FIXUP event notify call.
> > > +
> > > +For deleting a node, this can be done by declaring a macro::
> > > +
> > > +       DT_NON_COMPLIANT_PURGE(fwu_mdata) = {
> > > +               .node_path      = "/fwu-mdata",
> > > +       };
> > > +
> > > +Similarly, for deleting a property under a node, that can be done by
> > > +specifying the property name::
> > > +
> > > +       DT_NON_COMPLIANT_PURGE(capsule_key) = {
> > > +               .node_path      = "/signature",
> > > +               .prop           = "capsule-key",
> > > +       };
> > > +
> > > +In the first example, the entire node with path /fwu-mdata will be
> > > +removed. In the second example, the property capsule-key
> > > +under /signature node will be removed.
> > > +
> > > +Similarly, a list of nodes and properties can be specified using the
> > > +following macro::
> > > +
> > > +       DT_NON_COMPLIANT_PURGE_LIST(foo) = {
> > > +               { .node_path = "/some_node", .prop = "some_bar" },
> > > +               { .node_path = "/some_node" },
> > > +       };
> > > +
> > > +[1] - https://github.com/devicetree-org/dt-schema
> > > +[2] - https://www.arm.com/architecture/system-architectures/systemready-certification-program
> > > +[3] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/
> > > --
> > > 2.34.1
> > >
> >
> > Regards,
> > Simon

hi Simon,

On Tue, 29 Aug 2023 at 22:55, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 28 Aug 2023 at 12:35, Sughosh Ganu <sughosh.ganu@linaro.org> 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].
>
> That is not relevant though...we need to make sure all the nodes are
> in the dt schema.
>
> It is misleading because you imply that DT should only describe
> hardware. That is not true.
>
> >
> > >
> > > > +
> > > > +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.
>
> It looks like it should go in /options/u-boot ? Can you resubmit it there?

Okay. I will try this approach. If I face any hurdles, or the
devicetree maintainers have a different take on this, I will let you
know.

-sughosh

>
> The stripping is a no-go for me, sorry. It is absolutely going to
> destroy any chance of tidying up DT in U-Boot.
>
> Please also figure out how to add DT validation to U-Boot, so we can
> see what the gap is. Once we have that in, I will be happier to do
> workarounds.
>
> Regards,
> Simon
>
> >
> > -sughosh
> >
> > [1] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/#t
> >
> > >
> > > > +
> > > > +Removing nodes/properties
> > > > +-------------------------
> > > > +
> > > > +In U-Boot, this is been done through adding information on such nodes
> > > > +and properties in a list. The entire node can be deleted, or a
> > > > +specific property under a node can be deleted. The list of such nodes
> > > > +and properties is generated at compile time, and the function to purge
> > > > +these can be invoked through a EVT_FT_FIXUP event notify call.
> > > > +
> > > > +For deleting a node, this can be done by declaring a macro::
> > > > +
> > > > +       DT_NON_COMPLIANT_PURGE(fwu_mdata) = {
> > > > +               .node_path      = "/fwu-mdata",
> > > > +       };
> > > > +
> > > > +Similarly, for deleting a property under a node, that can be done by
> > > > +specifying the property name::
> > > > +
> > > > +       DT_NON_COMPLIANT_PURGE(capsule_key) = {
> > > > +               .node_path      = "/signature",
> > > > +               .prop           = "capsule-key",
> > > > +       };
> > > > +
> > > > +In the first example, the entire node with path /fwu-mdata will be
> > > > +removed. In the second example, the property capsule-key
> > > > +under /signature node will be removed.
> > > > +
> > > > +Similarly, a list of nodes and properties can be specified using the
> > > > +following macro::
> > > > +
> > > > +       DT_NON_COMPLIANT_PURGE_LIST(foo) = {
> > > > +               { .node_path = "/some_node", .prop = "some_bar" },
> > > > +               { .node_path = "/some_node" },
> > > > +       };
> > > > +
> > > > +[1] - https://github.com/devicetree-org/dt-schema
> > > > +[2] - https://www.arm.com/architecture/system-architectures/systemready-certification-program
> > > > +[3] - https://lore.kernel.org/u-boot/CAL_JsqJN4FeHomL7z3yj0WJ9bpx1oSE7zf26L_GV2oS6cg-5qg@mail.gmail.com/
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > Regards,
> > > Simon

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.

Thanks
/Ilias
>
> --
> Tom

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.

Hi Ilias,

On Wed, 30 Aug 2023 at 01:25, Ilias Apalodimas
<ilias.apalodimas@linaro.org> 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.

Isn't that controlled by ARM/Linaros, as are the devicetree bindings?
What am I missing? Let's just fix the bindings so they can be
accepted. What we decide here will have enormous repercussions in the
future. SoC vendors are watching to see whether they should bother to
upstream bindings or not.

>
> Yes, it can't be footnotes or hidden links, but this is totally
> different than what I am reading on this thread.

Regards,
Simon

Hi Simon,

On Thu, 31 Aug 2023 at 05:50, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 30 Aug 2023 at 01:25, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> 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:

[...]

> > > >
> > > > 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.
>
> Isn't that controlled by ARM/Linaros, as are the devicetree bindings?

The device tree validation happens through the dt schema validation.
You can read a bit more here [0]

> What am I missing? Let's just fix the bindings so they can be
> accepted.

I can go through the mailing lists, but I am pretty sure you've been
trying to do this for a number of years and got pushback for many of
those bindings.  Has that changed?  Can you guarantee to all the
vendors that all the bindings will get merged and they won't have a
problem getting their boards certified?

> What we decide here will have enormous repercussions in the
> future. SoC vendors are watching to see whether they should bother to
> upstream bindings or not.

How is that remotely relevant to the discussion we have here?  If it's
hardware that the OS has to use they *will* have to upstream it,
otherwise their device won't work with upstream kernels (not that they
care, I am just pointing it out).  If it's an internal thing they need
to perform an X action in U-Boot and the OS has no interest in that
property  (e.g. the driver model nodes), they can just get rid of it.

>
> >
> > Yes, it can't be footnotes or hidden links, but this is totally
> > different than what I am reading on this thread.
>
> Regards,
> Simon

[0] https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/

Thanks
/Ilias

On Thu, Aug 31, 2023 at 10:52:04AM +0300, Ilias Apalodimas wrote:
> Hi Simon,
> 
> On Thu, 31 Aug 2023 at 05:50, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 30 Aug 2023 at 01:25, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> 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:
> 
> [...]
> 
> > > > >
> > > > > 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.
> >
> > Isn't that controlled by ARM/Linaros, as are the devicetree bindings?
> 
> The device tree validation happens through the dt schema validation.
> You can read a bit more here [0]
> 
> > What am I missing? Let's just fix the bindings so they can be
> > accepted.
> 
> I can go through the mailing lists, but I am pretty sure you've been
> trying to do this for a number of years and got pushback for many of
> those bindings.  Has that changed?

Yes, yes it has changed. It has changed for the better, and we're
all working together to get things accepted.  Hence Simon's big concern
that adding the framework to delete things from the tree easily will
cause people to stop trying.

> Can you guarantee to all the
> vendors that all the bindings will get merged and they won't have a
> problem getting their boards certified?

It certainly seems that, ahem, no reasonable binding will be refused.