Message ID | 20230826090633.239342-6-sughosh.ganu@linaro.org |
---|---|
State | New |
Headers | show |
Series | Allow for removal of DT nodes and properties | expand |
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.
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 +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 +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]. + +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/
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