[RFC PATCH 0/5] Allow for removal of DT nodes and properties

[RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Sughosh Ganu]

Provide a way for removing certain devicetree nodes and/or properties
from the devicetree. This is needed to purge certain nodes and
properties which may be relevant only in U-Boot. Such nodes and
properties are then removed from the devicetree before it is passed to
the kernel. This ensures that the devicetree passed to the OS does not
contain any non-compliant nodes and properties.

The removal of the nodes and properties is being done through an
EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
behind any Kconfig symbol.

I have only build tested this on sandbox, and tested on qemu arm64
virt platform. This being a RFC, I have not put this through a CI run.

Sughosh Ganu (5):
  dt: Provide a way to remove non-compliant nodes and properties
  fwu: Add the fwu-mdata node for removal from devicetree
  capsule: Add the capsule-key property for removal from devicetree
  bootefi: Call the EVT_FT_FIXUP event handler
  doc: Add a document for non-compliant DT node/property removal

 cmd/bootefi.c                                 | 18 +++++
 .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
 drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
 include/dt-structs.h                          | 11 +++
 lib/Makefile                                  |  1 +
 lib/dt_purge.c                                | 73 +++++++++++++++++++
 lib/efi_loader/efi_capsule.c                  |  7 ++
 7 files changed, 179 insertions(+)
 create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
 create mode 100644 lib/dt_purge.c

-- 
2.34.1

[RFC PATCH 1/5] dt: Provide a way to remove non-compliant nodes and properties

[Sughosh Ganu]

Add a function which is registered to spy for a EVT_FT_FIXUP event,
and removes the non upstreamed nodes and properties from the
devicetree before it gets passed to the OS.

This allows removing entire nodes, or specific properties under nodes
from the devicetree. The required nodes and properties can be
registered for removal through the DT_NON_COMPLIANT_PURGE and
DT_NON_COMPLIANT_PURGE_LIST macros.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/dt-structs.h | 11 +++++++
 lib/Makefile         |  1 +
 lib/dt_purge.c       | 73 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)
 create mode 100644 lib/dt_purge.c

diff --git a/include/dt-structs.h b/include/dt-structs.h
index fa1622cb1d..f535c60471 100644
--- a/include/dt-structs.h
+++ b/include/dt-structs.h
@@ -57,3 +57,14 @@ struct phandle_2_arg {
 #endif
 
 #endif
+
+struct dt_non_compliant_purge {
+	const char *node_path;
+	const char *prop;
+};
+
+#define DT_NON_COMPLIANT_PURGE(__name)	\
+	ll_entry_declare(struct dt_non_compliant_purge, __name, dt_purge)
+
+#define DT_NON_COMPLIANT_PURGE_LIST(__name)	\
+	ll_entry_declare_list(struct dt_non_compliant_purge, __name, dt_purge)
diff --git a/lib/Makefile b/lib/Makefile
index 8d8ccc8bbc..82a906daa0 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -37,6 +37,7 @@ endif
 obj-y += crc8.o
 obj-y += crc16.o
 obj-y += crc16-ccitt.o
+obj-y += dt_purge.o
 obj-$(CONFIG_ERRNO_STR) += errno_str.o
 obj-$(CONFIG_FIT) += fdtdec_common.o
 obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
diff --git a/lib/dt_purge.c b/lib/dt_purge.c
new file mode 100644
index 0000000000..f893ba9796
--- /dev/null
+++ b/lib/dt_purge.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <dt-structs.h>
+#include <event.h>
+#include <linker_lists.h>
+
+#include <linux/libfdt.h>
+
+/**
+ * dt_non_compliant_purge() -	Remove non-upstreamed nodes and properties
+ *				from the DT
+ * @ctx: Context for event
+ * @event: Event to process
+ *
+ * Iterate through an array of DT nodes and properties, and remove them
+ * from the device-tree before the DT gets handed over to the kernel.
+ * These are nodes and properties which do not have upstream bindings
+ * and need to be purged before being handed over to the kernel.
+ *
+ * If both the node and property are specified, delete the property. If
+ * only the node is specified, delete the entire node, including it's
+ * subnodes, if any.
+ *
+ * Return: 0 if OK, -ve on error
+ */
+static int dt_non_compliant_purge(void *ctx, struct event *event)
+{
+	int nodeoff = 0;
+	int err = 0;
+	void *fdt;
+	const struct event_ft_fixup *fixup = &event->data.ft_fixup;
+	struct dt_non_compliant_purge *purge_entry;
+	struct dt_non_compliant_purge *purge_start =
+		ll_entry_start(struct dt_non_compliant_purge, dt_purge);
+	int nentries = ll_entry_count(struct dt_non_compliant_purge, dt_purge);
+
+	if (fixup->images)
+		return 0;
+
+	fdt = fixup->tree.fdt;
+	for (purge_entry = purge_start; purge_entry != purge_start + nentries;
+	     purge_entry++) {
+		nodeoff = fdt_path_offset(fdt, purge_entry->node_path);
+		if (nodeoff < 0) {
+			log_debug("Error (%d) getting node offset for %s\n",
+				  nodeoff, purge_entry->node_path);
+			continue;
+		}
+
+		if (purge_entry->prop) {
+			err = fdt_delprop(fdt, nodeoff, purge_entry->prop);
+			if (err < 0 && err != -FDT_ERR_NOTFOUND) {
+				log_debug("Error (%d) deleting %s\n",
+					  err, purge_entry->prop);
+				goto out;
+			}
+		} else {
+			err = fdt_del_node(fdt, nodeoff);
+			if (err) {
+				log_debug("Error (%d) trying to delete node %s\n",
+					  err, purge_entry->node_path);
+				goto out;
+			}
+		}
+	}
+
+out:
+	return err;
+}
+EVENT_SPY(EVT_FT_FIXUP, dt_non_compliant_purge);
-- 
2.34.1

[RFC PATCH 2/5] fwu: Add the fwu-mdata node for removal from devicetree

[Sughosh Ganu]

The FWU metadata devicetree node points to the device which stores the
metadata structure. This node is relevant only in U-Boot, and is not
to be passed to the OS. Register for purging this node from the
devicetree, before passing it to the OS.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 drivers/fwu-mdata/fwu-mdata-uclass.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
index 0a8edaaa41..71411d9c19 100644
--- a/drivers/fwu-mdata/fwu-mdata-uclass.c
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -7,6 +7,7 @@
 
 #include <common.h>
 #include <dm.h>
+#include <dt-structs.h>
 #include <efi_loader.h>
 #include <fwu.h>
 #include <fwu_mdata.h>
@@ -53,3 +54,7 @@ UCLASS_DRIVER(fwu_mdata) = {
 	.id		= UCLASS_FWU_MDATA,
 	.name		= "fwu-mdata",
 };
+
+DT_NON_COMPLIANT_PURGE(fwu_mdata) = {
+	.node_path	= "/fwu-mdata",
+};
-- 
2.34.1

[RFC PATCH 3/5] capsule: Add the capsule-key property for removal from devicetree

[Sughosh Ganu]

The capsule-key property contains the public key in the form of an EFI
Signature List(ESL) structure. This property is relevant only in
U-Boot, and is not to be passed to the OS. Register for purging this
property from the devicetree, before passing it to the OS.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 lib/efi_loader/efi_capsule.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index af8a2ee940..5c6c87458f 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -9,6 +9,7 @@
 #define LOG_CATEGORY LOGC_EFI
 
 #include <common.h>
+#include <dt-structs.h>
 #include <efi_loader.h>
 #include <efi_variable.h>
 #include <env.h>
@@ -403,6 +404,12 @@ out:
 
 	return status;
 }
+
+DT_NON_COMPLIANT_PURGE(capsule_key) = {
+	.node_path	= "/signature",
+	.prop		= "capsule-key",
+};
+
 #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
 
 static __maybe_unused bool fwu_empty_capsule(struct efi_capsule_header *capsule)
-- 
2.34.1

[RFC PATCH 4/5] bootefi: Call the EVT_FT_FIXUP event handler

[Sughosh Ganu]

The bootefi command passes the devicetree to the kernel through the
EFI config table. Call the event handlers for fixing the devicetree
before jumping into the kernel. This removes any devicetree nodes
and/or properties that are specific only to U-Boot, and are not to be
passed to the OS.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 cmd/bootefi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index f73d6eb0e2..c359a46ec4 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -237,6 +237,23 @@ static void *get_config_table(const efi_guid_t *guid)
 	return NULL;
 }
 
+/**
+ * event_notify_dt_fixup() - call ft_fixup event
+ *
+ * @fdt:	address of the device tree to be passed to the kernel
+ *		through the configuration table
+ * Return:	None
+ */
+static void event_notify_dt_fixup(void *fdt)
+{
+	int ret;
+	struct event_ft_fixup fixup = {0};
+
+	fixup.tree.fdt = fdt;
+	ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
+	if (ret)
+		printf("Error: %d: FDT Fixup event failed\n", ret);
+}
 #endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
 
 /**
@@ -318,6 +335,7 @@ efi_status_t efi_install_fdt(void *fdt)
 	efi_carve_out_dt_rsv(fdt);
 
 	efi_try_purge_kaslr_seed(fdt);
+	event_notify_dt_fixup(fdt);
 
 	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
 		ret = efi_tcg2_measure_dtb(fdt);
-- 
2.34.1

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

[Sughosh Ganu]

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

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

[Heinrich Schuchardt]

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/

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Heinrich Schuchardt]

On 8/26/23 11:06, Sughosh Ganu wrote:
>
> Provide a way for removing certain devicetree nodes and/or properties
> from the devicetree. This is needed to purge certain nodes and
> properties which may be relevant only in U-Boot. Such nodes and
> properties are then removed from the devicetree before it is passed to
> the kernel. This ensures that the devicetree passed to the OS does not
> contain any non-compliant nodes and properties.
>
> The removal of the nodes and properties is being done through an
> EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> behind any Kconfig symbol.
>
> I have only build tested this on sandbox, and tested on qemu arm64
> virt platform. This being a RFC, I have not put this through a CI run.

We should have a test for the new functionality. E.g. add some
superfluous properties and nodes to arch/sandbox/dts/test.dts, delete
them via the DT_PURGE macro, and check that the device-tree passed to an
EFI binary does not contain these properties and nodes.

Best regards

Heinrich

>
> Sughosh Ganu (5):
>    dt: Provide a way to remove non-compliant nodes and properties
>    fwu: Add the fwu-mdata node for removal from devicetree
>    capsule: Add the capsule-key property for removal from devicetree
>    bootefi: Call the EVT_FT_FIXUP event handler
>    doc: Add a document for non-compliant DT node/property removal
>
>   cmd/bootefi.c                                 | 18 +++++
>   .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
>   drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
>   include/dt-structs.h                          | 11 +++
>   lib/Makefile                                  |  1 +
>   lib/dt_purge.c                                | 73 +++++++++++++++++++
>   lib/efi_loader/efi_capsule.c                  |  7 ++
>   7 files changed, 179 insertions(+)
>   create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
>   create mode 100644 lib/dt_purge.c
>

Re: [RFC PATCH 1/5] dt: Provide a way to remove non-compliant nodes and properties

[Heinrich Schuchardt]

On 8/26/23 11:06, Sughosh Ganu wrote:
> Add a function which is registered to spy for a EVT_FT_FIXUP event,
> and removes the non upstreamed nodes and properties from the
> devicetree before it gets passed to the OS.
>
> This allows removing entire nodes, or specific properties under nodes
> from the devicetree. The required nodes and properties can be
> registered for removal through the DT_NON_COMPLIANT_PURGE and
> DT_NON_COMPLIANT_PURGE_LIST macros.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   include/dt-structs.h | 11 +++++++
>   lib/Makefile         |  1 +
>   lib/dt_purge.c       | 73 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 85 insertions(+)
>   create mode 100644 lib/dt_purge.c
>
> diff --git a/include/dt-structs.h b/include/dt-structs.h
> index fa1622cb1d..f535c60471 100644
> --- a/include/dt-structs.h
> +++ b/include/dt-structs.h
> @@ -57,3 +57,14 @@ struct phandle_2_arg {
>   #endif
>
>   #endif
> +
> +struct dt_non_compliant_purge {
> +	const char *node_path;
> +	const char *prop;
> +};
> +
> +#define DT_NON_COMPLIANT_PURGE(__name)	\
> +	ll_entry_declare(struct dt_non_compliant_purge, __name, dt_purge)
> +
> +#define DT_NON_COMPLIANT_PURGE_LIST(__name)	\
> +	ll_entry_declare_list(struct dt_non_compliant_purge, __name, dt_purge)
> diff --git a/lib/Makefile b/lib/Makefile
> index 8d8ccc8bbc..82a906daa0 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -37,6 +37,7 @@ endif
>   obj-y += crc8.o
>   obj-y += crc16.o
>   obj-y += crc16-ccitt.o
> +obj-y += dt_purge.o
>   obj-$(CONFIG_ERRNO_STR) += errno_str.o
>   obj-$(CONFIG_FIT) += fdtdec_common.o
>   obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
> diff --git a/lib/dt_purge.c b/lib/dt_purge.c
> new file mode 100644
> index 0000000000..f893ba9796
> --- /dev/null
> +++ b/lib/dt_purge.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <dt-structs.h>
> +#include <event.h>
> +#include <linker_lists.h>
> +
> +#include <linux/libfdt.h>
> +
> +/**
> + * dt_non_compliant_purge() -	Remove non-upstreamed nodes and properties
> + *				from the DT
> + * @ctx: Context for event
> + * @event: Event to process
> + *
> + * Iterate through an array of DT nodes and properties, and remove them
> + * from the device-tree before the DT gets handed over to the kernel.
> + * These are nodes and properties which do not have upstream bindings
> + * and need to be purged before being handed over to the kernel.
> + *
> + * If both the node and property are specified, delete the property. If
> + * only the node is specified, delete the entire node, including it's
> + * subnodes, if any.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +static int dt_non_compliant_purge(void *ctx, struct event *event)
> +{
> +	int nodeoff = 0;
> +	int err = 0;
> +	void *fdt;
> +	const struct event_ft_fixup *fixup = &event->data.ft_fixup;
> +	struct dt_non_compliant_purge *purge_entry;
> +	struct dt_non_compliant_purge *purge_start =
> +		ll_entry_start(struct dt_non_compliant_purge, dt_purge);
> +	int nentries = ll_entry_count(struct dt_non_compliant_purge, dt_purge);
> +
> +	if (fixup->images)
> +		return 0;
> +
> +	fdt = fixup->tree.fdt;
> +	for (purge_entry = purge_start; purge_entry != purge_start + nentries;
> +	     purge_entry++) {
> +		nodeoff = fdt_path_offset(fdt, purge_entry->node_path);
> +		if (nodeoff < 0) {
> +			log_debug("Error (%d) getting node offset for %s\n",
> +				  nodeoff, purge_entry->node_path);
> +			continue;
> +		}
> +
> +		if (purge_entry->prop) {
> +			err = fdt_delprop(fdt, nodeoff, purge_entry->prop);
> +			if (err < 0 && err != -FDT_ERR_NOTFOUND) {
> +				log_debug("Error (%d) deleting %s\n",
> +					  err, purge_entry->prop);
> +				goto out;
> +			}
> +		} else {
> +			err = fdt_del_node(fdt, nodeoff);
> +			if (err) {
> +				log_debug("Error (%d) trying to delete node %s\n",
> +					  err, purge_entry->node_path);
> +				goto out;
> +			}
> +		}
> +	}
> +
> +out:
> +	return err;
> +}
> +EVENT_SPY(EVT_FT_FIXUP, dt_non_compliant_purge);

This may interfere with other fixup code requiring U-Boot specific
properties like bootmeth_vbe_ft_fixup().

We should ensure that call dt_non_compliant_purge() is the last fixup
action. Please, consider a separate event.

This code does not check if the property or node complies to the device
tree scheme or not. So why not call the function dt_purge()?

Best regards

Heinrich

Re: [RFC PATCH 4/5] bootefi: Call the EVT_FT_FIXUP event handler

[Heinrich Schuchardt]

On 8/26/23 11:06, Sughosh Ganu wrote:
> The bootefi command passes the devicetree to the kernel through the
> EFI config table. Call the event handlers for fixing the devicetree
> before jumping into the kernel. This removes any devicetree nodes
> and/or properties that are specific only to U-Boot, and are not to be
> passed to the OS.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   cmd/bootefi.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index f73d6eb0e2..c359a46ec4 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -237,6 +237,23 @@ static void *get_config_table(const efi_guid_t *guid)
>   	return NULL;
>   }
>
> +/**
> + * event_notify_dt_fixup() - call ft_fixup event
> + *
> + * @fdt:	address of the device tree to be passed to the kernel
> + *		through the configuration table
> + * Return:	None
> + */
> +static void event_notify_dt_fixup(void *fdt)
> +{
> +	int ret;
> +	struct event_ft_fixup fixup = {0};
> +
> +	fixup.tree.fdt = fdt;
> +	ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
> +	if (ret)
> +		printf("Error: %d: FDT Fixup event failed\n", ret);
> +}
>   #endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
>
>   /**
> @@ -318,6 +335,7 @@ efi_status_t efi_install_fdt(void *fdt)
>   	efi_carve_out_dt_rsv(fdt);
>
>   	efi_try_purge_kaslr_seed(fdt);
> +	event_notify_dt_fixup(fdt);

The event is already triggered in image_setup_libfdt(). Don't trigger it
twice.

Best regards

Heinrich

>
>   	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
>   		ret = efi_tcg2_measure_dtb(fdt);

Re: [RFC PATCH 1/5] dt: Provide a way to remove non-compliant nodes and properties

[Heinrich Schuchardt]

On 8/26/23 11:06, Sughosh Ganu wrote:
> Add a function which is registered to spy for a EVT_FT_FIXUP event,
> and removes the non upstreamed nodes and properties from the
> devicetree before it gets passed to the OS.
>
> This allows removing entire nodes, or specific properties under nodes
> from the devicetree. The required nodes and properties can be
> registered for removal through the DT_NON_COMPLIANT_PURGE and
> DT_NON_COMPLIANT_PURGE_LIST macros.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   include/dt-structs.h | 11 +++++++
>   lib/Makefile         |  1 +
>   lib/dt_purge.c       | 73 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 85 insertions(+)
>   create mode 100644 lib/dt_purge.c
>
> diff --git a/include/dt-structs.h b/include/dt-structs.h
> index fa1622cb1d..f535c60471 100644
> --- a/include/dt-structs.h
> +++ b/include/dt-structs.h
> @@ -57,3 +57,14 @@ struct phandle_2_arg {
>   #endif
>
>   #endif
> +
> +struct dt_non_compliant_purge {
> +	const char *node_path;
> +	const char *prop;
> +};
> +
> +#define DT_NON_COMPLIANT_PURGE(__name)	\
> +	ll_entry_declare(struct dt_non_compliant_purge, __name, dt_purge)
> +
> +#define DT_NON_COMPLIANT_PURGE_LIST(__name)	\
> +	ll_entry_declare_list(struct dt_non_compliant_purge, __name, dt_purge)
> diff --git a/lib/Makefile b/lib/Makefile
> index 8d8ccc8bbc..82a906daa0 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -37,6 +37,7 @@ endif
>   obj-y += crc8.o
>   obj-y += crc16.o
>   obj-y += crc16-ccitt.o
> +obj-y += dt_purge.o

SPL can be the last boot stage (e.g. for Falcon Mode). So placing this
under 'ifndef CONFIG_SPL_BUILD' is not correct.

You need some logic that identifies into which boot stage this code
belongs, e.g. use obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT).

Best regards

Heinrich

>   obj-$(CONFIG_ERRNO_STR) += errno_str.o
>   obj-$(CONFIG_FIT) += fdtdec_common.o
>   obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
> diff --git a/lib/dt_purge.c b/lib/dt_purge.c
> new file mode 100644
> index 0000000000..f893ba9796
> --- /dev/null
> +++ b/lib/dt_purge.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <dt-structs.h>
> +#include <event.h>
> +#include <linker_lists.h>
> +
> +#include <linux/libfdt.h>
> +
> +/**
> + * dt_non_compliant_purge() -	Remove non-upstreamed nodes and properties
> + *				from the DT
> + * @ctx: Context for event
> + * @event: Event to process
> + *
> + * Iterate through an array of DT nodes and properties, and remove them
> + * from the device-tree before the DT gets handed over to the kernel.
> + * These are nodes and properties which do not have upstream bindings
> + * and need to be purged before being handed over to the kernel.
> + *
> + * If both the node and property are specified, delete the property. If
> + * only the node is specified, delete the entire node, including it's
> + * subnodes, if any.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +static int dt_non_compliant_purge(void *ctx, struct event *event)
> +{
> +	int nodeoff = 0;
> +	int err = 0;
> +	void *fdt;
> +	const struct event_ft_fixup *fixup = &event->data.ft_fixup;
> +	struct dt_non_compliant_purge *purge_entry;
> +	struct dt_non_compliant_purge *purge_start =
> +		ll_entry_start(struct dt_non_compliant_purge, dt_purge);
> +	int nentries = ll_entry_count(struct dt_non_compliant_purge, dt_purge);
> +
> +	if (fixup->images)
> +		return 0;
> +
> +	fdt = fixup->tree.fdt;
> +	for (purge_entry = purge_start; purge_entry != purge_start + nentries;
> +	     purge_entry++) {
> +		nodeoff = fdt_path_offset(fdt, purge_entry->node_path);
> +		if (nodeoff < 0) {
> +			log_debug("Error (%d) getting node offset for %s\n",
> +				  nodeoff, purge_entry->node_path);
> +			continue;
> +		}
> +
> +		if (purge_entry->prop) {
> +			err = fdt_delprop(fdt, nodeoff, purge_entry->prop);
> +			if (err < 0 && err != -FDT_ERR_NOTFOUND) {
> +				log_debug("Error (%d) deleting %s\n",
> +					  err, purge_entry->prop);
> +				goto out;
> +			}
> +		} else {
> +			err = fdt_del_node(fdt, nodeoff);
> +			if (err) {
> +				log_debug("Error (%d) trying to delete node %s\n",
> +					  err, purge_entry->node_path);
> +				goto out;
> +			}
> +		}
> +	}
> +
> +out:
> +	return err;
> +}
> +EVENT_SPY(EVT_FT_FIXUP, dt_non_compliant_purge);

Re: [RFC PATCH 1/5] dt: Provide a way to remove non-compliant nodes and properties

[Sughosh Ganu]

On Sat, 26 Aug 2023 at 15:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 8/26/23 11:06, Sughosh Ganu wrote:
> > Add a function which is registered to spy for a EVT_FT_FIXUP event,
> > and removes the non upstreamed nodes and properties from the
> > devicetree before it gets passed to the OS.
> >
> > This allows removing entire nodes, or specific properties under nodes
> > from the devicetree. The required nodes and properties can be
> > registered for removal through the DT_NON_COMPLIANT_PURGE and
> > DT_NON_COMPLIANT_PURGE_LIST macros.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   include/dt-structs.h | 11 +++++++
> >   lib/Makefile         |  1 +
> >   lib/dt_purge.c       | 73 ++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 85 insertions(+)
> >   create mode 100644 lib/dt_purge.c
> >
> > diff --git a/include/dt-structs.h b/include/dt-structs.h
> > index fa1622cb1d..f535c60471 100644
> > --- a/include/dt-structs.h
> > +++ b/include/dt-structs.h
> > @@ -57,3 +57,14 @@ struct phandle_2_arg {
> >   #endif
> >
> >   #endif
> > +
> > +struct dt_non_compliant_purge {
> > +     const char *node_path;
> > +     const char *prop;
> > +};
> > +
> > +#define DT_NON_COMPLIANT_PURGE(__name)       \
> > +     ll_entry_declare(struct dt_non_compliant_purge, __name, dt_purge)
> > +
> > +#define DT_NON_COMPLIANT_PURGE_LIST(__name)  \
> > +     ll_entry_declare_list(struct dt_non_compliant_purge, __name, dt_purge)
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 8d8ccc8bbc..82a906daa0 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -37,6 +37,7 @@ endif
> >   obj-y += crc8.o
> >   obj-y += crc16.o
> >   obj-y += crc16-ccitt.o
> > +obj-y += dt_purge.o
> >   obj-$(CONFIG_ERRNO_STR) += errno_str.o
> >   obj-$(CONFIG_FIT) += fdtdec_common.o
> >   obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
> > diff --git a/lib/dt_purge.c b/lib/dt_purge.c
> > new file mode 100644
> > index 0000000000..f893ba9796
> > --- /dev/null
> > +++ b/lib/dt_purge.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#include <dt-structs.h>
> > +#include <event.h>
> > +#include <linker_lists.h>
> > +
> > +#include <linux/libfdt.h>
> > +
> > +/**
> > + * dt_non_compliant_purge() -        Remove non-upstreamed nodes and properties
> > + *                           from the DT
> > + * @ctx: Context for event
> > + * @event: Event to process
> > + *
> > + * Iterate through an array of DT nodes and properties, and remove them
> > + * from the device-tree before the DT gets handed over to the kernel.
> > + * These are nodes and properties which do not have upstream bindings
> > + * and need to be purged before being handed over to the kernel.
> > + *
> > + * If both the node and property are specified, delete the property. If
> > + * only the node is specified, delete the entire node, including it's
> > + * subnodes, if any.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +static int dt_non_compliant_purge(void *ctx, struct event *event)
> > +{
> > +     int nodeoff = 0;
> > +     int err = 0;
> > +     void *fdt;
> > +     const struct event_ft_fixup *fixup = &event->data.ft_fixup;
> > +     struct dt_non_compliant_purge *purge_entry;
> > +     struct dt_non_compliant_purge *purge_start =
> > +             ll_entry_start(struct dt_non_compliant_purge, dt_purge);
> > +     int nentries = ll_entry_count(struct dt_non_compliant_purge, dt_purge);
> > +
> > +     if (fixup->images)
> > +             return 0;
> > +
> > +     fdt = fixup->tree.fdt;
> > +     for (purge_entry = purge_start; purge_entry != purge_start + nentries;
> > +          purge_entry++) {
> > +             nodeoff = fdt_path_offset(fdt, purge_entry->node_path);
> > +             if (nodeoff < 0) {
> > +                     log_debug("Error (%d) getting node offset for %s\n",
> > +                               nodeoff, purge_entry->node_path);
> > +                     continue;
> > +             }
> > +
> > +             if (purge_entry->prop) {
> > +                     err = fdt_delprop(fdt, nodeoff, purge_entry->prop);
> > +                     if (err < 0 && err != -FDT_ERR_NOTFOUND) {
> > +                             log_debug("Error (%d) deleting %s\n",
> > +                                       err, purge_entry->prop);
> > +                             goto out;
> > +                     }
> > +             } else {
> > +                     err = fdt_del_node(fdt, nodeoff);
> > +                     if (err) {
> > +                             log_debug("Error (%d) trying to delete node %s\n",
> > +                                       err, purge_entry->node_path);
> > +                             goto out;
> > +                     }
> > +             }
> > +     }
> > +
> > +out:
> > +     return err;
> > +}
> > +EVENT_SPY(EVT_FT_FIXUP, dt_non_compliant_purge);
>
> This may interfere with other fixup code requiring U-Boot specific
> properties like bootmeth_vbe_ft_fixup().

Okay, I was thinking if we can have a flag to indicate which event
handler is to be run. I will check this, and add a separate event if
this solution turns to be overly complex.

>
> We should ensure that call dt_non_compliant_purge() is the last fixup
> action. Please, consider a separate event.

Will check.

>
> This code does not check if the property or node complies to the device
> tree scheme or not. So why not call the function dt_purge()?

Okay

-sughosh

Re: [RFC PATCH 1/5] dt: Provide a way to remove non-compliant nodes and properties

[Sughosh Ganu]

On Sat, 26 Aug 2023 at 16:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/26/23 11:06, Sughosh Ganu wrote:
> > Add a function which is registered to spy for a EVT_FT_FIXUP event,
> > and removes the non upstreamed nodes and properties from the
> > devicetree before it gets passed to the OS.
> >
> > This allows removing entire nodes, or specific properties under nodes
> > from the devicetree. The required nodes and properties can be
> > registered for removal through the DT_NON_COMPLIANT_PURGE and
> > DT_NON_COMPLIANT_PURGE_LIST macros.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   include/dt-structs.h | 11 +++++++
> >   lib/Makefile         |  1 +
> >   lib/dt_purge.c       | 73 ++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 85 insertions(+)
> >   create mode 100644 lib/dt_purge.c
> >
> > diff --git a/include/dt-structs.h b/include/dt-structs.h
> > index fa1622cb1d..f535c60471 100644
> > --- a/include/dt-structs.h
> > +++ b/include/dt-structs.h
> > @@ -57,3 +57,14 @@ struct phandle_2_arg {
> >   #endif
> >
> >   #endif
> > +
> > +struct dt_non_compliant_purge {
> > +     const char *node_path;
> > +     const char *prop;
> > +};
> > +
> > +#define DT_NON_COMPLIANT_PURGE(__name)       \
> > +     ll_entry_declare(struct dt_non_compliant_purge, __name, dt_purge)
> > +
> > +#define DT_NON_COMPLIANT_PURGE_LIST(__name)  \
> > +     ll_entry_declare_list(struct dt_non_compliant_purge, __name, dt_purge)
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 8d8ccc8bbc..82a906daa0 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -37,6 +37,7 @@ endif
> >   obj-y += crc8.o
> >   obj-y += crc16.o
> >   obj-y += crc16-ccitt.o
> > +obj-y += dt_purge.o
>
> SPL can be the last boot stage (e.g. for Falcon Mode). So placing this
> under 'ifndef CONFIG_SPL_BUILD' is not correct.
>
> You need some logic that identifies into which boot stage this code
> belongs, e.g. use obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT).

Okay. Will check and add this under the suggested config symbol.

-sughosh

>
> Best regards
>
> Heinrich
>
> >   obj-$(CONFIG_ERRNO_STR) += errno_str.o
> >   obj-$(CONFIG_FIT) += fdtdec_common.o
> >   obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
> > diff --git a/lib/dt_purge.c b/lib/dt_purge.c
> > new file mode 100644
> > index 0000000000..f893ba9796
> > --- /dev/null
> > +++ b/lib/dt_purge.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#include <dt-structs.h>
> > +#include <event.h>
> > +#include <linker_lists.h>
> > +
> > +#include <linux/libfdt.h>
> > +
> > +/**
> > + * dt_non_compliant_purge() -        Remove non-upstreamed nodes and properties
> > + *                           from the DT
> > + * @ctx: Context for event
> > + * @event: Event to process
> > + *
> > + * Iterate through an array of DT nodes and properties, and remove them
> > + * from the device-tree before the DT gets handed over to the kernel.
> > + * These are nodes and properties which do not have upstream bindings
> > + * and need to be purged before being handed over to the kernel.
> > + *
> > + * If both the node and property are specified, delete the property. If
> > + * only the node is specified, delete the entire node, including it's
> > + * subnodes, if any.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +static int dt_non_compliant_purge(void *ctx, struct event *event)
> > +{
> > +     int nodeoff = 0;
> > +     int err = 0;
> > +     void *fdt;
> > +     const struct event_ft_fixup *fixup = &event->data.ft_fixup;
> > +     struct dt_non_compliant_purge *purge_entry;
> > +     struct dt_non_compliant_purge *purge_start =
> > +             ll_entry_start(struct dt_non_compliant_purge, dt_purge);
> > +     int nentries = ll_entry_count(struct dt_non_compliant_purge, dt_purge);
> > +
> > +     if (fixup->images)
> > +             return 0;
> > +
> > +     fdt = fixup->tree.fdt;
> > +     for (purge_entry = purge_start; purge_entry != purge_start + nentries;
> > +          purge_entry++) {
> > +             nodeoff = fdt_path_offset(fdt, purge_entry->node_path);
> > +             if (nodeoff < 0) {
> > +                     log_debug("Error (%d) getting node offset for %s\n",
> > +                               nodeoff, purge_entry->node_path);
> > +                     continue;
> > +             }
> > +
> > +             if (purge_entry->prop) {
> > +                     err = fdt_delprop(fdt, nodeoff, purge_entry->prop);
> > +                     if (err < 0 && err != -FDT_ERR_NOTFOUND) {
> > +                             log_debug("Error (%d) deleting %s\n",
> > +                                       err, purge_entry->prop);
> > +                             goto out;
> > +                     }
> > +             } else {
> > +                     err = fdt_del_node(fdt, nodeoff);
> > +                     if (err) {
> > +                             log_debug("Error (%d) trying to delete node %s\n",
> > +                                       err, purge_entry->node_path);
> > +                             goto out;
> > +                     }
> > +             }
> > +     }
> > +
> > +out:
> > +     return err;
> > +}
> > +EVENT_SPY(EVT_FT_FIXUP, dt_non_compliant_purge);
>

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Sughosh Ganu]

On Sat, 26 Aug 2023 at 15:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/26/23 11:06, Sughosh Ganu wrote:
> >
> > Provide a way for removing certain devicetree nodes and/or properties
> > from the devicetree. This is needed to purge certain nodes and
> > properties which may be relevant only in U-Boot. Such nodes and
> > properties are then removed from the devicetree before it is passed to
> > the kernel. This ensures that the devicetree passed to the OS does not
> > contain any non-compliant nodes and properties.
> >
> > The removal of the nodes and properties is being done through an
> > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > behind any Kconfig symbol.
> >
> > I have only build tested this on sandbox, and tested on qemu arm64
> > virt platform. This being a RFC, I have not put this through a CI run.
>
> We should have a test for the new functionality. E.g. add some
> superfluous properties and nodes to arch/sandbox/dts/test.dts, delete
> them via the DT_PURGE macro, and check that the device-tree passed to an
> EFI binary does not contain these properties and nodes.

Yes, I plan to have a test in the non-RFC version. I was thinking of
adding a command and then using that to test the functionality.

-sughosh

>
> Best regards
>
> Heinrich
>
> >
> > Sughosh Ganu (5):
> >    dt: Provide a way to remove non-compliant nodes and properties
> >    fwu: Add the fwu-mdata node for removal from devicetree
> >    capsule: Add the capsule-key property for removal from devicetree
> >    bootefi: Call the EVT_FT_FIXUP event handler
> >    doc: Add a document for non-compliant DT node/property removal
> >
> >   cmd/bootefi.c                                 | 18 +++++
> >   .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> >   drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> >   include/dt-structs.h                          | 11 +++
> >   lib/Makefile                                  |  1 +
> >   lib/dt_purge.c                                | 73 +++++++++++++++++++
> >   lib/efi_loader/efi_capsule.c                  |  7 ++
> >   7 files changed, 179 insertions(+)
> >   create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> >   create mode 100644 lib/dt_purge.c
> >

Re: [RFC PATCH 4/5] bootefi: Call the EVT_FT_FIXUP event handler

[Sughosh Ganu]

On Sat, 26 Aug 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/26/23 11:06, Sughosh Ganu wrote:
> > The bootefi command passes the devicetree to the kernel through the
> > EFI config table. Call the event handlers for fixing the devicetree
> > before jumping into the kernel. This removes any devicetree nodes
> > and/or properties that are specific only to U-Boot, and are not to be
> > passed to the OS.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   cmd/bootefi.c | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index f73d6eb0e2..c359a46ec4 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -237,6 +237,23 @@ static void *get_config_table(const efi_guid_t *guid)
> >       return NULL;
> >   }
> >
> > +/**
> > + * event_notify_dt_fixup() - call ft_fixup event
> > + *
> > + * @fdt:     address of the device tree to be passed to the kernel
> > + *           through the configuration table
> > + * Return:   None
> > + */
> > +static void event_notify_dt_fixup(void *fdt)
> > +{
> > +     int ret;
> > +     struct event_ft_fixup fixup = {0};
> > +
> > +     fixup.tree.fdt = fdt;
> > +     ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
> > +     if (ret)
> > +             printf("Error: %d: FDT Fixup event failed\n", ret);
> > +}
> >   #endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
> >
> >   /**
> > @@ -318,6 +335,7 @@ efi_status_t efi_install_fdt(void *fdt)
> >       efi_carve_out_dt_rsv(fdt);
> >
> >       efi_try_purge_kaslr_seed(fdt);
> > +     event_notify_dt_fixup(fdt);
>
> The event is already triggered in image_setup_libfdt(). Don't trigger it
> twice.

The reason I put an explicit event_notify call is because the
image_setup_libfdt() call only calls the ft_fixup handlers if the
livetree is not active. So the fixup handlers would not be called on
platforms that enable livetree. Although I'm not sure if livetree
should be disabled before the ft fixup has to happen, or platforms
that need ft fixup should not enable OF_LIVE. Disabling the livetree
is not happening now, so I am not sure how the fixup event should work
on platforms which have OF_LIVE enabled.

-sughosh

>
> Best regards
>
> Heinrich
>
> >
> >       if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> >               ret = efi_tcg2_measure_dtb(fdt);
>

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

[Sughosh Ganu]

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/

Re: [RFC PATCH 4/5] bootefi: Call the EVT_FT_FIXUP event handler

[Heinrich Schuchardt]

On 28.08.23 11:32, Sughosh Ganu wrote:
> On Sat, 26 Aug 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 8/26/23 11:06, Sughosh Ganu wrote:
>>> The bootefi command passes the devicetree to the kernel through the
>>> EFI config table. Call the event handlers for fixing the devicetree
>>> before jumping into the kernel. This removes any devicetree nodes
>>> and/or properties that are specific only to U-Boot, and are not to be
>>> passed to the OS.
>>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>> ---
>>>    cmd/bootefi.c | 18 ++++++++++++++++++
>>>    1 file changed, 18 insertions(+)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index f73d6eb0e2..c359a46ec4 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -237,6 +237,23 @@ static void *get_config_table(const efi_guid_t *guid)
>>>        return NULL;
>>>    }
>>>
>>> +/**
>>> + * event_notify_dt_fixup() - call ft_fixup event
>>> + *
>>> + * @fdt:     address of the device tree to be passed to the kernel
>>> + *           through the configuration table
>>> + * Return:   None
>>> + */
>>> +static void event_notify_dt_fixup(void *fdt)
>>> +{
>>> +     int ret;
>>> +     struct event_ft_fixup fixup = {0};
>>> +
>>> +     fixup.tree.fdt = fdt;
>>> +     ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
>>> +     if (ret)
>>> +             printf("Error: %d: FDT Fixup event failed\n", ret);
>>> +}
>>>    #endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
>>>
>>>    /**
>>> @@ -318,6 +335,7 @@ efi_status_t efi_install_fdt(void *fdt)
>>>        efi_carve_out_dt_rsv(fdt);
>>>
>>>        efi_try_purge_kaslr_seed(fdt);
>>> +     event_notify_dt_fixup(fdt);
>>
>> The event is already triggered in image_setup_libfdt(). Don't trigger it
>> twice.
>
> The reason I put an explicit event_notify call is because the
> image_setup_libfdt() call only calls the ft_fixup handlers if the
> livetree is not active. So the fixup handlers would not be called on
> platforms that enable livetree. Although I'm not sure if livetree
> should be disabled before the ft fixup has to happen, or platforms
> that need ft fixup should not enable OF_LIVE. Disabling the livetree
> is not happening now, so I am not sure how the fixup event should work
> on platforms which have OF_LIVE enabled.

We still must not call the same event twice. We further must ensure that
our fix-up is called last (e.g. by calling it zzz_something) and not
before adding VBE nodes.

Is there any need to use the event mechanism here?

@Simon:
None of the other function calls in image_setup_libfdt() depend on
of_live_active(). I think it would be preferable to move that check into
the listeners. If instead of of_live_active() you could use a
configuration setting for the decision, gcc could eliminate a lot of code.

We definitely need better descriptions in include/event.h and a man-page
generated from that include:

EVT_NONE
     undescribed
EVT_TEST
     undescribed
EVT_DM_POST_INIT_F
     undescribed
EVT_DM_POST_INIT_R
     undescribed
EVT_DM_PRE_PROBE
     Device is about to be probed
EVT_DM_POST_PROBE
     undescribed
EVT_DM_PRE_REMOVE
     undescribed
EVT_DM_POST_REMOVE
     undescribed
EVT_MISC_INIT_F
     undescribed
EVT_FPGA_LOAD
     undescribed
EVT_FT_FIXUP
     undescribed
EVT_MAIN_LOOP
     undescribed
EVT_COUNT
     undescribed

Best regards

Heinrich

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi,

On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
>
> Provide a way for removing certain devicetree nodes and/or properties
> from the devicetree. This is needed to purge certain nodes and
> properties which may be relevant only in U-Boot. Such nodes and
> properties are then removed from the devicetree before it is passed to
> the kernel. This ensures that the devicetree passed to the OS does not
> contain any non-compliant nodes and properties.
>
> The removal of the nodes and properties is being done through an
> EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> behind any Kconfig symbol.
>
> I have only build tested this on sandbox, and tested on qemu arm64
> virt platform. This being a RFC, I have not put this through a CI run.
>
> Sughosh Ganu (5):
>   dt: Provide a way to remove non-compliant nodes and properties
>   fwu: Add the fwu-mdata node for removal from devicetree
>   capsule: Add the capsule-key property for removal from devicetree
>   bootefi: Call the EVT_FT_FIXUP event handler
>   doc: Add a document for non-compliant DT node/property removal
>
>  cmd/bootefi.c                                 | 18 +++++
>  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
>  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
>  include/dt-structs.h                          | 11 +++
>  lib/Makefile                                  |  1 +
>  lib/dt_purge.c                                | 73 +++++++++++++++++++
>  lib/efi_loader/efi_capsule.c                  |  7 ++
>  7 files changed, 179 insertions(+)
>  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
>  create mode 100644 lib/dt_purge.c

What is the point of removing them? Instead, we should make sure that
we upstream the bindings and encourage SoC vendors to sync them. If we
remove them, no one will bother and U-Boot just becomes a dumping
ground.

Instead of this, how about working on bringing the DT validation into
U-Boot so we can see what state things are in?

Please send the bindings for Linaro's recent series (which I suspect
are motivating this series) so we can start the process.

Regards,
Simon

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Peter Robinson]

On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> >
> > Provide a way for removing certain devicetree nodes and/or properties
> > from the devicetree. This is needed to purge certain nodes and
> > properties which may be relevant only in U-Boot. Such nodes and
> > properties are then removed from the devicetree before it is passed to
> > the kernel. This ensures that the devicetree passed to the OS does not
> > contain any non-compliant nodes and properties.
> >
> > The removal of the nodes and properties is being done through an
> > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > behind any Kconfig symbol.
> >
> > I have only build tested this on sandbox, and tested on qemu arm64
> > virt platform. This being a RFC, I have not put this through a CI run.
> >
> > Sughosh Ganu (5):
> >   dt: Provide a way to remove non-compliant nodes and properties
> >   fwu: Add the fwu-mdata node for removal from devicetree
> >   capsule: Add the capsule-key property for removal from devicetree
> >   bootefi: Call the EVT_FT_FIXUP event handler
> >   doc: Add a document for non-compliant DT node/property removal
> >
> >  cmd/bootefi.c                                 | 18 +++++
> >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> >  include/dt-structs.h                          | 11 +++
> >  lib/Makefile                                  |  1 +
> >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> >  lib/efi_loader/efi_capsule.c                  |  7 ++
> >  7 files changed, 179 insertions(+)
> >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> >  create mode 100644 lib/dt_purge.c
>
> What is the point of removing them? Instead, we should make sure that
> we upstream the bindings and encourage SoC vendors to sync them. If we
> remove them, no one will bother and U-Boot just becomes a dumping
> ground.

Well things like the binman entries in DT are U-Boot specific and not
useful for HW related descriptions or for Linux or another OS being
able to deal with HW so arguably we're already a dumping ground to
some degree for not defining hardware.

> Instead of this, how about working on bringing the DT validation into
> U-Boot so we can see what state things are in?
>
> Please send the bindings for Linaro's recent series (which I suspect
> are motivating this series) so we can start the process.
>
> Regards,
> Simon

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Mon, Aug 28, 2023 at 05:37:45PM +0100, Peter Robinson wrote:
> On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > >
> > > Provide a way for removing certain devicetree nodes and/or properties
> > > from the devicetree. This is needed to purge certain nodes and
> > > properties which may be relevant only in U-Boot. Such nodes and
> > > properties are then removed from the devicetree before it is passed to
> > > the kernel. This ensures that the devicetree passed to the OS does not
> > > contain any non-compliant nodes and properties.
> > >
> > > The removal of the nodes and properties is being done through an
> > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > behind any Kconfig symbol.
> > >
> > > I have only build tested this on sandbox, and tested on qemu arm64
> > > virt platform. This being a RFC, I have not put this through a CI run.
> > >
> > > Sughosh Ganu (5):
> > >   dt: Provide a way to remove non-compliant nodes and properties
> > >   fwu: Add the fwu-mdata node for removal from devicetree
> > >   capsule: Add the capsule-key property for removal from devicetree
> > >   bootefi: Call the EVT_FT_FIXUP event handler
> > >   doc: Add a document for non-compliant DT node/property removal
> > >
> > >  cmd/bootefi.c                                 | 18 +++++
> > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > >  include/dt-structs.h                          | 11 +++
> > >  lib/Makefile                                  |  1 +
> > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > >  7 files changed, 179 insertions(+)
> > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > >  create mode 100644 lib/dt_purge.c
> >
> > What is the point of removing them? Instead, we should make sure that
> > we upstream the bindings and encourage SoC vendors to sync them. If we
> > remove them, no one will bother and U-Boot just becomes a dumping
> > ground.
> 
> Well things like the binman entries in DT are U-Boot specific and not
> useful for HW related descriptions or for Linux or another OS being
> able to deal with HW so arguably we're already a dumping ground to
> some degree for not defining hardware.

It's about validation and Simon is literally in the process of having
the binman bindings upstreamed.

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Peter,

On Mon, 28 Aug 2023 at 10:37, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > >
> > > Provide a way for removing certain devicetree nodes and/or properties
> > > from the devicetree. This is needed to purge certain nodes and
> > > properties which may be relevant only in U-Boot. Such nodes and
> > > properties are then removed from the devicetree before it is passed to
> > > the kernel. This ensures that the devicetree passed to the OS does not
> > > contain any non-compliant nodes and properties.
> > >
> > > The removal of the nodes and properties is being done through an
> > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > behind any Kconfig symbol.
> > >
> > > I have only build tested this on sandbox, and tested on qemu arm64
> > > virt platform. This being a RFC, I have not put this through a CI run.
> > >
> > > Sughosh Ganu (5):
> > >   dt: Provide a way to remove non-compliant nodes and properties
> > >   fwu: Add the fwu-mdata node for removal from devicetree
> > >   capsule: Add the capsule-key property for removal from devicetree
> > >   bootefi: Call the EVT_FT_FIXUP event handler
> > >   doc: Add a document for non-compliant DT node/property removal
> > >
> > >  cmd/bootefi.c                                 | 18 +++++
> > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > >  include/dt-structs.h                          | 11 +++
> > >  lib/Makefile                                  |  1 +
> > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > >  7 files changed, 179 insertions(+)
> > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > >  create mode 100644 lib/dt_purge.c
> >
> > What is the point of removing them? Instead, we should make sure that
> > we upstream the bindings and encourage SoC vendors to sync them. If we
> > remove them, no one will bother and U-Boot just becomes a dumping
> > ground.
>
> Well things like the binman entries in DT are U-Boot specific and not
> useful for HW related descriptions or for Linux or another OS being
> able to deal with HW so arguably we're already a dumping ground to
> some degree for not defining hardware.

I have started the process to upstream the binman bindings.

Perhaps we should use the issue tracker[1] to follow progress on all
of this. We need to clean it up.

>
> > Instead of this, how about working on bringing the DT validation into
> > U-Boot so we can see what state things are in?
> >
> > Please send the bindings for Linaro's recent series (which I suspect
> > are motivating this series) so we can start the process.

Regards,
Simon

[1] https://source.denx.de/u-boot/u-boot/-/issues

Re: [RFC PATCH 4/5] bootefi: Call the EVT_FT_FIXUP event handler

[Simon Glass]

Hi Heinrich,

On Mon, 28 Aug 2023 at 05:02, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 28.08.23 11:32, Sughosh Ganu wrote:
> > On Sat, 26 Aug 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 8/26/23 11:06, Sughosh Ganu wrote:
> >>> The bootefi command passes the devicetree to the kernel through the
> >>> EFI config table. Call the event handlers for fixing the devicetree
> >>> before jumping into the kernel. This removes any devicetree nodes
> >>> and/or properties that are specific only to U-Boot, and are not to be
> >>> passed to the OS.
> >>>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >>> ---
> >>>    cmd/bootefi.c | 18 ++++++++++++++++++
> >>>    1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index f73d6eb0e2..c359a46ec4 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -237,6 +237,23 @@ static void *get_config_table(const efi_guid_t *guid)
> >>>        return NULL;
> >>>    }
> >>>
> >>> +/**
> >>> + * event_notify_dt_fixup() - call ft_fixup event
> >>> + *
> >>> + * @fdt:     address of the device tree to be passed to the kernel
> >>> + *           through the configuration table
> >>> + * Return:   None
> >>> + */
> >>> +static void event_notify_dt_fixup(void *fdt)
> >>> +{
> >>> +     int ret;
> >>> +     struct event_ft_fixup fixup = {0};
> >>> +
> >>> +     fixup.tree.fdt = fdt;
> >>> +     ret = event_notify(EVT_FT_FIXUP, &fixup, sizeof(fixup));
> >>> +     if (ret)
> >>> +             printf("Error: %d: FDT Fixup event failed\n", ret);
> >>> +}
> >>>    #endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
> >>>
> >>>    /**
> >>> @@ -318,6 +335,7 @@ efi_status_t efi_install_fdt(void *fdt)
> >>>        efi_carve_out_dt_rsv(fdt);
> >>>
> >>>        efi_try_purge_kaslr_seed(fdt);
> >>> +     event_notify_dt_fixup(fdt);
> >>
> >> The event is already triggered in image_setup_libfdt(). Don't trigger it
> >> twice.
> >
> > The reason I put an explicit event_notify call is because the
> > image_setup_libfdt() call only calls the ft_fixup handlers if the
> > livetree is not active. So the fixup handlers would not be called on
> > platforms that enable livetree. Although I'm not sure if livetree
> > should be disabled before the ft fixup has to happen, or platforms
> > that need ft fixup should not enable OF_LIVE. Disabling the livetree
> > is not happening now, so I am not sure how the fixup event should work
> > on platforms which have OF_LIVE enabled.
>
> We still must not call the same event twice. We further must ensure that
> our fix-up is called last (e.g. by calling it zzz_something) and not
> before adding VBE nodes.
>
> Is there any need to use the event mechanism here?
>
> @Simon:
> None of the other function calls in image_setup_libfdt() depend on
> of_live_active(). I think it would be preferable to move that check into
> the listeners. If instead of of_live_active() you could use a
> configuration setting for the decision, gcc could eliminate a lot of code.

Firstly, of_live_active() should check CONFIG_IS_ENABLED(OF_LIVE)
first. I noticed this over the weekend when looking at something else.

The original code was due to a limitation with U-Boot's ofnode
implementation, in that it did not support multiple devicetrees
without OF_LIVE. We need to support at least two, since we have the
'control' DTB and the one being set up to boot the OS.

Anyway, that limitation can be removed, so long as
CONFIG_MULTI_DTB_FIT is set. In other words, we should enable
MULTI_DTB_FIT if !OF_LIVE and all will be well.

>
> We definitely need better descriptions in include/event.h and a man-page
> generated from that include:
>
> EVT_NONE
>      undescribed
> EVT_TEST
>      undescribed
> EVT_DM_POST_INIT_F
>      undescribed
> EVT_DM_POST_INIT_R
>      undescribed
> EVT_DM_PRE_PROBE
>      Device is about to be probed
> EVT_DM_POST_PROBE
>      undescribed
> EVT_DM_PRE_REMOVE
>      undescribed
> EVT_DM_POST_REMOVE
>      undescribed
> EVT_MISC_INIT_F
>      undescribed
> EVT_FPGA_LOAD
>      undescribed
> EVT_FT_FIXUP
>      undescribed
> EVT_MAIN_LOOP
>      undescribed
> EVT_COUNT
>      undescribed

Yes indeed. I added some for the latest patch in this area.

https://patchwork.ozlabs.org/project/uboot/patch/20230822031705.1340941-15-sjg@chromium.org/

Regards,
Simon

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

[Simon Glass]

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Mon, Aug 28, 2023 at 10:19:55AM -0600, Simon Glass wrote:
> Hi,
> 
> On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> >
> > Provide a way for removing certain devicetree nodes and/or properties
> > from the devicetree. This is needed to purge certain nodes and
> > properties which may be relevant only in U-Boot. Such nodes and
> > properties are then removed from the devicetree before it is passed to
> > the kernel. This ensures that the devicetree passed to the OS does not
> > contain any non-compliant nodes and properties.
> >
> > The removal of the nodes and properties is being done through an
> > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > behind any Kconfig symbol.
> >
> > I have only build tested this on sandbox, and tested on qemu arm64
> > virt platform. This being a RFC, I have not put this through a CI run.
> >
> > Sughosh Ganu (5):
> >   dt: Provide a way to remove non-compliant nodes and properties
> >   fwu: Add the fwu-mdata node for removal from devicetree
> >   capsule: Add the capsule-key property for removal from devicetree
> >   bootefi: Call the EVT_FT_FIXUP event handler
> >   doc: Add a document for non-compliant DT node/property removal
> >
> >  cmd/bootefi.c                                 | 18 +++++
> >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> >  include/dt-structs.h                          | 11 +++
> >  lib/Makefile                                  |  1 +
> >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> >  lib/efi_loader/efi_capsule.c                  |  7 ++
> >  7 files changed, 179 insertions(+)
> >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> >  create mode 100644 lib/dt_purge.c
> 
> What is the point of removing them? Instead, we should make sure that
> we upstream the bindings and encourage SoC vendors to sync them. If we
> remove them, no one will bother and U-Boot just becomes a dumping
> ground.

It's about having a defined process to remove them, rather than an
ad-hoc process like one can do today to remove them. And it's about
having control over the situation rather than dismissing it, as vendor
can already say they used $this version of the software for validation,
so patches-on-top aren't out of the question.

-- 
Tom

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

Re: [RFC PATCH 1/5] dt: Provide a way to remove non-compliant nodes and properties

[Tom Rini]

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

On Sat, Aug 26, 2023 at 02:36:29PM +0530, Sughosh Ganu wrote:

> Add a function which is registered to spy for a EVT_FT_FIXUP event,
> and removes the non upstreamed nodes and properties from the
> devicetree before it gets passed to the OS.
> 
> This allows removing entire nodes, or specific properties under nodes
> from the devicetree. The required nodes and properties can be
> registered for removal through the DT_NON_COMPLIANT_PURGE and
> DT_NON_COMPLIANT_PURGE_LIST macros.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

So, the conceptual problem here is that while we do need to have a way
to purge nodes and properties that have been rejected by upstream, we
also need to ensure an upstream attempt was made. To that end, how I
envision things is:
- When we build the docs, something under doc/develop/ has a page /
  section for each binding we're purging, which links to and summarizes
  why it's not appropriate for upstream.
- It's non-default to purge said nodes.
- We at least make checkpatch / et al complain about new purges being
  added, and CI failing on the number of purges increasing without also
  increasing the number of allowed purges.

-- 
Tom

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

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

[Sughosh Ganu]

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

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

[Tom Rini]

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

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.

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Peter Robinson]

On Mon, Aug 28, 2023 at 6:54 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Peter,
>
> On Mon, 28 Aug 2023 at 10:37, Peter Robinson <pbrobinson@gmail.com> wrote:
> >
> > On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > >
> > > > Provide a way for removing certain devicetree nodes and/or properties
> > > > from the devicetree. This is needed to purge certain nodes and
> > > > properties which may be relevant only in U-Boot. Such nodes and
> > > > properties are then removed from the devicetree before it is passed to
> > > > the kernel. This ensures that the devicetree passed to the OS does not
> > > > contain any non-compliant nodes and properties.
> > > >
> > > > The removal of the nodes and properties is being done through an
> > > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > > behind any Kconfig symbol.
> > > >
> > > > I have only build tested this on sandbox, and tested on qemu arm64
> > > > virt platform. This being a RFC, I have not put this through a CI run.
> > > >
> > > > Sughosh Ganu (5):
> > > >   dt: Provide a way to remove non-compliant nodes and properties
> > > >   fwu: Add the fwu-mdata node for removal from devicetree
> > > >   capsule: Add the capsule-key property for removal from devicetree
> > > >   bootefi: Call the EVT_FT_FIXUP event handler
> > > >   doc: Add a document for non-compliant DT node/property removal
> > > >
> > > >  cmd/bootefi.c                                 | 18 +++++
> > > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > > >  include/dt-structs.h                          | 11 +++
> > > >  lib/Makefile                                  |  1 +
> > > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > > >  7 files changed, 179 insertions(+)
> > > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > > >  create mode 100644 lib/dt_purge.c
> > >
> > > What is the point of removing them? Instead, we should make sure that
> > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > remove them, no one will bother and U-Boot just becomes a dumping
> > > ground.
> >
> > Well things like the binman entries in DT are U-Boot specific and not
> > useful for HW related descriptions or for Linux or another OS being
> > able to deal with HW so arguably we're already a dumping ground to
> > some degree for not defining hardware.
>
> I have started the process to upstream the binman bindings.

I don't think they should be in DT at all, they don't describe
anything to do with hardware, or generally even the runtime of a
device, they don't even describe the boot/runtime state of the
firmware, they describe build time, so I don't see what that has to do
with device tree? Can you explain that? To me those sorts of things
should live in a build time style config file.

> Perhaps we should use the issue tracker[1] to follow progress on all
> of this. We need to clean it up.
>
> >
> > > Instead of this, how about working on bringing the DT validation into
> > > U-Boot so we can see what state things are in?
> > >
> > > Please send the bindings for Linaro's recent series (which I suspect
> > > are motivating this series) so we can start the process.
>
> Regards,
> Simon
>
> [1] https://source.denx.de/u-boot/u-boot/-/issues

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Peter,

On Mon, 28 Aug 2023 at 14:29, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> On Mon, Aug 28, 2023 at 6:54 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Peter,
> >
> > On Mon, 28 Aug 2023 at 10:37, Peter Robinson <pbrobinson@gmail.com> wrote:
> > >
> > > On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > >
> > > > > Provide a way for removing certain devicetree nodes and/or properties
> > > > > from the devicetree. This is needed to purge certain nodes and
> > > > > properties which may be relevant only in U-Boot. Such nodes and
> > > > > properties are then removed from the devicetree before it is passed to
> > > > > the kernel. This ensures that the devicetree passed to the OS does not
> > > > > contain any non-compliant nodes and properties.
> > > > >
> > > > > The removal of the nodes and properties is being done through an
> > > > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > > > behind any Kconfig symbol.
> > > > >
> > > > > I have only build tested this on sandbox, and tested on qemu arm64
> > > > > virt platform. This being a RFC, I have not put this through a CI run.
> > > > >
> > > > > Sughosh Ganu (5):
> > > > >   dt: Provide a way to remove non-compliant nodes and properties
> > > > >   fwu: Add the fwu-mdata node for removal from devicetree
> > > > >   capsule: Add the capsule-key property for removal from devicetree
> > > > >   bootefi: Call the EVT_FT_FIXUP event handler
> > > > >   doc: Add a document for non-compliant DT node/property removal
> > > > >
> > > > >  cmd/bootefi.c                                 | 18 +++++
> > > > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > > > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > > > >  include/dt-structs.h                          | 11 +++
> > > > >  lib/Makefile                                  |  1 +
> > > > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > > > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > > > >  7 files changed, 179 insertions(+)
> > > > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > >  create mode 100644 lib/dt_purge.c
> > > >
> > > > What is the point of removing them? Instead, we should make sure that
> > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > ground.
> > >
> > > Well things like the binman entries in DT are U-Boot specific and not
> > > useful for HW related descriptions or for Linux or another OS being
> > > able to deal with HW so arguably we're already a dumping ground to
> > > some degree for not defining hardware.
> >
> > I have started the process to upstream the binman bindings.
>
> I don't think they should be in DT at all, they don't describe
> anything to do with hardware, or generally even the runtime of a
> device, they don't even describe the boot/runtime state of the
> firmware, they describe build time, so I don't see what that has to do
> with device tree? Can you explain that? To me those sorts of things
> should live in a build time style config file.

I beg to differ. Devicetree is more than just hardware and always has
been. See, for example the /chosen and /options nodes.

We need run-time configuration here, since we cannot know at build
time what we will be asked to do by a previous firmware phase.

>
> > Perhaps we should use the issue tracker[1] to follow progress on all
> > of this. We need to clean it up.
> >
> > >
> > > > Instead of this, how about working on bringing the DT validation into
> > > > U-Boot so we can see what state things are in?
> > > >
> > > > Please send the bindings for Linaro's recent series (which I suspect
> > > > are motivating this series) so we can start the process.
> >
> > Regards,
> > Simon
> >
> > [1] https://source.denx.de/u-boot/u-boot/-/issues

Regards,
Simon

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Peter Robinson]

> > > > > > Provide a way for removing certain devicetree nodes and/or properties
> > > > > > from the devicetree. This is needed to purge certain nodes and
> > > > > > properties which may be relevant only in U-Boot. Such nodes and
> > > > > > properties are then removed from the devicetree before it is passed to
> > > > > > the kernel. This ensures that the devicetree passed to the OS does not
> > > > > > contain any non-compliant nodes and properties.
> > > > > >
> > > > > > The removal of the nodes and properties is being done through an
> > > > > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > > > > behind any Kconfig symbol.
> > > > > >
> > > > > > I have only build tested this on sandbox, and tested on qemu arm64
> > > > > > virt platform. This being a RFC, I have not put this through a CI run.
> > > > > >
> > > > > > Sughosh Ganu (5):
> > > > > >   dt: Provide a way to remove non-compliant nodes and properties
> > > > > >   fwu: Add the fwu-mdata node for removal from devicetree
> > > > > >   capsule: Add the capsule-key property for removal from devicetree
> > > > > >   bootefi: Call the EVT_FT_FIXUP event handler
> > > > > >   doc: Add a document for non-compliant DT node/property removal
> > > > > >
> > > > > >  cmd/bootefi.c                                 | 18 +++++
> > > > > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > > > > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > > > > >  include/dt-structs.h                          | 11 +++
> > > > > >  lib/Makefile                                  |  1 +
> > > > > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > > > > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > > > > >  7 files changed, 179 insertions(+)
> > > > > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > > >  create mode 100644 lib/dt_purge.c
> > > > >
> > > > > What is the point of removing them? Instead, we should make sure that
> > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > ground.
> > > >
> > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > useful for HW related descriptions or for Linux or another OS being
> > > > able to deal with HW so arguably we're already a dumping ground to
> > > > some degree for not defining hardware.
> > >
> > > I have started the process to upstream the binman bindings.
> >
> > I don't think they should be in DT at all, they don't describe
> > anything to do with hardware, or generally even the runtime of a
> > device, they don't even describe the boot/runtime state of the
> > firmware, they describe build time, so I don't see what that has to do
> > with device tree? Can you explain that? To me those sorts of things
> > should live in a build time style config file.
>
> I beg to differ. Devicetree is more than just hardware and always has
> been. See, for example the /chosen and /options nodes.

Can you give me an example of "options" as grep doesn't give me a
single one in the kernel tree? I think we can just agree to disagree
here.

> We need run-time configuration here, since we cannot know at build
> time what we will be asked to do by a previous firmware phase.

Can you provide an example as to how that is used during runtime? Do
you mean runtime during the build process or runtime on the device?

> >
> > > Perhaps we should use the issue tracker[1] to follow progress on all
> > > of this. We need to clean it up.
> > >
> > > >
> > > > > Instead of this, how about working on bringing the DT validation into
> > > > > U-Boot so we can see what state things are in?
> > > > >
> > > > > Please send the bindings for Linaro's recent series (which I suspect
> > > > > are motivating this series) so we can start the process.
> > >
> > > Regards,
> > > Simon
> > >
> > > [1] https://source.denx.de/u-boot/u-boot/-/issues
>
> Regards,
> Simon

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

[Simon Glass]

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Peter,

On Tue, 29 Aug 2023 at 04:33, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> > > > > > > Provide a way for removing certain devicetree nodes and/or properties
> > > > > > > from the devicetree. This is needed to purge certain nodes and
> > > > > > > properties which may be relevant only in U-Boot. Such nodes and
> > > > > > > properties are then removed from the devicetree before it is passed to
> > > > > > > the kernel. This ensures that the devicetree passed to the OS does not
> > > > > > > contain any non-compliant nodes and properties.
> > > > > > >
> > > > > > > The removal of the nodes and properties is being done through an
> > > > > > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > > > > > behind any Kconfig symbol.
> > > > > > >
> > > > > > > I have only build tested this on sandbox, and tested on qemu arm64
> > > > > > > virt platform. This being a RFC, I have not put this through a CI run.
> > > > > > >
> > > > > > > Sughosh Ganu (5):
> > > > > > >   dt: Provide a way to remove non-compliant nodes and properties
> > > > > > >   fwu: Add the fwu-mdata node for removal from devicetree
> > > > > > >   capsule: Add the capsule-key property for removal from devicetree
> > > > > > >   bootefi: Call the EVT_FT_FIXUP event handler
> > > > > > >   doc: Add a document for non-compliant DT node/property removal
> > > > > > >
> > > > > > >  cmd/bootefi.c                                 | 18 +++++
> > > > > > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > > > > > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > > > > > >  include/dt-structs.h                          | 11 +++
> > > > > > >  lib/Makefile                                  |  1 +
> > > > > > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > > > > > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > > > > > >  7 files changed, 179 insertions(+)
> > > > > > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > > > >  create mode 100644 lib/dt_purge.c
> > > > > >
> > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > ground.
> > > > >
> > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > some degree for not defining hardware.
> > > >
> > > > I have started the process to upstream the binman bindings.
> > >
> > > I don't think they should be in DT at all, they don't describe
> > > anything to do with hardware, or generally even the runtime of a
> > > device, they don't even describe the boot/runtime state of the
> > > firmware, they describe build time, so I don't see what that has to do
> > > with device tree? Can you explain that? To me those sorts of things
> > > should live in a build time style config file.
> >
> > I beg to differ. Devicetree is more than just hardware and always has
> > been. See, for example the /chosen and /options nodes.
>
> Can you give me an example of "options" as grep doesn't give me a
> single one in the kernel tree? I think we can just agree to disagree
> here.

See here: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml

I don't mind disagreeing, so long as it doesn't result in any
restrictions on using devicetree in firmware. But it is very important
to the success of firmware that we can make full use of the
devicetree.

> > We need run-time configuration here, since we cannot know at build
> > time what we will be asked to do by a previous firmware phase.
>
> Can you provide an example as to how that is used during runtime? Do
> you mean runtime during the build process or runtime on the device?

I mean runtime on the device. An example is that we might want to
control whether the serial UART is enabled, without having to rebuild
each program in the firmware stack.

>
> > >
> > > > Perhaps we should use the issue tracker[1] to follow progress on all
> > > > of this. We need to clean it up.
> > > >
> > > > >
> > > > > > Instead of this, how about working on bringing the DT validation into
> > > > > > U-Boot so we can see what state things are in?
> > > > > >
> > > > > > Please send the bindings for Linaro's recent series (which I suspect
> > > > > > are motivating this series) so we can start the process.
> > > >
> > > > Regards,
> > > > Simon
> > > >
> > > > [1] https://source.denx.de/u-boot/u-boot/-/issues
> >
> > Regards,
> > Simon

Regards,
Simon

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

[Sughosh Ganu]

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

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

[Ilias Apalodimas]

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Peter Robinson]

> > > > > I have started the process to upstream the binman bindings.
> > > >
> > > > I don't think they should be in DT at all, they don't describe
> > > > anything to do with hardware, or generally even the runtime of a
> > > > device, they don't even describe the boot/runtime state of the
> > > > firmware, they describe build time, so I don't see what that has to do
> > > > with device tree? Can you explain that? To me those sorts of things
> > > > should live in a build time style config file.
> > >
> > > I beg to differ. Devicetree is more than just hardware and always has
> > > been. See, for example the /chosen and /options nodes.
> >
> > Can you give me an example of "options" as grep doesn't give me a
> > single one in the kernel tree? I think we can just agree to disagree
> > here.
>
> See here: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml

All of those options look to me like they would work just fine in a
.env file like you've added board/raspberrypi/rpi/rpi.env

> I don't mind disagreeing, so long as it doesn't result in any
> restrictions on using devicetree in firmware. But it is very important
> to the success of firmware that we can make full use of the
> devicetree.

That statement doesn't make sense, of course the firmware makes full
use of the DT because it needs it to instantiate the HW. You don't
actually answer my question though.

> > > We need run-time configuration here, since we cannot know at build
> > > time what we will be asked to do by a previous firmware phase.
> >
> > Can you provide an example as to how that is used during runtime? Do
> > you mean runtime during the build process or runtime on the device?
>
> I mean runtime on the device. An example is that we might want to
> control whether the serial UART is enabled, without having to rebuild
> each program in the firmware stack.

That is describing the HW though, such as whether a serial port is
enabled or not, it's not how the binman pieces you're adding to DT are
used during runtime which is the question I was asking.

> >
> > > >
> > > > > Perhaps we should use the issue tracker[1] to follow progress on all
> > > > > of this. We need to clean it up.
> > > > >
> > > > > >
> > > > > > > Instead of this, how about working on bringing the DT validation into
> > > > > > > U-Boot so we can see what state things are in?
> > > > > > >
> > > > > > > Please send the bindings for Linaro's recent series (which I suspect
> > > > > > > are motivating this series) so we can start the process.
> > > > >
> > > > > Regards,
> > > > > Simon
> > > > >
> > > > > [1] https://source.denx.de/u-boot/u-boot/-/issues
> > >
> > > Regards,
> > > Simon
>
> Regards,
> Simon

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

[Tom Rini]

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

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

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

-- 
Tom

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

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

[Simon Glass]

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Peter,

On Wed, 30 Aug 2023 at 02:19, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> > > > > > I have started the process to upstream the binman bindings.
> > > > >
> > > > > I don't think they should be in DT at all, they don't describe
> > > > > anything to do with hardware, or generally even the runtime of a
> > > > > device, they don't even describe the boot/runtime state of the
> > > > > firmware, they describe build time, so I don't see what that has to do
> > > > > with device tree? Can you explain that? To me those sorts of things
> > > > > should live in a build time style config file.
> > > >
> > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > been. See, for example the /chosen and /options nodes.
> > >
> > > Can you give me an example of "options" as grep doesn't give me a
> > > single one in the kernel tree? I think we can just agree to disagree
> > > here.
> >
> > See here: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml
>
> All of those options look to me like they would work just fine in a
> .env file like you've added board/raspberrypi/rpi/rpi.env

But that is built into U-Boot. How can it be controlled by another
previous progress in the firmware stack?

>
> > I don't mind disagreeing, so long as it doesn't result in any
> > restrictions on using devicetree in firmware. But it is very important
> > to the success of firmware that we can make full use of the
> > devicetree.
>
> That statement doesn't make sense, of course the firmware makes full
> use of the DT because it needs it to instantiate the HW. You don't
> actually answer my question though.

I think I tried to answer your question but we are not on the same
page. Please don't say that DT is just for hardware, since that is a
sore point with me. It helf back U-Boot for many years, to no useful
purpose.

Perhaps you could restate the question?

Also, why are you trying to keep things out of the DT?

>
> > > > We need run-time configuration here, since we cannot know at build
> > > > time what we will be asked to do by a previous firmware phase.
> > >
> > > Can you provide an example as to how that is used during runtime? Do
> > > you mean runtime during the build process or runtime on the device?
> >
> > I mean runtime on the device. An example is that we might want to
> > control whether the serial UART is enabled, without having to rebuild
> > each program in the firmware stack.
>
> That is describing the HW though, such as whether a serial port is
> enabled or not, it's not how the binman pieces you're adding to DT are
> used during runtime which is the question I was asking.

There is:

1. whether the serial device is enabled in the DT
2. whether it emits character or not

Often we always want 1, in case we need to emit something. But for 2
we want to control it with a global setting that applies to all
programs in the firmware flow.

Another example would be a framebuffer address, where we want to
provide it in the DT so that all programs can use the same one.


>
> > >
> > > > >
> > > > > > Perhaps we should use the issue tracker[1] to follow progress on all
> > > > > > of this. We need to clean it up.
> > > > > >
> > > > > > >
> > > > > > > > Instead of this, how about working on bringing the DT validation into
> > > > > > > > U-Boot so we can see what state things are in?
> > > > > > > >
> > > > > > > > Please send the bindings for Linaro's recent series (which I suspect
> > > > > > > > are motivating this series) so we can start the process.
> > > > > >
> > > > > > Regards,
> > > > > > Simon
> > > > > >
> > > > > > [1] https://source.denx.de/u-boot/u-boot/-/issues

Regards,
Simon

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

[Ilias Apalodimas]

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

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

[Tom Rini]

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

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.

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Rob Herring]

On Mon, Aug 28, 2023 at 04:09:29PM -0600, Simon Glass wrote:
> Hi Peter,
> 
> On Mon, 28 Aug 2023 at 14:29, Peter Robinson <pbrobinson@gmail.com> wrote:
> >
> > On Mon, Aug 28, 2023 at 6:54 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Peter,
> > >
> > > On Mon, 28 Aug 2023 at 10:37, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > >
> > > > On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > >
> > > > > > Provide a way for removing certain devicetree nodes and/or properties
> > > > > > from the devicetree. This is needed to purge certain nodes and
> > > > > > properties which may be relevant only in U-Boot. Such nodes and
> > > > > > properties are then removed from the devicetree before it is passed to
> > > > > > the kernel. This ensures that the devicetree passed to the OS does not
> > > > > > contain any non-compliant nodes and properties.
> > > > > >
> > > > > > The removal of the nodes and properties is being done through an
> > > > > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > > > > behind any Kconfig symbol.
> > > > > >
> > > > > > I have only build tested this on sandbox, and tested on qemu arm64
> > > > > > virt platform. This being a RFC, I have not put this through a CI run.
> > > > > >
> > > > > > Sughosh Ganu (5):
> > > > > >   dt: Provide a way to remove non-compliant nodes and properties
> > > > > >   fwu: Add the fwu-mdata node for removal from devicetree
> > > > > >   capsule: Add the capsule-key property for removal from devicetree
> > > > > >   bootefi: Call the EVT_FT_FIXUP event handler
> > > > > >   doc: Add a document for non-compliant DT node/property removal
> > > > > >
> > > > > >  cmd/bootefi.c                                 | 18 +++++
> > > > > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > > > > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > > > > >  include/dt-structs.h                          | 11 +++
> > > > > >  lib/Makefile                                  |  1 +
> > > > > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > > > > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > > > > >  7 files changed, 179 insertions(+)
> > > > > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > > >  create mode 100644 lib/dt_purge.c
> > > > >
> > > > > What is the point of removing them? Instead, we should make sure that
> > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > ground.
> > > >
> > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > useful for HW related descriptions or for Linux or another OS being
> > > > able to deal with HW so arguably we're already a dumping ground to
> > > > some degree for not defining hardware.
> > >
> > > I have started the process to upstream the binman bindings.
> >
> > I don't think they should be in DT at all, they don't describe
> > anything to do with hardware, or generally even the runtime of a
> > device, they don't even describe the boot/runtime state of the
> > firmware, they describe build time, so I don't see what that has to do
> > with device tree? Can you explain that? To me those sorts of things
> > should live in a build time style config file.

For the record, I tend to agree.

> I beg to differ. Devicetree is more than just hardware and always has
> been. See, for example the /chosen and /options nodes.

There are exceptions...

> We need run-time configuration here, since we cannot know at build
> time what we will be asked to do by a previous firmware phase.

Really, I don't want to have to care about the binman binding. If it is 
u-boot specific, then it should stay in u-boot. I took /options/u-boot/, 
but now I'm starting to have second thoughts on that being in dtschema 
if it is going to be continually and frequently extended. Validating it 
in SR does little. If a vendor is abusing /options/u-boot/ in some way 
they could just as easily remove the node in their u-boot fork to pass. 
SR is certainly not going to require the node be there.

On A/B updates, that really doesn't seem like a u-boot specific problem 
to me. No one wants A/B updates in EDK2 or anything else?

Rob

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Rob,

On Wed, 6 Sept 2023 at 08:21, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Aug 28, 2023 at 04:09:29PM -0600, Simon Glass wrote:
> > Hi Peter,
> >
> > On Mon, 28 Aug 2023 at 14:29, Peter Robinson <pbrobinson@gmail.com> wrote:
> > >
> > > On Mon, Aug 28, 2023 at 6:54 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Peter,
> > > >
> > > > On Mon, 28 Aug 2023 at 10:37, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > > >
> > > > > On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > >
> > > > > > > Provide a way for removing certain devicetree nodes and/or properties
> > > > > > > from the devicetree. This is needed to purge certain nodes and
> > > > > > > properties which may be relevant only in U-Boot. Such nodes and
> > > > > > > properties are then removed from the devicetree before it is passed to
> > > > > > > the kernel. This ensures that the devicetree passed to the OS does not
> > > > > > > contain any non-compliant nodes and properties.
> > > > > > >
> > > > > > > The removal of the nodes and properties is being done through an
> > > > > > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > > > > > behind any Kconfig symbol.
> > > > > > >
> > > > > > > I have only build tested this on sandbox, and tested on qemu arm64
> > > > > > > virt platform. This being a RFC, I have not put this through a CI run.
> > > > > > >
> > > > > > > Sughosh Ganu (5):
> > > > > > >   dt: Provide a way to remove non-compliant nodes and properties
> > > > > > >   fwu: Add the fwu-mdata node for removal from devicetree
> > > > > > >   capsule: Add the capsule-key property for removal from devicetree
> > > > > > >   bootefi: Call the EVT_FT_FIXUP event handler
> > > > > > >   doc: Add a document for non-compliant DT node/property removal
> > > > > > >
> > > > > > >  cmd/bootefi.c                                 | 18 +++++
> > > > > > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > > > > > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > > > > > >  include/dt-structs.h                          | 11 +++
> > > > > > >  lib/Makefile                                  |  1 +
> > > > > > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > > > > > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > > > > > >  7 files changed, 179 insertions(+)
> > > > > > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > > > >  create mode 100644 lib/dt_purge.c
> > > > > >
> > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > ground.
> > > > >
> > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > some degree for not defining hardware.
> > > >
> > > > I have started the process to upstream the binman bindings.
> > >
> > > I don't think they should be in DT at all, they don't describe
> > > anything to do with hardware, or generally even the runtime of a
> > > device, they don't even describe the boot/runtime state of the
> > > firmware, they describe build time, so I don't see what that has to do
> > > with device tree? Can you explain that? To me those sorts of things
> > > should live in a build time style config file.
>
> For the record, I tend to agree.
>
> > I beg to differ. Devicetree is more than just hardware and always has
> > been. See, for example the /chosen and /options nodes.
>
> There are exceptions...

I wish we could stop having this discussion entirely...to me the
devicetree is all that firmware has to handle its configuration, both
within projects and across project boundaries. There is no user space,
no filesystem, etc.

>
> > We need run-time configuration here, since we cannot know at build
> > time what we will be asked to do by a previous firmware phase.
>
> Really, I don't want to have to care about the binman binding. If it is
> u-boot specific, then it should stay in u-boot.

I don't believe it is U-Boot-specific since it describes the system
firmware as a whole. It collects things from multiple projects and we
want to know where they ended up, so we can do firmware update, etc.

> I took /options/u-boot/,
> but now I'm starting to have second thoughts on that being in dtschema
> if it is going to be continually and frequently extended. Validating it
> in SR does little. If a vendor is abusing /options/u-boot/ in some way
> they could just as easily remove the node in their u-boot fork to pass.
> SR is certainly not going to require the node be there.

The challenge here is that we are trying to validate that the U-Boot
DT matches the schema. We don't want to have people adding things
willy nilly, even in /options/u-boot.

>
> On A/B updates, that really doesn't seem like a u-boot specific problem
> to me. No one wants A/B updates in EDK2 or anything else?

I agree we should specify it correctly and in a general manner.

Regards,
Simon

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Wed, Sep 06, 2023 at 09:21:39AM -0500, Rob Herring wrote:

[snip]
> On A/B updates, that really doesn't seem like a u-boot specific problem 
> to me. No one wants A/B updates in EDK2 or anything else?

This specific case goes back to:
https://lore.kernel.org/u-boot/20230410232112.72778-1-jaswinder.singh@linaro.org/

But I believe Sughosh and Ilias are basically the people who are in
charge of the code at this point?  So perhaps a step here is some advice
on whatever they need to redo/rethink to get something that is
acceptable.

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Wed, Sep 06, 2023 at 09:21:39AM -0500, Rob Herring wrote:
> On Mon, Aug 28, 2023 at 04:09:29PM -0600, Simon Glass wrote:
> > Hi Peter,
> > 
> > On Mon, 28 Aug 2023 at 14:29, Peter Robinson <pbrobinson@gmail.com> wrote:
> > >
> > > On Mon, Aug 28, 2023 at 6:54 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Peter,
> > > >
> > > > On Mon, 28 Aug 2023 at 10:37, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > > >
> > > > > On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > >
> > > > > > > Provide a way for removing certain devicetree nodes and/or properties
> > > > > > > from the devicetree. This is needed to purge certain nodes and
> > > > > > > properties which may be relevant only in U-Boot. Such nodes and
> > > > > > > properties are then removed from the devicetree before it is passed to
> > > > > > > the kernel. This ensures that the devicetree passed to the OS does not
> > > > > > > contain any non-compliant nodes and properties.
> > > > > > >
> > > > > > > The removal of the nodes and properties is being done through an
> > > > > > > EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
> > > > > > > behind any Kconfig symbol.
> > > > > > >
> > > > > > > I have only build tested this on sandbox, and tested on qemu arm64
> > > > > > > virt platform. This being a RFC, I have not put this through a CI run.
> > > > > > >
> > > > > > > Sughosh Ganu (5):
> > > > > > >   dt: Provide a way to remove non-compliant nodes and properties
> > > > > > >   fwu: Add the fwu-mdata node for removal from devicetree
> > > > > > >   capsule: Add the capsule-key property for removal from devicetree
> > > > > > >   bootefi: Call the EVT_FT_FIXUP event handler
> > > > > > >   doc: Add a document for non-compliant DT node/property removal
> > > > > > >
> > > > > > >  cmd/bootefi.c                                 | 18 +++++
> > > > > > >  .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
> > > > > > >  drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
> > > > > > >  include/dt-structs.h                          | 11 +++
> > > > > > >  lib/Makefile                                  |  1 +
> > > > > > >  lib/dt_purge.c                                | 73 +++++++++++++++++++
> > > > > > >  lib/efi_loader/efi_capsule.c                  |  7 ++
> > > > > > >  7 files changed, 179 insertions(+)
> > > > > > >  create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
> > > > > > >  create mode 100644 lib/dt_purge.c
> > > > > >
> > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > ground.
> > > > >
> > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > some degree for not defining hardware.
> > > >
> > > > I have started the process to upstream the binman bindings.
> > >
> > > I don't think they should be in DT at all, they don't describe
> > > anything to do with hardware, or generally even the runtime of a
> > > device, they don't even describe the boot/runtime state of the
> > > firmware, they describe build time, so I don't see what that has to do
> > > with device tree? Can you explain that? To me those sorts of things
> > > should live in a build time style config file.
> 
> For the record, I tend to agree.
> 
> > I beg to differ. Devicetree is more than just hardware and always has
> > been. See, for example the /chosen and /options nodes.
> 
> There are exceptions...
> 
> > We need run-time configuration here, since we cannot know at build
> > time what we will be asked to do by a previous firmware phase.
> 
> Really, I don't want to have to care about the binman binding. If it is 
> u-boot specific, then it should stay in u-boot. I took /options/u-boot/, 
> but now I'm starting to have second thoughts on that being in dtschema 
> if it is going to be continually and frequently extended. Validating it 
> in SR does little. If a vendor is abusing /options/u-boot/ in some way 
> they could just as easily remove the node in their u-boot fork to pass. 
> SR is certainly not going to require the node be there.

It's going to continue to be a "fun" tight-rope to walk. No one wants an
easy way for vendors to cheat the requirements which is why in some
other parts of this thread (you may or may not have been on, I don't
recall, sorry) I've been trying to make it clear that the removal
mechanism should be both a slight pain to add to, and at least wrt
upstream, clear that effort was made to upstream things.  Cheaters are
going to cheat and they could just chain a bunch of "fdt rm" together to
do it today.

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Heinrich Schuchardt]

On 9/7/23 00:04, Tom Rini wrote:
> On Wed, Sep 06, 2023 at 09:21:39AM -0500, Rob Herring wrote:
>> On Mon, Aug 28, 2023 at 04:09:29PM -0600, Simon Glass wrote:
>>> Hi Peter,
>>>
>>> On Mon, 28 Aug 2023 at 14:29, Peter Robinson <pbrobinson@gmail.com> wrote:
>>>>
>>>> On Mon, Aug 28, 2023 at 6:54 PM Simon Glass <sjg@chromium.org> wrote:
>>>>>
>>>>> Hi Peter,
>>>>>
>>>>> On Mon, 28 Aug 2023 at 10:37, Peter Robinson <pbrobinson@gmail.com> wrote:
>>>>>>
>>>>>> On Mon, Aug 28, 2023 at 5:20 PM Simon Glass <sjg@chromium.org> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Sat, 26 Aug 2023 at 03:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Provide a way for removing certain devicetree nodes and/or properties
>>>>>>>> from the devicetree. This is needed to purge certain nodes and
>>>>>>>> properties which may be relevant only in U-Boot. Such nodes and
>>>>>>>> properties are then removed from the devicetree before it is passed to
>>>>>>>> the kernel. This ensures that the devicetree passed to the OS does not
>>>>>>>> contain any non-compliant nodes and properties.
>>>>>>>>
>>>>>>>> The removal of the nodes and properties is being done through an
>>>>>>>> EVT_FT_FIXUP handler. I am not sure if the removal code needs to be
>>>>>>>> behind any Kconfig symbol.
>>>>>>>>
>>>>>>>> I have only build tested this on sandbox, and tested on qemu arm64
>>>>>>>> virt platform. This being a RFC, I have not put this through a CI run.
>>>>>>>>
>>>>>>>> Sughosh Ganu (5):
>>>>>>>>    dt: Provide a way to remove non-compliant nodes and properties
>>>>>>>>    fwu: Add the fwu-mdata node for removal from devicetree
>>>>>>>>    capsule: Add the capsule-key property for removal from devicetree
>>>>>>>>    bootefi: Call the EVT_FT_FIXUP event handler
>>>>>>>>    doc: Add a document for non-compliant DT node/property removal
>>>>>>>>
>>>>>>>>   cmd/bootefi.c                                 | 18 +++++
>>>>>>>>   .../devicetree/dt_non_compliant_purge.rst     | 64 ++++++++++++++++
>>>>>>>>   drivers/fwu-mdata/fwu-mdata-uclass.c          |  5 ++
>>>>>>>>   include/dt-structs.h                          | 11 +++
>>>>>>>>   lib/Makefile                                  |  1 +
>>>>>>>>   lib/dt_purge.c                                | 73 +++++++++++++++++++
>>>>>>>>   lib/efi_loader/efi_capsule.c                  |  7 ++
>>>>>>>>   7 files changed, 179 insertions(+)
>>>>>>>>   create mode 100644 doc/develop/devicetree/dt_non_compliant_purge.rst
>>>>>>>>   create mode 100644 lib/dt_purge.c
>>>>>>>
>>>>>>> What is the point of removing them? Instead, we should make sure that
>>>>>>> we upstream the bindings and encourage SoC vendors to sync them. If we
>>>>>>> remove them, no one will bother and U-Boot just becomes a dumping
>>>>>>> ground.
>>>>>>
>>>>>> Well things like the binman entries in DT are U-Boot specific and not
>>>>>> useful for HW related descriptions or for Linux or another OS being
>>>>>> able to deal with HW so arguably we're already a dumping ground to
>>>>>> some degree for not defining hardware.
>>>>>
>>>>> I have started the process to upstream the binman bindings.
>>>>
>>>> I don't think they should be in DT at all, they don't describe
>>>> anything to do with hardware, or generally even the runtime of a
>>>> device, they don't even describe the boot/runtime state of the
>>>> firmware, they describe build time, so I don't see what that has to do
>>>> with device tree? Can you explain that? To me those sorts of things
>>>> should live in a build time style config file.
>>
>> For the record, I tend to agree.
>>
>>> I beg to differ. Devicetree is more than just hardware and always has
>>> been. See, for example the /chosen and /options nodes.
>>
>> There are exceptions...
>>
>>> We need run-time configuration here, since we cannot know at build
>>> time what we will be asked to do by a previous firmware phase.
>>
>> Really, I don't want to have to care about the binman binding. If it is
>> u-boot specific, then it should stay in u-boot. I took /options/u-boot/,
>> but now I'm starting to have second thoughts on that being in dtschema
>> if it is going to be continually and frequently extended. Validating it
>> in SR does little. If a vendor is abusing /options/u-boot/ in some way
>> they could just as easily remove the node in their u-boot fork to pass.
>> SR is certainly not going to require the node be there.

The missing piece is validation of the U-Boot internal device-trees
against a schema in the U-Boot CI. This should be possible even if some
of the schema yaml files are maintained inside the U-Boot repo.

Best regards

Heinrich

>
> It's going to continue to be a "fun" tight-rope to walk. No one wants an
> easy way for vendors to cheat the requirements which is why in some
> other parts of this thread (you may or may not have been on, I don't
> recall, sorry) I've been trying to make it clear that the removal
> mechanism should be both a slight pain to add to, and at least wrt
> upstream, clear that effort was made to upstream things.  Cheaters are
> going to cheat and they could just chain a bunch of "fdt rm" together to
> do it today.
>

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Thu, Sep 07, 2023 at 01:30:01AM +0200, Heinrich Schuchardt wrote:

[snip]
> The missing piece is validation of the U-Boot internal device-trees
> against a schema in the U-Boot CI. This should be possible even if some
> of the schema yaml files are maintained inside the U-Boot repo.

Dropping Rob since I don't think he cares about this part.  Yes, what we
need is some help in re-syncing our Kbuild (and Kconfig, but..) logic
with the kernel as it's stuck at v4.20 still and all of the parts that
make running the validation targets would be much easier (free) if we
were in sync or much closer at least. It would be much appreciated if
Linaro or Canonical or one of the other companies doing U-Boot work
could get someone to focus on this task.  It would help with a number of
things, this included.

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Ilias Apalodimas]

Hi Rob,

[...]

> > > > > >
> > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > ground.
> > > > >
> > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > some degree for not defining hardware.
> > > >
> > > > I have started the process to upstream the binman bindings.
> > >
> > > I don't think they should be in DT at all, they don't describe
> > > anything to do with hardware, or generally even the runtime of a
> > > device, they don't even describe the boot/runtime state of the
> > > firmware, they describe build time, so I don't see what that has to do
> > > with device tree? Can you explain that? To me those sorts of things
> > > should live in a build time style config file.
>
> For the record, I tend to agree.
>

+1

> > I beg to differ. Devicetree is more than just hardware and always has
> > been. See, for example the /chosen and /options nodes.
>
> There are exceptions...
>

We've been this over and over again and frankly it gets a bit annoying.
It's called *DEVICE* tree for a reason.  As Rob pointed out there are
exceptions, but those made a lot of sense.  Having arbitrary internal ABI
stuff of various projects in the schema just defeats the definition of a
spec.

> > We need run-time configuration here, since we cannot know at build
> > time what we will be asked to do by a previous firmware phase.
>
> Really, I don't want to have to care about the binman binding. If it is
> u-boot specific, then it should stay in u-boot. I took /options/u-boot/,
> but now I'm starting to have second thoughts on that being in dtschema
> if it is going to be continually and frequently extended. Validating it
> in SR does little. If a vendor is abusing /options/u-boot/ in some way
> they could just as easily remove the node in their u-boot fork to pass.
> SR is certainly not going to require the node be there.
>
> On A/B updates, that really doesn't seem like a u-boot specific problem
> to me. No one wants A/B updates in EDK2 or anything else?

A/B updates might be implemented in EDK2 or any other bootloader that
chooses to implement it.  The metadata information a bootloader needs
to implement it are documented here [0].  Those metadata are not part of
the DT.  If they were it would make sense to add them on the schema.  The
DT entry we are using though serves a different purpose.  It tells the
bootloader the location of the metadata (iow where can I read them from the
disk).  Since bootloaders have a different way of storing their
configuration, I don't think this needs to be in the spec.  EDK2 for
example doesn't always use a DT and I don't think they'll ever use it to
store configuration information.

[0] https://developer.arm.com/documentation/den0118/b/?lang=en

Thanks
/Ilias
>
> Rob

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Ilias,

On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Rob,
>
> [...]
>
> > > > > > >
> > > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > > ground.
> > > > > >
> > > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > > some degree for not defining hardware.
> > > > >
> > > > > I have started the process to upstream the binman bindings.
> > > >
> > > > I don't think they should be in DT at all, they don't describe
> > > > anything to do with hardware, or generally even the runtime of a
> > > > device, they don't even describe the boot/runtime state of the
> > > > firmware, they describe build time, so I don't see what that has to do
> > > > with device tree? Can you explain that? To me those sorts of things
> > > > should live in a build time style config file.
> >
> > For the record, I tend to agree.
> >
>
> +1
>
> > > I beg to differ. Devicetree is more than just hardware and always has
> > > been. See, for example the /chosen and /options nodes.
> >
> > There are exceptions...
> >
>
> We've been this over and over again and frankly it gets a bit annoying.
> It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> stuff of various projects in the schema just defeats the definition of a
> spec.

Our efforts should not just be about internal ABI, but working to
provide a consistent configuration system for all firmware elements.

We cannot have it both ways, i.e. refusing to accept non-hardware
bindings and then complaining that U-Boot does not pass schema
validation. Devicetree should be a shared resource, not just for the
use of Linux. We already have reserved-memory, flash layout and other
things which don't relate to hardware. I would love to somehome get
past this fundamental discussion which seems to come up every time we
get close to making progress.

>
> > > We need run-time configuration here, since we cannot know at build
> > > time what we will be asked to do by a previous firmware phase.
> >
> > Really, I don't want to have to care about the binman binding. If it is
> > u-boot specific, then it should stay in u-boot. I took /options/u-boot/,
> > but now I'm starting to have second thoughts on that being in dtschema
> > if it is going to be continually and frequently extended. Validating it
> > in SR does little. If a vendor is abusing /options/u-boot/ in some way
> > they could just as easily remove the node in their u-boot fork to pass.
> > SR is certainly not going to require the node be there.
> >
> > On A/B updates, that really doesn't seem like a u-boot specific problem
> > to me. No one wants A/B updates in EDK2 or anything else?
>
> A/B updates might be implemented in EDK2 or any other bootloader that
> chooses to implement it.  The metadata information a bootloader needs
> to implement it are documented here [0].  Those metadata are not part of
> the DT.  If they were it would make sense to add them on the schema.  The
> DT entry we are using though serves a different purpose.  It tells the
> bootloader the location of the metadata (iow where can I read them from the
> disk).  Since bootloaders have a different way of storing their
> configuration, I don't think this needs to be in the spec.  EDK2 for
> example doesn't always use a DT and I don't think they'll ever use it to
> store configuration information.

In another thread with Rob involved, we are trying to provide bindings
for EDK2 to use.

Whatever the efforts of those trying to fragment the firmware
industry, there are efforts to bring it together with a consistent
configuration story. Ultimately I believe these will have to prevail,
as the complexity increases.

>
> [0] https://developer.arm.com/documentation/den0118/b/?lang=en
>
> Thanks
> /Ilias
> >
> > Rob

Regards,
Simon

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Ilias Apalodimas]

Hi Simon,

On Thu, 7 Sept 2023 at 15:23, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Rob,
> >
> > [...]
> >
> > > > > > > >
> > > > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > > > ground.
> > > > > > >
> > > > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > > > some degree for not defining hardware.
> > > > > >
> > > > > > I have started the process to upstream the binman bindings.
> > > > >
> > > > > I don't think they should be in DT at all, they don't describe
> > > > > anything to do with hardware, or generally even the runtime of a
> > > > > device, they don't even describe the boot/runtime state of the
> > > > > firmware, they describe build time, so I don't see what that has to do
> > > > > with device tree? Can you explain that? To me those sorts of things
> > > > > should live in a build time style config file.
> > >
> > > For the record, I tend to agree.
> > >
> >
> > +1
> >
> > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > been. See, for example the /chosen and /options nodes.
> > >
> > > There are exceptions...
> > >
> >
> > We've been this over and over again and frankly it gets a bit annoying.
> > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > stuff of various projects in the schema just defeats the definition of a
> > spec.
>
> Our efforts should not just be about internal ABI, but working to
> provide a consistent configuration system for all firmware elements.

And that's what the firmware handoff was all about.
I get what you are trying to do here.  I am just aware of any other
project apart from U-Boot which uses DT for it's own configuration.
So trying to standardize some bindings that are useful to all projects
that use DT is fine. Trying to *enforce* them to use it for config
isn't IMHO.

>
> We cannot have it both ways, i.e. refusing to accept non-hardware
> bindings and then complaining that U-Boot does not pass schema
> validation. Devicetree should be a shared resource, not just for the
> use of Linux.

It's not for the use of Linux, I've wasted enough time repeating that
and so has Rob.  Please go back to previous emails and read the
arguments.

> We already have reserved-memory, flash layout and other
> things which don't relate to hardware. I would love to somehome get
> past this fundamental discussion which seems to come up every time we
> get close to making progress

Most of the nodes we already have were used across projects and made
sense to all of them. U-Boot might need to reserve some memory and so
does linux etc etc.
Some other nodes make nosense at all to and they just serve internal
ABI implementation details.  I can't possibly fathom how these would
be justifiable.  On top of all that, there's a huge danger here.  How
are you planning on separating arbitrary entries from various
projects?

What I am afraid is going to happen here is simple.  If a project
doesn't use DT to configure itself and wants to provide a DT to
U-Boot, then are you going to say "Can you please inject various DT
nodes in the tree because U-Boot *needs* them and they are now part of
the spec"?  Anyway, it's not up to me to decide here, I am just saying
what makes sense to me.

>
> >
> > > > We need run-time configuration here, since we cannot know at build
> > > > time what we will be asked to do by a previous firmware phase.
> > >
> > > Really, I don't want to have to care about the binman binding. If it is
> > > u-boot specific, then it should stay in u-boot. I took /options/u-boot/,
> > > but now I'm starting to have second thoughts on that being in dtschema
> > > if it is going to be continually and frequently extended. Validating it
> > > in SR does little. If a vendor is abusing /options/u-boot/ in some way
> > > they could just as easily remove the node in their u-boot fork to pass.
> > > SR is certainly not going to require the node be there.
> > >
> > > On A/B updates, that really doesn't seem like a u-boot specific problem
> > > to me. No one wants A/B updates in EDK2 or anything else?
> >
> > A/B updates might be implemented in EDK2 or any other bootloader that
> > chooses to implement it.  The metadata information a bootloader needs
> > to implement it are documented here [0].  Those metadata are not part of
> > the DT.  If they were it would make sense to add them on the schema.  The
> > DT entry we are using though serves a different purpose.  It tells the
> > bootloader the location of the metadata (iow where can I read them from the
> > disk).  Since bootloaders have a different way of storing their
> > configuration, I don't think this needs to be in the spec.  EDK2 for
> > example doesn't always use a DT and I don't think they'll ever use it to
> > store configuration information.
>
> In another thread with Rob involved, we are trying to provide bindings
> for EDK2 to use.
>
> Whatever the efforts of those trying to fragment the firmware
> industry, there are efforts to bring it together with a consistent
> configuration story. Ultimately I believe these will have to prevail,
> as the complexity increases.


Regards
/Ilias

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Jassi Brar]

Hi Simon,

On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> >
> > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > been. See, for example the /chosen and /options nodes.
> > >
> > > There are exceptions...
> > >
> >
> > We've been this over and over again and frankly it gets a bit annoying.
> > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > stuff of various projects in the schema just defeats the definition of a
> > spec.
>
> Our efforts should not just be about internal ABI, but working to
> provide a consistent configuration system for all firmware elements.
>
Sure, programmatically we can pass any data/info via DT, however it is
only meant to map hardware features onto data structures.

devicetree.org  landing page
    "The devicetree is a data structure for describing hardware."

devicetree-specification-v0.3.pdf  Chapter-2 Line-1
   "DTSpec specifies a construct called a devicetree to describe
system hardware."

If we want to digress from the spec, we need the majority of
developers to switch sides :)  which is unlikely to happen and rightly
so, imho.

regards.

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Fri, Sep 08, 2023 at 09:37:12AM -0500, Jassi Brar wrote:
> Hi Simon,
> 
> On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > >
> > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > been. See, for example the /chosen and /options nodes.
> > > >
> > > > There are exceptions...
> > > >
> > >
> > > We've been this over and over again and frankly it gets a bit annoying.
> > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > stuff of various projects in the schema just defeats the definition of a
> > > spec.
> >
> > Our efforts should not just be about internal ABI, but working to
> > provide a consistent configuration system for all firmware elements.
> >
> Sure, programmatically we can pass any data/info via DT, however it is
> only meant to map hardware features onto data structures.
> 
> devicetree.org  landing page
>     "The devicetree is a data structure for describing hardware."
> 
> devicetree-specification-v0.3.pdf  Chapter-2 Line-1
>    "DTSpec specifies a construct called a devicetree to describe
> system hardware."
> 
> If we want to digress from the spec, we need the majority of
> developers to switch sides :)  which is unlikely to happen and rightly
> so, imho.

It's the same language-lawyering that's been going on since device trees
moved from the neat thing Apple-based PowerPC platforms provided (and
extended), and then got ad-hoc'd to support other PowerPC platforms, and
then was used to solve "Linus is sick of the patch conflicts on ARM"
problems.  And everyone is tired of talking about the exceptions to "it
must be hardware" because if your software isn't where the
software-burned-into-the-hardware wants it to be, the hardware won't
work and so on.

-- 
Tom

p.s. Yes, I know Apple didn't invent device trees either.

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Fri, Sep 08, 2023 at 01:13:42PM +0300, Ilias Apalodimas wrote:
> Hi Simon,
> 
> On Thu, 7 Sept 2023 at 15:23, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Rob,
> > >
> > > [...]
> > >
> > > > > > > > >
> > > > > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > > > > ground.
> > > > > > > >
> > > > > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > > > > some degree for not defining hardware.
> > > > > > >
> > > > > > > I have started the process to upstream the binman bindings.
> > > > > >
> > > > > > I don't think they should be in DT at all, they don't describe
> > > > > > anything to do with hardware, or generally even the runtime of a
> > > > > > device, they don't even describe the boot/runtime state of the
> > > > > > firmware, they describe build time, so I don't see what that has to do
> > > > > > with device tree? Can you explain that? To me those sorts of things
> > > > > > should live in a build time style config file.
> > > >
> > > > For the record, I tend to agree.
> > > >
> > >
> > > +1
> > >
> > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > been. See, for example the /chosen and /options nodes.
> > > >
> > > > There are exceptions...
> > > >
> > >
> > > We've been this over and over again and frankly it gets a bit annoying.
> > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > stuff of various projects in the schema just defeats the definition of a
> > > spec.
> >
> > Our efforts should not just be about internal ABI, but working to
> > provide a consistent configuration system for all firmware elements.
> 
> And that's what the firmware handoff was all about.
> I get what you are trying to do here.  I am just aware of any other

"just not aware" did you mean?

> project apart from U-Boot which uses DT for it's own configuration.
> So trying to standardize some bindings that are useful to all projects
> that use DT is fine. Trying to *enforce* them to use it for config
> isn't IMHO.
> 
> >
> > We cannot have it both ways, i.e. refusing to accept non-hardware
> > bindings and then complaining that U-Boot does not pass schema
> > validation. Devicetree should be a shared resource, not just for the
> > use of Linux.
> 
> It's not for the use of Linux, I've wasted enough time repeating that
> and so has Rob.  Please go back to previous emails and read the
> arguments.

Right, it's not just for Linux, but Linux is where most of the
exceptions to the "ONLY HARDWARE" rule got in, because they also make
sense.  And the overarching point Simon keeps trying to make I believe
can be boiled down to that too.  There are things that one does not have
a (reasonable) choice about how to do things with when interacting with
the hunk of melted sand on your desk and that information needs to go
somewhere.

> > We already have reserved-memory, flash layout and other
> > things which don't relate to hardware. I would love to somehome get
> > past this fundamental discussion which seems to come up every time we
> > get close to making progress
> 
> Most of the nodes we already have were used across projects and made
> sense to all of them. U-Boot might need to reserve some memory and so
> does linux etc etc.
> Some other nodes make nosense at all to and they just serve internal
> ABI implementation details.  I can't possibly fathom how these would
> be justifiable.  On top of all that, there's a huge danger here.  How
> are you planning on separating arbitrary entries from various
> projects?

I think in some ways this is the whole point of at least what I'm asking
for.  It's fine to say "Here is the mechanism to remove nodes /
properties from the device tree".  BUT adding entries to that list MUST
document where someone tried to upstream and explain that this is
something that belongs in the device tree because it is useful to
everyone.

> What I am afraid is going to happen here is simple.  If a project
> doesn't use DT to configure itself and wants to provide a DT to
> U-Boot, then are you going to say "Can you please inject various DT
> nodes in the tree because U-Boot *needs* them and they are now part of
> the spec"?  Anyway, it's not up to me to decide here, I am just saying
> what makes sense to me.

What's the difference between that and "If a project doesn't use DT to
configure itself and wants to provide a DT to Linux, ..." ?

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Ilias Apalodimas]

Hi Tom,

On Fri, 8 Sept 2023 at 17:54, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Sep 08, 2023 at 01:13:42PM +0300, Ilias Apalodimas wrote:
> > Hi Simon,
> >
> > On Thu, 7 Sept 2023 at 15:23, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Rob,
> > > >
> > > > [...]
> > > >
> > > > > > > > > >
> > > > > > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > > > > > ground.
> > > > > > > > >
> > > > > > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > > > > > some degree for not defining hardware.
> > > > > > > >
> > > > > > > > I have started the process to upstream the binman bindings.
> > > > > > >
> > > > > > > I don't think they should be in DT at all, they don't describe
> > > > > > > anything to do with hardware, or generally even the runtime of a
> > > > > > > device, they don't even describe the boot/runtime state of the
> > > > > > > firmware, they describe build time, so I don't see what that has to do
> > > > > > > with device tree? Can you explain that? To me those sorts of things
> > > > > > > should live in a build time style config file.
> > > > >
> > > > > For the record, I tend to agree.
> > > > >
> > > >
> > > > +1
> > > >
> > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > been. See, for example the /chosen and /options nodes.
> > > > >
> > > > > There are exceptions...
> > > > >
> > > >
> > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > stuff of various projects in the schema just defeats the definition of a
> > > > spec.
> > >
> > > Our efforts should not just be about internal ABI, but working to
> > > provide a consistent configuration system for all firmware elements.
> >
> > And that's what the firmware handoff was all about.
> > I get what you are trying to do here.  I am just aware of any other
>
> "just not aware" did you mean?

Yep, sorry!

>
> > project apart from U-Boot which uses DT for it's own configuration.
> > So trying to standardize some bindings that are useful to all projects
> > that use DT is fine. Trying to *enforce* them to use it for config
> > isn't IMHO.
> >
> > >
> > > We cannot have it both ways, i.e. refusing to accept non-hardware
> > > bindings and then complaining that U-Boot does not pass schema
> > > validation. Devicetree should be a shared resource, not just for the
> > > use of Linux.
> >
> > It's not for the use of Linux, I've wasted enough time repeating that
> > and so has Rob.  Please go back to previous emails and read the
> > arguments.
>
> Right, it's not just for Linux, but Linux is where most of the
> exceptions to the "ONLY HARDWARE" rule got in, because they also make
> sense.

Exactly.

> And the overarching point Simon keeps trying to make I believe
> can be boiled down to that too.  There are things that one does not have
> a (reasonable) choice about how to do things with when interacting with
> the hunk of melted sand on your desk and that information needs to go
> somewhere.

DT is a big hammer indeed, but that doesn't mean we always need to use
it.  I never disagreed with adding nodes that make sense and will be
useful for others. For example, the internal Driver model
configuration options used to enable a device early etc etc are
probably useful to more projects.  On the other hand, if U-Boot is
indeed the only project using DT for its internal configuration why
should we care?

For example, let's imagine you build TF-A, and TF-A is configured and
bundled with a device tree that gets passed along to U-Boot (using
OF_BOARD).  Why on earth should TF-A be aware of internal DM
implementation details and build a device tree containing
u-boot,dm-pre-reloc, u-boot,dm-spl, dm-tpl, and every other
non-upstreamed nodes we have?
Another example would be the public key that we shoehorn on the DT.
In commit ddf67daac39d ("efi_capsule: Move signature from DTB to
.rodata") I tried to get rid of that because since I was aware of the
dt-schema conformance and honestly having the capsule public portion
of the key there makes little sense.  Unfortunately, that got reverted
in commit 47a25e81d35c8 with a bogus commit message as well.  So again
imagine building TF-A, which is a first-stage bootloader and has no
understanding of UEFI whatsoever,  asking the TF-A project to start
injecting public keys around that has no idea why or how they will be
used.

 Can you imagine how the device tree would look like in a couple of
years from now if we allow *every* project to add its own special
config needs in there?  So perhaps we should take a step back, agree
that some level of config is needed, identify the common options, and
add that to the spec instead of dumping everything that doesn't fit
somewhere else in there.

>
> > > We already have reserved-memory, flash layout and other
> > > things which don't relate to hardware. I would love to somehome get
> > > past this fundamental discussion which seems to come up every time we
> > > get close to making progress
> >
> > Most of the nodes we already have were used across projects and made
> > sense to all of them. U-Boot might need to reserve some memory and so
> > does linux etc etc.
> > Some other nodes make nosense at all to and they just serve internal
> > ABI implementation details.  I can't possibly fathom how these would
> > be justifiable.  On top of all that, there's a huge danger here.  How
> > are you planning on separating arbitrary entries from various
> > projects?
>
> I think in some ways this is the whole point of at least what I'm asking
> for.  It's fine to say "Here is the mechanism to remove nodes /
> properties from the device tree".  BUT adding entries to that list MUST
> document where someone tried to upstream and explain that this is
> something that belongs in the device tree because it is useful to
> everyone.

And we don't disagree on that either. That's why the link to the FWU
discussion was there (although it should have been in a doc and not in
a mail). I am not arguing against adding nodes, I am arguing that we
shouldn't rush them and that there's zero chance that we manage to
upstream everything and keep some level of sanity on the spec.
So, since U-Boot is currently using the DT for its own configuration
needs, not having the ability to provide a DT that conforms to the
spec and hope that we can upstream everything will just delay all of
SystemReady 2.0 conformance efforts (and is unrealistic IMHO).
>
> > What I am afraid is going to happen here is simple.  If a project
> > doesn't use DT to configure itself and wants to provide a DT to
> > U-Boot, then are you going to say "Can you please inject various DT
> > nodes in the tree because U-Boot *needs* them and they are now part of
> > the spec"?  Anyway, it's not up to me to decide here, I am just saying
> > what makes sense to me.
>
> What's the difference between that and "If a project doesn't use DT to
> configure itself and wants to provide a DT to Linux, ..." ?

See the example above with TF-A, I hope that clears it up.

Thanks
/Ilias
>
> --
> Tom

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Mark Kettenis]

> From: Jassi Brar <jassisinghbrar@gmail.com>
> Date: Fri, 8 Sep 2023 09:37:12 -0500
> 
> Hi Simon,
> 
> On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > >
> > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > been. See, for example the /chosen and /options nodes.
> > > >
> > > > There are exceptions...
> > > >
> > >
> > > We've been this over and over again and frankly it gets a bit annoying.
> > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > stuff of various projects in the schema just defeats the definition of a
> > > spec.
> >
> > Our efforts should not just be about internal ABI, but working to
> > provide a consistent configuration system for all firmware elements.
> >
> Sure, programmatically we can pass any data/info via DT, however it is
> only meant to map hardware features onto data structures.
> 
> devicetree.org  landing page
>     "The devicetree is a data structure for describing hardware."
> 
> devicetree-specification-v0.3.pdf  Chapter-2 Line-1
>    "DTSpec specifies a construct called a devicetree to describe
> system hardware."

But it doesn't say that it describes *only* hardware.  And it does
describe more than just hardware.  There is /chosen to specify
firmware configuration and there are several examples of device tree
bindings that describe non-discoverable firmware interfaces in the
Linux tree.  And it has been that way since the days of IEEE 1275.
There are also bindings describing things like the firmware partition
layout.

> If we want to digress from the spec, we need the majority of
> developers to switch sides :)  which is unlikely to happen and rightly
> so, imho.

It isn't even clear what the spec is.  There is the document you
reference above, there are the yaml files at
https://github.com/devicetree-org/dt-schema and then there are
additional yaml files in the Linux tree.  As far as I know it isn't
written down anywhere that those are the only places where device tree
bindings can live.

Anyway, let's face it, there is some consensus among developers that
what Simon has done in U-Boot is pushing the use of devicetree beyond
the point where a significant fraction of developers thinks it makes
sense.  And I think I agree with that.  But you can't beat him with
the spec to make your point.

Now the devicetree is cleverly constructed such that it is possible to
define additional bindings without the risk of conflicting with
bindings developed by other parties.  In particular if U-Boot is
augmenting a device tree with properties that are prefixed with
"u-boot," this isn't going to hurt an operating system that uses such
an augmented device tree.

The real problem is that some folks developed a certification program
that allegedly requires schema verification and now propose adding
code to U-Boot that doesn't really solve any problem.  My proposed
solution would be to change said certification program to allow
firmware to augment the device tree with properties and nodes with
compatibles that are in the namespace controlled by the firmware.

Cheers,

Mark

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Fri, Sep 08, 2023 at 06:28:14PM +0300, Ilias Apalodimas wrote:
> Hi Tom,
> 
> On Fri, 8 Sept 2023 at 17:54, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Sep 08, 2023 at 01:13:42PM +0300, Ilias Apalodimas wrote:
> > > Hi Simon,
> > >
> > > On Thu, 7 Sept 2023 at 15:23, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Rob,
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > > > >
> > > > > > > > > > > What is the point of removing them? Instead, we should make sure that
> > > > > > > > > > > we upstream the bindings and encourage SoC vendors to sync them. If we
> > > > > > > > > > > remove them, no one will bother and U-Boot just becomes a dumping
> > > > > > > > > > > ground.
> > > > > > > > > >
> > > > > > > > > > Well things like the binman entries in DT are U-Boot specific and not
> > > > > > > > > > useful for HW related descriptions or for Linux or another OS being
> > > > > > > > > > able to deal with HW so arguably we're already a dumping ground to
> > > > > > > > > > some degree for not defining hardware.
> > > > > > > > >
> > > > > > > > > I have started the process to upstream the binman bindings.
> > > > > > > >
> > > > > > > > I don't think they should be in DT at all, they don't describe
> > > > > > > > anything to do with hardware, or generally even the runtime of a
> > > > > > > > device, they don't even describe the boot/runtime state of the
> > > > > > > > firmware, they describe build time, so I don't see what that has to do
> > > > > > > > with device tree? Can you explain that? To me those sorts of things
> > > > > > > > should live in a build time style config file.
> > > > > >
> > > > > > For the record, I tend to agree.
> > > > > >
> > > > >
> > > > > +1
> > > > >
> > > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > > been. See, for example the /chosen and /options nodes.
> > > > > >
> > > > > > There are exceptions...
> > > > > >
> > > > >
> > > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > > stuff of various projects in the schema just defeats the definition of a
> > > > > spec.
> > > >
> > > > Our efforts should not just be about internal ABI, but working to
> > > > provide a consistent configuration system for all firmware elements.
> > >
> > > And that's what the firmware handoff was all about.
> > > I get what you are trying to do here.  I am just aware of any other
> >
> > "just not aware" did you mean?
> 
> Yep, sorry!
> 
> >
> > > project apart from U-Boot which uses DT for it's own configuration.
> > > So trying to standardize some bindings that are useful to all projects
> > > that use DT is fine. Trying to *enforce* them to use it for config
> > > isn't IMHO.
> > >
> > > >
> > > > We cannot have it both ways, i.e. refusing to accept non-hardware
> > > > bindings and then complaining that U-Boot does not pass schema
> > > > validation. Devicetree should be a shared resource, not just for the
> > > > use of Linux.
> > >
> > > It's not for the use of Linux, I've wasted enough time repeating that
> > > and so has Rob.  Please go back to previous emails and read the
> > > arguments.
> >
> > Right, it's not just for Linux, but Linux is where most of the
> > exceptions to the "ONLY HARDWARE" rule got in, because they also make
> > sense.
> 
> Exactly.
> 
> > And the overarching point Simon keeps trying to make I believe
> > can be boiled down to that too.  There are things that one does not have
> > a (reasonable) choice about how to do things with when interacting with
> > the hunk of melted sand on your desk and that information needs to go
> > somewhere.
> 
> DT is a big hammer indeed, but that doesn't mean we always need to use
> it.  I never disagreed with adding nodes that make sense and will be
> useful for others. For example, the internal Driver model
> configuration options used to enable a device early etc etc are
> probably useful to more projects.  On the other hand, if U-Boot is
> indeed the only project using DT for its internal configuration why
> should we care?
> 
> For example, let's imagine you build TF-A, and TF-A is configured and
> bundled with a device tree that gets passed along to U-Boot (using
> OF_BOARD).  Why on earth should TF-A be aware of internal DM
> implementation details and build a device tree containing
> u-boot,dm-pre-reloc, u-boot,dm-spl, dm-tpl, and every other
> non-upstreamed nodes we have?

I don't think this is a clear example, sorry.  "dm-pre-reloc" etc are
the bootph things now that you say could be useful.  So they're an
example of how (now that things are more receptive) we need to look at
what U-Boot has that doesn't pass validation and see "does this make
sense, today" or not.

I guess I'm confused as to why it's a theoretical problem for TF-A to
pass along /binman/ but not a problem to pass along
/soc/.../snvs/.../linux,snvs_pwrkey on i.MX8.  _Sometimes_ internals
just need to be there.  That also does not mean every single should be
there.

> Another example would be the public key that we shoehorn on the DT.
> In commit ddf67daac39d ("efi_capsule: Move signature from DTB to
> .rodata") I tried to get rid of that because since I was aware of the
> dt-schema conformance and honestly having the capsule public portion
> of the key there makes little sense.  Unfortunately, that got reverted
> in commit 47a25e81d35c8 with a bogus commit message as well.  So again
> imagine building TF-A, which is a first-stage bootloader and has no
> understanding of UEFI whatsoever,  asking the TF-A project to start
> injecting public keys around that has no idea why or how they will be
> used.
> 
>  Can you imagine how the device tree would look like in a couple of
> years from now if we allow *every* project to add its own special
> config needs in there?  So perhaps we should take a step back, agree
> that some level of config is needed, identify the common options, and
> add that to the spec instead of dumping everything that doesn't fit
> somewhere else in there.

Part of the problem here and now with capsule update stuff seems to be
that, well, I don't know what the heck we should do.  It's a "lovely"
specification defined feature and so I honestly don't know how much
leeway we have for how we can and can't represent and implement the
portions that are left up to the implementation or board specific.  I
don't see why TF-A would inject something that should have been present
already?  And/or ...

> > > > We already have reserved-memory, flash layout and other
> > > > things which don't relate to hardware. I would love to somehome get
> > > > past this fundamental discussion which seems to come up every time we
> > > > get close to making progress
> > >
> > > Most of the nodes we already have were used across projects and made
> > > sense to all of them. U-Boot might need to reserve some memory and so
> > > does linux etc etc.
> > > Some other nodes make nosense at all to and they just serve internal
> > > ABI implementation details.  I can't possibly fathom how these would
> > > be justifiable.  On top of all that, there's a huge danger here.  How
> > > are you planning on separating arbitrary entries from various
> > > projects?
> >
> > I think in some ways this is the whole point of at least what I'm asking
> > for.  It's fine to say "Here is the mechanism to remove nodes /
> > properties from the device tree".  BUT adding entries to that list MUST
> > document where someone tried to upstream and explain that this is
> > something that belongs in the device tree because it is useful to
> > everyone.
> 
> And we don't disagree on that either. That's why the link to the FWU
> discussion was there (although it should have been in a doc and not in
> a mail). I am not arguing against adding nodes, I am arguing that we
> shouldn't rush them and that there's zero chance that we manage to
> upstream everything and keep some level of sanity on the spec.
> So, since U-Boot is currently using the DT for its own configuration
> needs, not having the ability to provide a DT that conforms to the
> spec and hope that we can upstream everything will just delay all of
> SystemReady 2.0 conformance efforts (and is unrealistic IMHO).

The first problem is how does the capsule update specification specify
handling the stuff that we put in the FWU nodes that we then need to
delete?

The second problem is that I don't want the discussion link to just be
in the cover letter, I want it in the tree, in documentation and heck,
an unused-by-the-compiler parameter in the macro that adds a node to
delete that is the rST file that documents the "we tried, it was
rejected, this still makes sense" or whatever is appropriate as to why
we're deleting the node.  Cheaters shall cheat here, yes, but upstream
will have a record of trying.

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Ilias Apalodimas]

Hi Tom,

[...]

> > > > > > > > > I don't think they should be in DT at all, they don't describe
> > > > > > > > > anything to do with hardware, or generally even the runtime of a
> > > > > > > > > device, they don't even describe the boot/runtime state of the
> > > > > > > > > firmware, they describe build time, so I don't see what that has to do
> > > > > > > > > with device tree? Can you explain that? To me those sorts of things
> > > > > > > > > should live in a build time style config file.
> > > > > > >
> > > > > > > For the record, I tend to agree.
> > > > > > >
> > > > > >
> > > > > > +1
> > > > > >
> > > > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > > > been. See, for example the /chosen and /options nodes.
> > > > > > >
> > > > > > > There are exceptions...
> > > > > > >
> > > > > >
> > > > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > > > stuff of various projects in the schema just defeats the definition of a
> > > > > > spec.
> > > > >
> > > > > Our efforts should not just be about internal ABI, but working to
> > > > > provide a consistent configuration system for all firmware elements.
> > > >
> > > > And that's what the firmware handoff was all about.
> > > > I get what you are trying to do here.  I am just aware of any other
> > >
> > > "just not aware" did you mean?
> >
> > Yep, sorry!
> >
> > >
> > > > project apart from U-Boot which uses DT for it's own configuration.
> > > > So trying to standardize some bindings that are useful to all projects
> > > > that use DT is fine. Trying to *enforce* them to use it for config
> > > > isn't IMHO.
> > > >
> > > > >
> > > > > We cannot have it both ways, i.e. refusing to accept non-hardware
> > > > > bindings and then complaining that U-Boot does not pass schema
> > > > > validation. Devicetree should be a shared resource, not just for the
> > > > > use of Linux.
> > > >
> > > > It's not for the use of Linux, I've wasted enough time repeating that
> > > > and so has Rob.  Please go back to previous emails and read the
> > > > arguments.
> > >
> > > Right, it's not just for Linux, but Linux is where most of the
> > > exceptions to the "ONLY HARDWARE" rule got in, because they also make
> > > sense.
> >
> > Exactly.
> >
> > > And the overarching point Simon keeps trying to make I believe
> > > can be boiled down to that too.  There are things that one does not have
> > > a (reasonable) choice about how to do things with when interacting with
> > > the hunk of melted sand on your desk and that information needs to go
> > > somewhere.
> >
> > DT is a big hammer indeed, but that doesn't mean we always need to use
> > it.  I never disagreed with adding nodes that make sense and will be
> > useful for others. For example, the internal Driver model
> > configuration options used to enable a device early etc etc are
> > probably useful to more projects.  On the other hand, if U-Boot is
> > indeed the only project using DT for its internal configuration why
> > should we care?
> >
> > For example, let's imagine you build TF-A, and TF-A is configured and
> > bundled with a device tree that gets passed along to U-Boot (using
> > OF_BOARD).  Why on earth should TF-A be aware of internal DM
> > implementation details and build a device tree containing
> > u-boot,dm-pre-reloc, u-boot,dm-spl, dm-tpl, and every other
> > non-upstreamed nodes we have?
>
> I don't think this is a clear example, sorry.  "dm-pre-reloc" etc are
> the bootph things now that you say could be useful.  So they're an
> example of how (now that things are more receptive) we need to look at
> what U-Boot has that doesn't pass validation and see "does this make
> sense, today" or not.
>

The point here is a bit different though.  We need this in U-Boot *because*
we use the DT to configure things.  They are useful information, but unless
another bootloader uses the same config method, U-Boot is the only consumer.

If we could split those nodes in an internal u-boot .dtsi file that would
be much much cleaner.  But IIRC we'll have problems with patching DTs in
TPL/SPL with limited memory.

> I guess I'm confused as to why it's a theoretical problem for TF-A to
> pass along /binman/ but not a problem to pass along
> /soc/.../snvs/.../linux,snvs_pwrkey on i.MX8.  _Sometimes_ internals

It's the same problem and I don't think it's ok for TF-A to pass those as
either.

> just need to be there.  That also does not mean every single should be
> there.
>

> > Another example would be the public key that we shoehorn on the DT.
> > In commit ddf67daac39d ("efi_capsule: Move signature from DTB to
> > .rodata") I tried to get rid of that because since I was aware of the
> > dt-schema conformance and honestly having the capsule public portion
> > of the key there makes little sense.  Unfortunately, that got reverted
> > in commit 47a25e81d35c8 with a bogus commit message as well.  So again
> > imagine building TF-A, which is a first-stage bootloader and has no
> > understanding of UEFI whatsoever,  asking the TF-A project to start
> > injecting public keys around that has no idea why or how they will be
> > used.
> >
> >  Can you imagine how the device tree would look like in a couple of
> > years from now if we allow *every* project to add its own special
> > config needs in there?  So perhaps we should take a step back, agree
> > that some level of config is needed, identify the common options, and
> > add that to the spec instead of dumping everything that doesn't fit
> > somewhere else in there.
>
> Part of the problem here and now with capsule update stuff seems to be
> that, well, I don't know what the heck we should do.  It's a "lovely"
> specification defined feature and so I honestly don't know how much
> leeway we have for how we can and can't represent and implement the
> portions that are left up to the implementation or board specific.

Heinrich and I can help on that.  In short, the capsule update chapter
doesn't define where the key should be stored.  It should obviously be on a
tamper resistant medium and it has a specific format, but that's as far as
the spec would go.

> I don't see why TF-A would inject something that should have been present
> already?  And/or ...

I am not following you here.  The public key is unique per class of
devices.  If someone builds TF-A and decides to provide a DTB though that,
you then need to, unpack the TF-A binary when you build U-Boot,  amend it
and repack it via binman.  On top of that, those binaries will
probably be signed, so the repacking exercise becomes pretty painful.

>
> > > > > We already have reserved-memory, flash layout and other
> > > > > things which don't relate to hardware. I would love to somehome get
> > > > > past this fundamental discussion which seems to come up every time we
> > > > > get close to making progress
> > > >
> > > > Most of the nodes we already have were used across projects and made
> > > > sense to all of them. U-Boot might need to reserve some memory and so
> > > > does linux etc etc.
> > > > Some other nodes make nosense at all to and they just serve internal
> > > > ABI implementation details.  I can't possibly fathom how these would
> > > > be justifiable.  On top of all that, there's a huge danger here.  How
> > > > are you planning on separating arbitrary entries from various
> > > > projects?
> > >
> > > I think in some ways this is the whole point of at least what I'm asking
> > > for.  It's fine to say "Here is the mechanism to remove nodes /
> > > properties from the device tree".  BUT adding entries to that list MUST
> > > document where someone tried to upstream and explain that this is
> > > something that belongs in the device tree because it is useful to
> > > everyone.

We never disagreed on that.  I already said that the FWU link Sughosh sent
in the cover letter should be added on a doc.  But that's irrelevant to
'hard NAKing' patches [0].  It's also the complete opposite of what we are
discussing here, since AFAICT you are fine with the removal mechanism as
long as the nodes-to-be-removed are documented properly and there has been
an upstream effort of those beforehand.

> >
> > And we don't disagree on that either. That's why the link to the FWU
> > discussion was there (although it should have been in a doc and not in
> > a mail). I am not arguing against adding nodes, I am arguing that we
> > shouldn't rush them and that there's zero chance that we manage to
> > upstream everything and keep some level of sanity on the spec.
> > So, since U-Boot is currently using the DT for its own configuration
> > needs, not having the ability to provide a DT that conforms to the
> > spec and hope that we can upstream everything will just delay all of
> > SystemReady 2.0 conformance efforts (and is unrealistic IMHO).
>
> The first problem is how does the capsule update specification specify
> handling the stuff that we put in the FWU nodes that we then need to
> delete?
>

It doesn't.  The A/B update support is not part of the spec.  FWU and the
A/B updates are designed on top of the EFI spec to provide an easy way to
do firmware updates without bricking the board while at the same time
provide rollback protection.

> The second problem is that I don't want the discussion link to just be
> in the cover letter, I want it in the tree, in documentation and heck,
> an unused-by-the-compiler parameter in the macro that adds a node to
> delete that is the rST file that documents the "we tried, it was
> rejected, this still makes sense" or whatever is appropriate as to why
> we're deleting the node.  Cheaters shall cheat here, yes, but upstream
> will have a record of trying.

Again, we never disagreed on that

[0] https://lore.kernel.org/u-boot/CAPnjgZ3AexW4vfO-A8WYGE0OD5EZnOUA7tA1QP71Bcw51QArBQ@mail.gmail.com/

Thanks
/Ilias

>
> --
> Tom

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Rob Herring]

On Fri, Sep 8, 2023 at 1:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Jassi Brar <jassisinghbrar@gmail.com>
> > Date: Fri, 8 Sep 2023 09:37:12 -0500
> >
> > Hi Simon,
> >
> > On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > >
> > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > been. See, for example the /chosen and /options nodes.
> > > > >
> > > > > There are exceptions...
> > > > >
> > > >
> > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > stuff of various projects in the schema just defeats the definition of a
> > > > spec.
> > >
> > > Our efforts should not just be about internal ABI, but working to
> > > provide a consistent configuration system for all firmware elements.
> > >
> > Sure, programmatically we can pass any data/info via DT, however it is
> > only meant to map hardware features onto data structures.
> >
> > devicetree.org  landing page
> >     "The devicetree is a data structure for describing hardware."
> >
> > devicetree-specification-v0.3.pdf  Chapter-2 Line-1
> >    "DTSpec specifies a construct called a devicetree to describe
> > system hardware."
>
> But it doesn't say that it describes *only* hardware.  And it does
> describe more than just hardware.  There is /chosen to specify
> firmware configuration and there are several examples of device tree
> bindings that describe non-discoverable firmware interfaces in the
> Linux tree.  And it has been that way since the days of IEEE 1275.
> There are also bindings describing things like the firmware partition
> layout.

Yes. The exact title for 1275[1] is: IEEE Standard for Boot
(Initialization Configuration)
Firmware: Core Requirements and Practices

I see "configuration" in there. However, in the OF case, it's
generally how firmware configured the hardware and what it discovered.
That's a little different than telling the OS how to configure the
hardware which is what we do with FDT. Then there's stuff that's how
to configure Linux which we try to reject.

Once we get into configuration of the OS/client (including u-boot),
making that an ABI is a lot harder and if we use DT for that, I don't
think it should be an ABI.

> > If we want to digress from the spec, we need the majority of
> > developers to switch sides :)  which is unlikely to happen and rightly
> > so, imho.
>
> It isn't even clear what the spec is.  There is the document you
> reference above, there are the yaml files at
> https://github.com/devicetree-org/dt-schema and then there are
> additional yaml files in the Linux tree.  As far as I know it isn't
> written down anywhere that those are the only places where device tree
> bindings can live.

There's not any restriction.

My intention with dtschema schemas is to only have "spec level"
schemas. (Stuff we'd add to DTSpec (but don't because no one wants to
write specs).) Hardware specific stuff lives somewhere else. That
happens to be the Linux tree because that is where all the h/w support
is (nothing else is close (by far)). Last I checked, we've got ~3700
schemas and ~1500 unconverted bindings.

> Anyway, let's face it, there is some consensus among developers that
> what Simon has done in U-Boot is pushing the use of devicetree beyond
> the point where a significant fraction of developers thinks it makes
> sense.  And I think I agree with that.  But you can't beat him with
> the spec to make your point.
>
> Now the devicetree is cleverly constructed such that it is possible to
> define additional bindings without the risk of conflicting with
> bindings developed by other parties.  In particular if U-Boot is
> augmenting a device tree with properties that are prefixed with
> "u-boot," this isn't going to hurt an operating system that uses such
> an augmented device tree.

Until someone has some great idea to start using them. If the OS
doesn't need something, then why pass it in the first place? What
purpose does it serve? It's an invitation for someone somewhere to
start using them.

The downside of keeping the nodes is it creates an ABI where there
doesn't need to be one necessarily.

> The real problem is that some folks developed a certification program
> that allegedly requires schema verification and now propose adding
> code to U-Boot that doesn't really solve any problem.  My proposed
> solution would be to change said certification program to allow
> firmware to augment the device tree with properties and nodes with
> compatibles that are in the namespace controlled by the firmware.

I don't think we should decide what to do here based on said
certification program. It can adapt to what's decided. Probably, the
/option nodes will be stripped out or ignored for certification.

I accepted u-boot options node schema into dtschema, but now have
second thoughts on that. Now I'm getting more u-boot specific
(perhaps, not clear) stuff and widevine stuff internal to a TEE.

Rob

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Rob,

On Wed, 13 Sept 2023 at 16:39, Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Sep 8, 2023 at 1:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > From: Jassi Brar <jassisinghbrar@gmail.com>
> > > Date: Fri, 8 Sep 2023 09:37:12 -0500
> > >
> > > Hi Simon,
> > >
> > > On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > > >
> > > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > > been. See, for example the /chosen and /options nodes.
> > > > > >
> > > > > > There are exceptions...
> > > > > >
> > > > >
> > > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > > stuff of various projects in the schema just defeats the definition of a
> > > > > spec.
> > > >
> > > > Our efforts should not just be about internal ABI, but working to
> > > > provide a consistent configuration system for all firmware elements.
> > > >
> > > Sure, programmatically we can pass any data/info via DT, however it is
> > > only meant to map hardware features onto data structures.
> > >
> > > devicetree.org  landing page
> > >     "The devicetree is a data structure for describing hardware."
> > >
> > > devicetree-specification-v0.3.pdf  Chapter-2 Line-1
> > >    "DTSpec specifies a construct called a devicetree to describe
> > > system hardware."
> >
> > But it doesn't say that it describes *only* hardware.  And it does
> > describe more than just hardware.  There is /chosen to specify
> > firmware configuration and there are several examples of device tree
> > bindings that describe non-discoverable firmware interfaces in the
> > Linux tree.  And it has been that way since the days of IEEE 1275.
> > There are also bindings describing things like the firmware partition
> > layout.
>
> Yes. The exact title for 1275[1] is: IEEE Standard for Boot
> (Initialization Configuration)
> Firmware: Core Requirements and Practices
>
> I see "configuration" in there. However, in the OF case, it's
> generally how firmware configured the hardware and what it discovered.
> That's a little different than telling the OS how to configure the
> hardware which is what we do with FDT.

For the /options node it says "The properties of this node are the
system’s configuration variables."

Then there is section 7.4.4 which has a large list of options which
don't seem to be so narrowly defined.

In any case, this is not quite the point, which IMO is that we need DT
to support firmware use cases, whether or not a 29-year-old spec
thought of it or not. In fact it is amazing to me how forward-looking
Open Firmware was.

> Then there's stuff that's how
> to configure Linux which we try to reject.

Fair enough: Linux has user-space for that.

>
> Once we get into configuration of the OS/client (including u-boot),
> making that an ABI is a lot harder and if we use DT for that, I don't
> think it should be an ABI.
>
> > > If we want to digress from the spec, we need the majority of
> > > developers to switch sides :)  which is unlikely to happen and rightly
> > > so, imho.
> >
> > It isn't even clear what the spec is.  There is the document you
> > reference above, there are the yaml files at
> > https://github.com/devicetree-org/dt-schema and then there are
> > additional yaml files in the Linux tree.  As far as I know it isn't
> > written down anywhere that those are the only places where device tree
> > bindings can live.
>
> There's not any restriction.
>
> My intention with dtschema schemas is to only have "spec level"
> schemas. (Stuff we'd add to DTSpec (but don't because no one wants to
> write specs).) Hardware specific stuff lives somewhere else. That
> happens to be the Linux tree because that is where all the h/w support
> is (nothing else is close (by far)). Last I checked, we've got ~3700
> schemas and ~1500 unconverted bindings.

That scope is quite a bit narrower than I understood it to be. It
seems to leave a significant gap between the Linux repo and yours.

>
> > Anyway, let's face it, there is some consensus among developers that
> > what Simon has done in U-Boot is pushing the use of devicetree beyond
> > the point where a significant fraction of developers thinks it makes
> > sense.  And I think I agree with that.  But you can't beat him with
> > the spec to make your point.
> >
> > Now the devicetree is cleverly constructed such that it is possible to
> > define additional bindings without the risk of conflicting with
> > bindings developed by other parties.  In particular if U-Boot is
> > augmenting a device tree with properties that are prefixed with
> > "u-boot," this isn't going to hurt an operating system that uses such
> > an augmented device tree.
>
> Until someone has some great idea to start using them. If the OS
> doesn't need something, then why pass it in the first place? What
> purpose does it serve? It's an invitation for someone somewhere to
> start using them.
>
> The downside of keeping the nodes is it creates an ABI where there
> doesn't need to be one necessarily.

I'd love to get away from the idea that DT is just for the OS. We are
trying to use it for firmware-to-firmware communication, too. The
programs on each side of that interface may be different projects. For
that, we do need to have a binding, otherwise we end up with series
like this one.

>
> > The real problem is that some folks developed a certification program
> > that allegedly requires schema verification and now propose adding
> > code to U-Boot that doesn't really solve any problem.  My proposed
> > solution would be to change said certification program to allow
> > firmware to augment the device tree with properties and nodes with
> > compatibles that are in the namespace controlled by the firmware.
>
> I don't think we should decide what to do here based on said
> certification program. It can adapt to what's decided. Probably, the
> /option nodes will be stripped out or ignored for certification.
>
> I accepted u-boot options node schema into dtschema, but now have
> second thoughts on that. Now I'm getting more u-boot specific
> (perhaps, not clear) stuff and widevine stuff internal to a TEE.

Where should these bindings go such that ARM / Linaro are not trying
to remove them? I would be OK with moving them out somewhere else, but
how are people supposed to deal with such fragmentation? My
understanding was that dt-schema was an attempt to set up a neutral
area where bindings could be accepted that were not just for
Linux...did that change?

U-Boot is certainly the firmware project that is trying hardest to
clean up things in this area...but it isn't the only consumer of these
bindings. Given that we invent a new bootloader about every 6 months,
I'm pretty sure the number of consumers will grow.

Regards,
Simon

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Thu, Sep 14, 2023 at 04:41:43PM -0600, Simon Glass wrote:
> Hi Rob,
> 
> On Wed, 13 Sept 2023 at 16:39, Rob Herring <robh@kernel.org> wrote:
[snip]
> > I don't think we should decide what to do here based on said
> > certification program. It can adapt to what's decided. Probably, the
> > /option nodes will be stripped out or ignored for certification.
> >
> > I accepted u-boot options node schema into dtschema, but now have
> > second thoughts on that. Now I'm getting more u-boot specific
> > (perhaps, not clear) stuff and widevine stuff internal to a TEE.
> 
> Where should these bindings go such that ARM / Linaro are not trying
> to remove them? I would be OK with moving them out somewhere else, but
> how are people supposed to deal with such fragmentation? My
> understanding was that dt-schema was an attempt to set up a neutral
> area where bindings could be accepted that were not just for
> Linux...did that change?

Well, part of the problem here is that I've been talking with Ilias more
about what's intended here and the fwu-* stuff that Rob rejected is
indeed not right.  We should drop it and replace it with something that
really addresses the underlying problem (which is how do you know
how/where to find some GUIDs) and I think we think it's something that
can be shared between projects too and so be easier to convince Rob that
the next form of it is right (or the right direction).

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Ilias Apalodimas]

+CC Jose who's maintaining the metadata spec from Arm side.

On Fri, 15 Sept 2023 at 02:38, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Sep 14, 2023 at 04:41:43PM -0600, Simon Glass wrote:
> > Hi Rob,
> >
> > On Wed, 13 Sept 2023 at 16:39, Rob Herring <robh@kernel.org> wrote:
> [snip]
> > > I don't think we should decide what to do here based on said
> > > certification program. It can adapt to what's decided. Probably, the
> > > /option nodes will be stripped out or ignored for certification.
> > >
> > > I accepted u-boot options node schema into dtschema, but now have
> > > second thoughts on that. Now I'm getting more u-boot specific
> > > (perhaps, not clear) stuff and widevine stuff internal to a TEE.
> >
> > Where should these bindings go such that ARM / Linaro are not trying
> > to remove them? I would be OK with moving them out somewhere else, but
> > how are people supposed to deal with such fragmentation? My
> > understanding was that dt-schema was an attempt to set up a neutral
> > area where bindings could be accepted that were not just for
> > Linux...did that change?
>
> Well, part of the problem here is that I've been talking with Ilias more
> about what's intended here and the fwu-* stuff that Rob rejected is
> indeed not right.  We should drop it and replace it with something that
> really addresses the underlying problem (which is how do you know
> how/where to find some GUIDs) and I think we think it's something that
> can be shared between projects too and so be easier to convince Rob that
> the next form of it is right (or the right direction).

Tom, I gave the discussion we had last night a bit of thought (thanks
for that) and talked with Jose.
The spec as is written right now recommends a GPT-based partition.  It
then has various GPT UUIDs that indicate where to find the metadata as
well as the various firmware banks that participate in the whole
scheme.  If you have that GPT, then the only thing you really need is
to find the metadata UUID and handle the various banks.

However,  the synquacer which boots from a NOR has no GPT.  What that
fwu node entry does, in order to abstract the rest of the
implementation, is map UUIIDs <-> offset + length.  That way the core
code still thinks it's trying to discover UUIDs but the eventual
reads/writes end up on a NOR.
That box boots via SCP [0] , which has no understanding of device
trees, instead they hardcode the UUIDS, offsets etc for the Synquacer.
But if they ever wanted to switch over having that information would
come in handy.

Jose and I discussed this a bit.  I'll backpaddle and have someone
look into upstreaming the fwu part.  There are a few changes required,
since we already have some MTD information in the DT and those entries
would make more sense there instead of inventing a new node, but we
can discuss this when the patches are sent. Once those are sent, we
can add a recommendation on the spec, pointing to that DT entry for
any future early stage boot loaders that want to implement it,
pointing to the DT entry.

That only solves a fraction of the problem though.  Other nodes, like
the EFI public key etc. still need to be removed.

[0] https://github.com/ARM-software/SCP-firmware

Thanks
/Ilias
>
> --
> Tom

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Rob Herring]

On Thu, Sep 14, 2023 at 5:42 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rob,
>
> On Wed, 13 Sept 2023 at 16:39, Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Sep 8, 2023 at 1:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > >
> > > > From: Jassi Brar <jassisinghbrar@gmail.com>
> > > > Date: Fri, 8 Sep 2023 09:37:12 -0500
> > > >
> > > > Hi Simon,
> > > >
> > > > On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > > > >
> > > > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > > > been. See, for example the /chosen and /options nodes.
> > > > > > >
> > > > > > > There are exceptions...
> > > > > > >
> > > > > >
> > > > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > > > stuff of various projects in the schema just defeats the definition of a
> > > > > > spec.
> > > > >
> > > > > Our efforts should not just be about internal ABI, but working to
> > > > > provide a consistent configuration system for all firmware elements.
> > > > >
> > > > Sure, programmatically we can pass any data/info via DT, however it is
> > > > only meant to map hardware features onto data structures.
> > > >
> > > > devicetree.org  landing page
> > > >     "The devicetree is a data structure for describing hardware."
> > > >
> > > > devicetree-specification-v0.3.pdf  Chapter-2 Line-1
> > > >    "DTSpec specifies a construct called a devicetree to describe
> > > > system hardware."
> > >
> > > But it doesn't say that it describes *only* hardware.  And it does
> > > describe more than just hardware.  There is /chosen to specify
> > > firmware configuration and there are several examples of device tree
> > > bindings that describe non-discoverable firmware interfaces in the
> > > Linux tree.  And it has been that way since the days of IEEE 1275.
> > > There are also bindings describing things like the firmware partition
> > > layout.
> >
> > Yes. The exact title for 1275[1] is: IEEE Standard for Boot
> > (Initialization Configuration)
> > Firmware: Core Requirements and Practices
> >
> > I see "configuration" in there. However, in the OF case, it's
> > generally how firmware configured the hardware and what it discovered.
> > That's a little different than telling the OS how to configure the
> > hardware which is what we do with FDT.
>
> For the /options node it says "The properties of this node are the
> system’s configuration variables."
>
> Then there is section 7.4.4 which has a large list of options which
> don't seem to be so narrowly defined.
>
> In any case, this is not quite the point, which IMO is that we need DT
> to support firmware use cases, whether or not a 29-year-old spec
> thought of it or not. In fact it is amazing to me how forward-looking
> Open Firmware was.
>
> > Then there's stuff that's how
> > to configure Linux which we try to reject.
>
> Fair enough: Linux has user-space for that.

Yep, the question I usually ask is who needs to configure something
and what determines the config. Changing things with sysfs is much
easier than changing the DT provided by firmware.

> >
> > Once we get into configuration of the OS/client (including u-boot),
> > making that an ABI is a lot harder and if we use DT for that, I don't
> > think it should be an ABI.
> >
> > > > If we want to digress from the spec, we need the majority of
> > > > developers to switch sides :)  which is unlikely to happen and rightly
> > > > so, imho.
> > >
> > > It isn't even clear what the spec is.  There is the document you
> > > reference above, there are the yaml files at
> > > https://github.com/devicetree-org/dt-schema and then there are
> > > additional yaml files in the Linux tree.  As far as I know it isn't
> > > written down anywhere that those are the only places where device tree
> > > bindings can live.
> >
> > There's not any restriction.
> >
> > My intention with dtschema schemas is to only have "spec level"
> > schemas. (Stuff we'd add to DTSpec (but don't because no one wants to
> > write specs).) Hardware specific stuff lives somewhere else. That
> > happens to be the Linux tree because that is where all the h/w support
> > is (nothing else is close (by far)). Last I checked, we've got ~3700
> > schemas and ~1500 unconverted bindings.
>
> That scope is quite a bit narrower than I understood it to be. It
> seems to leave a significant gap between the Linux repo and yours.
>
> >
> > > Anyway, let's face it, there is some consensus among developers that
> > > what Simon has done in U-Boot is pushing the use of devicetree beyond
> > > the point where a significant fraction of developers thinks it makes
> > > sense.  And I think I agree with that.  But you can't beat him with
> > > the spec to make your point.
> > >
> > > Now the devicetree is cleverly constructed such that it is possible to
> > > define additional bindings without the risk of conflicting with
> > > bindings developed by other parties.  In particular if U-Boot is
> > > augmenting a device tree with properties that are prefixed with
> > > "u-boot," this isn't going to hurt an operating system that uses such
> > > an augmented device tree.
> >
> > Until someone has some great idea to start using them. If the OS
> > doesn't need something, then why pass it in the first place? What
> > purpose does it serve? It's an invitation for someone somewhere to
> > start using them.
> >
> > The downside of keeping the nodes is it creates an ABI where there
> > doesn't need to be one necessarily.
>
> I'd love to get away from the idea that DT is just for the OS. We are
> trying to use it for firmware-to-firmware communication, too. The
> programs on each side of that interface may be different projects. For
> that, we do need to have a binding, otherwise we end up with series
> like this one.

I don't think DT is just for the OS, but DT for the OS is an ABI and
other cases may not be. Or they just don't need to follow all the same
rules. Zephyr is doing their own thing for example.

I don't think /options/u-boot is an ABI. AIUI, u-boot populates the
node and consumes it. No ABI there (or limited to SPL to u-boot
proper). I suppose a prior firmware stage could populate it, but that
doesn't scale as then the prior stage has to know details of the next
stage.

> > > The real problem is that some folks developed a certification program
> > > that allegedly requires schema verification and now propose adding
> > > code to U-Boot that doesn't really solve any problem.  My proposed
> > > solution would be to change said certification program to allow
> > > firmware to augment the device tree with properties and nodes with
> > > compatibles that are in the namespace controlled by the firmware.
> >
> > I don't think we should decide what to do here based on said
> > certification program. It can adapt to what's decided. Probably, the
> > /option nodes will be stripped out or ignored for certification.
> >
> > I accepted u-boot options node schema into dtschema, but now have
> > second thoughts on that. Now I'm getting more u-boot specific
> > (perhaps, not clear) stuff and widevine stuff internal to a TEE.
>
> Where should these bindings go such that ARM / Linaro are not trying
> to remove them? I would be OK with moving them out somewhere else, but
> how are people supposed to deal with such fragmentation? My
> understanding was that dt-schema was an attempt to set up a neutral
> area where bindings could be accepted that were not just for
> Linux...did that change?

No change, just not communicated I guess. And again, bindings are not
"just for Linux". They are hosted there because that is where *all*
the hardware support is (by far). But we'll probably never get past
the "Linux binding" perception no matter what we do or how many times
I say it.

To rephrase things a bit, I'm happy to host anything that's
multi-project, not a large number of bindings, and not a
device/hardware specific binding. The device specific bindings live in
the kernel tree primarily. For any new binding (foos/#foo-cell type
ones), I want to see multiple users. Really for these, probably best
to start with them in Linux repo (or elsewhere) and then promote them
to dtschema. That gives a little flexibility in changing/abandoning
them.

Removing nodes and/or properties and where things live are mostly
independent discussions. SystemReady can adapt to whatever is decided
for the former. In general, IMO, when passing DT from stage N to N+1,
the N+1 stage should remove things which only apply to N+2 stage. For
example, kexec removes /chosen and recreates it for the next kernel.

Rob

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Rob,

On Mon, 18 Sept 2023 at 11:00, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Sep 14, 2023 at 5:42 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > On Wed, 13 Sept 2023 at 16:39, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri, Sep 8, 2023 at 1:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > >
> > > > > From: Jassi Brar <jassisinghbrar@gmail.com>
> > > > > Date: Fri, 8 Sep 2023 09:37:12 -0500
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > > > > >
> > > > > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > > > > been. See, for example the /chosen and /options nodes.
> > > > > > > >
> > > > > > > > There are exceptions...
> > > > > > > >
> > > > > > >
> > > > > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > > > > stuff of various projects in the schema just defeats the definition of a
> > > > > > > spec.
> > > > > >
> > > > > > Our efforts should not just be about internal ABI, but working to
> > > > > > provide a consistent configuration system for all firmware elements.
> > > > > >
> > > > > Sure, programmatically we can pass any data/info via DT, however it is
> > > > > only meant to map hardware features onto data structures.
> > > > >
> > > > > devicetree.org  landing page
> > > > >     "The devicetree is a data structure for describing hardware."
> > > > >
> > > > > devicetree-specification-v0.3.pdf  Chapter-2 Line-1
> > > > >    "DTSpec specifies a construct called a devicetree to describe
> > > > > system hardware."
> > > >
> > > > But it doesn't say that it describes *only* hardware.  And it does
> > > > describe more than just hardware.  There is /chosen to specify
> > > > firmware configuration and there are several examples of device tree
> > > > bindings that describe non-discoverable firmware interfaces in the
> > > > Linux tree.  And it has been that way since the days of IEEE 1275.
> > > > There are also bindings describing things like the firmware partition
> > > > layout.
> > >
> > > Yes. The exact title for 1275[1] is: IEEE Standard for Boot
> > > (Initialization Configuration)
> > > Firmware: Core Requirements and Practices
> > >
> > > I see "configuration" in there. However, in the OF case, it's
> > > generally how firmware configured the hardware and what it discovered.
> > > That's a little different than telling the OS how to configure the
> > > hardware which is what we do with FDT.
> >
> > For the /options node it says "The properties of this node are the
> > system’s configuration variables."
> >
> > Then there is section 7.4.4 which has a large list of options which
> > don't seem to be so narrowly defined.
> >
> > In any case, this is not quite the point, which IMO is that we need DT
> > to support firmware use cases, whether or not a 29-year-old spec
> > thought of it or not. In fact it is amazing to me how forward-looking
> > Open Firmware was.
> >
> > > Then there's stuff that's how
> > > to configure Linux which we try to reject.
> >
> > Fair enough: Linux has user-space for that.
>
> Yep, the question I usually ask is who needs to configure something
> and what determines the config. Changing things with sysfs is much
> easier than changing the DT provided by firmware.
>
> > >
> > > Once we get into configuration of the OS/client (including u-boot),
> > > making that an ABI is a lot harder and if we use DT for that, I don't
> > > think it should be an ABI.
> > >
> > > > > If we want to digress from the spec, we need the majority of
> > > > > developers to switch sides :)  which is unlikely to happen and rightly
> > > > > so, imho.
> > > >
> > > > It isn't even clear what the spec is.  There is the document you
> > > > reference above, there are the yaml files at
> > > > https://github.com/devicetree-org/dt-schema and then there are
> > > > additional yaml files in the Linux tree.  As far as I know it isn't
> > > > written down anywhere that those are the only places where device tree
> > > > bindings can live.
> > >
> > > There's not any restriction.
> > >
> > > My intention with dtschema schemas is to only have "spec level"
> > > schemas. (Stuff we'd add to DTSpec (but don't because no one wants to
> > > write specs).) Hardware specific stuff lives somewhere else. That
> > > happens to be the Linux tree because that is where all the h/w support
> > > is (nothing else is close (by far)). Last I checked, we've got ~3700
> > > schemas and ~1500 unconverted bindings.
> >
> > That scope is quite a bit narrower than I understood it to be. It
> > seems to leave a significant gap between the Linux repo and yours.
> >
> > >
> > > > Anyway, let's face it, there is some consensus among developers that
> > > > what Simon has done in U-Boot is pushing the use of devicetree beyond
> > > > the point where a significant fraction of developers thinks it makes
> > > > sense.  And I think I agree with that.  But you can't beat him with
> > > > the spec to make your point.
> > > >
> > > > Now the devicetree is cleverly constructed such that it is possible to
> > > > define additional bindings without the risk of conflicting with
> > > > bindings developed by other parties.  In particular if U-Boot is
> > > > augmenting a device tree with properties that are prefixed with
> > > > "u-boot," this isn't going to hurt an operating system that uses such
> > > > an augmented device tree.
> > >
> > > Until someone has some great idea to start using them. If the OS
> > > doesn't need something, then why pass it in the first place? What
> > > purpose does it serve? It's an invitation for someone somewhere to
> > > start using them.
> > >
> > > The downside of keeping the nodes is it creates an ABI where there
> > > doesn't need to be one necessarily.
> >
> > I'd love to get away from the idea that DT is just for the OS. We are
> > trying to use it for firmware-to-firmware communication, too. The
> > programs on each side of that interface may be different projects. For
> > that, we do need to have a binding, otherwise we end up with series
> > like this one.
>
> I don't think DT is just for the OS, but DT for the OS is an ABI and
> other cases may not be. Or they just don't need to follow all the same
> rules. Zephyr is doing their own thing for example.
>
> I don't think /options/u-boot is an ABI. AIUI, u-boot populates the
> node and consumes it. No ABI there (or limited to SPL to u-boot
> proper). I suppose a prior firmware stage could populate it, but that
> doesn't scale as then the prior stage has to know details of the next
> stage.

I still don't understand the distinction, or really why U-Boot (or
u-boot) is different. The main reason for the /options/u-boot node is
so that prior phase firmware can tell U-Boot what to do. It is similar
to the /chosen node for the OS. If it is not an ABI, how can any
project rely on it? It is absolutely not just U-Boot generating
something for its own consumption. Where is that idea even coming
from? We even have an OF_HAS_PRIOR_STAGE Kconfig option for it in
U-Boot, specifically for TF-A, etc.

As to the scaling, I agree. That suggests we try to make things
generic, i.e. have an /options node with these sorts of generic
settings. Candidates for that are console and logging controls, for
example. I would prefer that to /options/u-boot, as you know.

>
> > > > The real problem is that some folks developed a certification program
> > > > that allegedly requires schema verification and now propose adding
> > > > code to U-Boot that doesn't really solve any problem.  My proposed
> > > > solution would be to change said certification program to allow
> > > > firmware to augment the device tree with properties and nodes with
> > > > compatibles that are in the namespace controlled by the firmware.
> > >
> > > I don't think we should decide what to do here based on said
> > > certification program. It can adapt to what's decided. Probably, the
> > > /option nodes will be stripped out or ignored for certification.
> > >
> > > I accepted u-boot options node schema into dtschema, but now have
> > > second thoughts on that. Now I'm getting more u-boot specific
> > > (perhaps, not clear) stuff and widevine stuff internal to a TEE.
> >
> > Where should these bindings go such that ARM / Linaro are not trying
> > to remove them? I would be OK with moving them out somewhere else, but
> > how are people supposed to deal with such fragmentation? My
> > understanding was that dt-schema was an attempt to set up a neutral
> > area where bindings could be accepted that were not just for
> > Linux...did that change?
>
> No change, just not communicated I guess. And again, bindings are not
> "just for Linux". They are hosted there because that is where *all*
> the hardware support is (by far). But we'll probably never get past
> the "Linux binding" perception no matter what we do or how many times
> I say it.
>
> To rephrase things a bit, I'm happy to host anything that's
> multi-project, not a large number of bindings, and not a
> device/hardware specific binding. The device specific bindings live in
> the kernel tree primarily. For any new binding (foos/#foo-cell type
> ones), I want to see multiple users. Really for these, probably best
> to start with them in Linux repo (or elsewhere) and then promote them
> to dtschema. That gives a little flexibility in changing/abandoning
> them.

So now I am not sure what you are suggesting. Are you wanting bindings
in many places (yours, Linux, U-Boot, TF-A, ...)? I am sure you are
aware that if we put bindings in U-Boot they will not be considered as
bindings at all, including by Linaro.

To restate the problem, we need (and until your previous email I
thought we had) a unified repo where cross-project, firmware-targeted
bindings can be accepted and agreed by interested projects.

>
> Removing nodes and/or properties and where things live are mostly
> independent discussions. SystemReady can adapt to whatever is decided
> for the former. In general, IMO, when passing DT from stage N to N+1,
> the N+1 stage should remove things which only apply to N+2 stage. For
> example, kexec removes /chosen and recreates it for the next kernel.

Sounds good. But note that the reason for that is a conflict, since
the node is used for different things - i.e. the two kernels need
different settings. That is quite different from the case I am talking
about, where the settings either apply globally (to interested
projects) or to a single project (for all boot phases of that
project). In my case, I believe there is no need to remove anything.

Regards,
Simon

PS Zephyr is absolutely doing its own thing. Apart from the fact that
it doesn't even have a runtime DT, the bindings are entirely within
the project. and bear little relation to Linux bindings. I was not
around for that decision, but I suspect part of the rationale was that
many of the MCUs which Zephyr targets are not supported by Linux. Of
course, that is not fully true and I believe it will become less true
with time. Then, perhaps, in 5 years it will be Zephyr's turn to think
about bindings more deeply.

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Tom Rini]

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

On Tue, Sep 19, 2023 at 02:25:49PM -0600, Simon Glass wrote:
> Hi Rob,
> 
> On Mon, 18 Sept 2023 at 11:00, Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Sep 14, 2023 at 5:42 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Rob,
> > >
> > > On Wed, 13 Sept 2023 at 16:39, Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Fri, Sep 8, 2023 at 1:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > >
> > > > > > From: Jassi Brar <jassisinghbrar@gmail.com>
> > > > > > Date: Fri, 8 Sep 2023 09:37:12 -0500
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > > > > > >
> > > > > > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > > > > > been. See, for example the /chosen and /options nodes.
> > > > > > > > >
> > > > > > > > > There are exceptions...
> > > > > > > > >
> > > > > > > >
> > > > > > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > > > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > > > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > > > > > stuff of various projects in the schema just defeats the definition of a
> > > > > > > > spec.
> > > > > > >
> > > > > > > Our efforts should not just be about internal ABI, but working to
> > > > > > > provide a consistent configuration system for all firmware elements.
> > > > > > >
> > > > > > Sure, programmatically we can pass any data/info via DT, however it is
> > > > > > only meant to map hardware features onto data structures.
> > > > > >
> > > > > > devicetree.org  landing page
> > > > > >     "The devicetree is a data structure for describing hardware."
> > > > > >
> > > > > > devicetree-specification-v0.3.pdf  Chapter-2 Line-1
> > > > > >    "DTSpec specifies a construct called a devicetree to describe
> > > > > > system hardware."
> > > > >
> > > > > But it doesn't say that it describes *only* hardware.  And it does
> > > > > describe more than just hardware.  There is /chosen to specify
> > > > > firmware configuration and there are several examples of device tree
> > > > > bindings that describe non-discoverable firmware interfaces in the
> > > > > Linux tree.  And it has been that way since the days of IEEE 1275.
> > > > > There are also bindings describing things like the firmware partition
> > > > > layout.
> > > >
> > > > Yes. The exact title for 1275[1] is: IEEE Standard for Boot
> > > > (Initialization Configuration)
> > > > Firmware: Core Requirements and Practices
> > > >
> > > > I see "configuration" in there. However, in the OF case, it's
> > > > generally how firmware configured the hardware and what it discovered.
> > > > That's a little different than telling the OS how to configure the
> > > > hardware which is what we do with FDT.
> > >
> > > For the /options node it says "The properties of this node are the
> > > system’s configuration variables."
> > >
> > > Then there is section 7.4.4 which has a large list of options which
> > > don't seem to be so narrowly defined.
> > >
> > > In any case, this is not quite the point, which IMO is that we need DT
> > > to support firmware use cases, whether or not a 29-year-old spec
> > > thought of it or not. In fact it is amazing to me how forward-looking
> > > Open Firmware was.
> > >
> > > > Then there's stuff that's how
> > > > to configure Linux which we try to reject.
> > >
> > > Fair enough: Linux has user-space for that.
> >
> > Yep, the question I usually ask is who needs to configure something
> > and what determines the config. Changing things with sysfs is much
> > easier than changing the DT provided by firmware.
> >
> > > >
> > > > Once we get into configuration of the OS/client (including u-boot),
> > > > making that an ABI is a lot harder and if we use DT for that, I don't
> > > > think it should be an ABI.
> > > >
> > > > > > If we want to digress from the spec, we need the majority of
> > > > > > developers to switch sides :)  which is unlikely to happen and rightly
> > > > > > so, imho.
> > > > >
> > > > > It isn't even clear what the spec is.  There is the document you
> > > > > reference above, there are the yaml files at
> > > > > https://github.com/devicetree-org/dt-schema and then there are
> > > > > additional yaml files in the Linux tree.  As far as I know it isn't
> > > > > written down anywhere that those are the only places where device tree
> > > > > bindings can live.
> > > >
> > > > There's not any restriction.
> > > >
> > > > My intention with dtschema schemas is to only have "spec level"
> > > > schemas. (Stuff we'd add to DTSpec (but don't because no one wants to
> > > > write specs).) Hardware specific stuff lives somewhere else. That
> > > > happens to be the Linux tree because that is where all the h/w support
> > > > is (nothing else is close (by far)). Last I checked, we've got ~3700
> > > > schemas and ~1500 unconverted bindings.
> > >
> > > That scope is quite a bit narrower than I understood it to be. It
> > > seems to leave a significant gap between the Linux repo and yours.
> > >
> > > >
> > > > > Anyway, let's face it, there is some consensus among developers that
> > > > > what Simon has done in U-Boot is pushing the use of devicetree beyond
> > > > > the point where a significant fraction of developers thinks it makes
> > > > > sense.  And I think I agree with that.  But you can't beat him with
> > > > > the spec to make your point.
> > > > >
> > > > > Now the devicetree is cleverly constructed such that it is possible to
> > > > > define additional bindings without the risk of conflicting with
> > > > > bindings developed by other parties.  In particular if U-Boot is
> > > > > augmenting a device tree with properties that are prefixed with
> > > > > "u-boot," this isn't going to hurt an operating system that uses such
> > > > > an augmented device tree.
> > > >
> > > > Until someone has some great idea to start using them. If the OS
> > > > doesn't need something, then why pass it in the first place? What
> > > > purpose does it serve? It's an invitation for someone somewhere to
> > > > start using them.
> > > >
> > > > The downside of keeping the nodes is it creates an ABI where there
> > > > doesn't need to be one necessarily.
> > >
> > > I'd love to get away from the idea that DT is just for the OS. We are
> > > trying to use it for firmware-to-firmware communication, too. The
> > > programs on each side of that interface may be different projects. For
> > > that, we do need to have a binding, otherwise we end up with series
> > > like this one.
> >
> > I don't think DT is just for the OS, but DT for the OS is an ABI and
> > other cases may not be. Or they just don't need to follow all the same
> > rules. Zephyr is doing their own thing for example.
> >
> > I don't think /options/u-boot is an ABI. AIUI, u-boot populates the
> > node and consumes it. No ABI there (or limited to SPL to u-boot
> > proper). I suppose a prior firmware stage could populate it, but that
> > doesn't scale as then the prior stage has to know details of the next
> > stage.
> 
> I still don't understand the distinction, or really why U-Boot (or
> u-boot) is different. The main reason for the /options/u-boot node is
> so that prior phase firmware can tell U-Boot what to do. It is similar
> to the /chosen node for the OS. If it is not an ABI, how can any
> project rely on it? It is absolutely not just U-Boot generating
> something for its own consumption. Where is that idea even coming
> from? We even have an OF_HAS_PRIOR_STAGE Kconfig option for it in
> U-Boot, specifically for TF-A, etc.

Well, maybe we aren't different, we just need to figure out for
ourselves what is useful to have in DT, and what isn't because no one
else will use it and it's not the best mechanism for our use either.  Or
there might be cases where it is useful to us to do it that way and we
just delete that part of the tree prior to passing on to the OS.

-- 
Tom

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

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Rob Herring]

On Tue, Sep 19, 2023 at 3:26 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rob,
>
> On Mon, 18 Sept 2023 at 11:00, Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Sep 14, 2023 at 5:42 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Rob,
> > >
> > > On Wed, 13 Sept 2023 at 16:39, Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Fri, Sep 8, 2023 at 1:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > >
> > > > > > From: Jassi Brar <jassisinghbrar@gmail.com>
> > > > > > Date: Fri, 8 Sep 2023 09:37:12 -0500
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > > > > > >
> > > > > > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > > > > > been. See, for example the /chosen and /options nodes.
> > > > > > > > >
> > > > > > > > > There are exceptions...
> > > > > > > > >
> > > > > > > >
> > > > > > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > > > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > > > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > > > > > stuff of various projects in the schema just defeats the definition of a
> > > > > > > > spec.
> > > > > > >
> > > > > > > Our efforts should not just be about internal ABI, but working to
> > > > > > > provide a consistent configuration system for all firmware elements.
> > > > > > >
> > > > > > Sure, programmatically we can pass any data/info via DT, however it is
> > > > > > only meant to map hardware features onto data structures.
> > > > > >
> > > > > > devicetree.org  landing page
> > > > > >     "The devicetree is a data structure for describing hardware."
> > > > > >
> > > > > > devicetree-specification-v0.3.pdf  Chapter-2 Line-1
> > > > > >    "DTSpec specifies a construct called a devicetree to describe
> > > > > > system hardware."
> > > > >
> > > > > But it doesn't say that it describes *only* hardware.  And it does
> > > > > describe more than just hardware.  There is /chosen to specify
> > > > > firmware configuration and there are several examples of device tree
> > > > > bindings that describe non-discoverable firmware interfaces in the
> > > > > Linux tree.  And it has been that way since the days of IEEE 1275.
> > > > > There are also bindings describing things like the firmware partition
> > > > > layout.
> > > >
> > > > Yes. The exact title for 1275[1] is: IEEE Standard for Boot
> > > > (Initialization Configuration)
> > > > Firmware: Core Requirements and Practices
> > > >
> > > > I see "configuration" in there. However, in the OF case, it's
> > > > generally how firmware configured the hardware and what it discovered.
> > > > That's a little different than telling the OS how to configure the
> > > > hardware which is what we do with FDT.
> > >
> > > For the /options node it says "The properties of this node are the
> > > system’s configuration variables."
> > >
> > > Then there is section 7.4.4 which has a large list of options which
> > > don't seem to be so narrowly defined.
> > >
> > > In any case, this is not quite the point, which IMO is that we need DT
> > > to support firmware use cases, whether or not a 29-year-old spec
> > > thought of it or not. In fact it is amazing to me how forward-looking
> > > Open Firmware was.
> > >
> > > > Then there's stuff that's how
> > > > to configure Linux which we try to reject.
> > >
> > > Fair enough: Linux has user-space for that.
> >
> > Yep, the question I usually ask is who needs to configure something
> > and what determines the config. Changing things with sysfs is much
> > easier than changing the DT provided by firmware.
> >
> > > >
> > > > Once we get into configuration of the OS/client (including u-boot),
> > > > making that an ABI is a lot harder and if we use DT for that, I don't
> > > > think it should be an ABI.
> > > >
> > > > > > If we want to digress from the spec, we need the majority of
> > > > > > developers to switch sides :)  which is unlikely to happen and rightly
> > > > > > so, imho.
> > > > >
> > > > > It isn't even clear what the spec is.  There is the document you
> > > > > reference above, there are the yaml files at
> > > > > https://github.com/devicetree-org/dt-schema and then there are
> > > > > additional yaml files in the Linux tree.  As far as I know it isn't
> > > > > written down anywhere that those are the only places where device tree
> > > > > bindings can live.
> > > >
> > > > There's not any restriction.
> > > >
> > > > My intention with dtschema schemas is to only have "spec level"
> > > > schemas. (Stuff we'd add to DTSpec (but don't because no one wants to
> > > > write specs).) Hardware specific stuff lives somewhere else. That
> > > > happens to be the Linux tree because that is where all the h/w support
> > > > is (nothing else is close (by far)). Last I checked, we've got ~3700
> > > > schemas and ~1500 unconverted bindings.
> > >
> > > That scope is quite a bit narrower than I understood it to be. It
> > > seems to leave a significant gap between the Linux repo and yours.
> > >
> > > >
> > > > > Anyway, let's face it, there is some consensus among developers that
> > > > > what Simon has done in U-Boot is pushing the use of devicetree beyond
> > > > > the point where a significant fraction of developers thinks it makes
> > > > > sense.  And I think I agree with that.  But you can't beat him with
> > > > > the spec to make your point.
> > > > >
> > > > > Now the devicetree is cleverly constructed such that it is possible to
> > > > > define additional bindings without the risk of conflicting with
> > > > > bindings developed by other parties.  In particular if U-Boot is
> > > > > augmenting a device tree with properties that are prefixed with
> > > > > "u-boot," this isn't going to hurt an operating system that uses such
> > > > > an augmented device tree.
> > > >
> > > > Until someone has some great idea to start using them. If the OS
> > > > doesn't need something, then why pass it in the first place? What
> > > > purpose does it serve? It's an invitation for someone somewhere to
> > > > start using them.
> > > >
> > > > The downside of keeping the nodes is it creates an ABI where there
> > > > doesn't need to be one necessarily.
> > >
> > > I'd love to get away from the idea that DT is just for the OS. We are
> > > trying to use it for firmware-to-firmware communication, too. The
> > > programs on each side of that interface may be different projects. For
> > > that, we do need to have a binding, otherwise we end up with series
> > > like this one.
> >
> > I don't think DT is just for the OS, but DT for the OS is an ABI and
> > other cases may not be. Or they just don't need to follow all the same
> > rules. Zephyr is doing their own thing for example.
> >
> > I don't think /options/u-boot is an ABI. AIUI, u-boot populates the
> > node and consumes it. No ABI there (or limited to SPL to u-boot
> > proper). I suppose a prior firmware stage could populate it, but that
> > doesn't scale as then the prior stage has to know details of the next
> > stage.
>
> I still don't understand the distinction, or really why U-Boot (or
> u-boot) is different. The main reason for the /options/u-boot node is
> so that prior phase firmware can tell U-Boot what to do. It is similar
> to the /chosen node for the OS. If it is not an ABI, how can any
> project rely on it? It is absolutely not just U-Boot generating
> something for its own consumption. Where is that idea even coming
> from? We even have an OF_HAS_PRIOR_STAGE Kconfig option for it in
> U-Boot, specifically for TF-A, etc.

Okay, I misunderstood the intent since I thought u-boot gets its DT
from the one in the u-boot tree (at least usually).

Though I just looked at TF-A source and see no evidence of it doing
anything with /options or /options/u-boot. I'm really not that
interested in what is possible and only used by 1 board and fork
somewhere. I'm interested in supporting what the community defines as
best practice (e.g. DT (for OS) comes from firmware), but so far that
hasn't been defined below the firmware-OS level. Yes, there's efforts
addressing aspects of it, but it's not really clear to me what the
high-level goals are and what projects are onboard.

Note that I did find "/secure-chosen" though. And if they want to use
that, then fine, but I don't want to see it.

> As to the scaling, I agree. That suggests we try to make things
> generic, i.e. have an /options node with these sorts of generic
> settings. Candidates for that are console and logging controls, for
> example. I would prefer that to /options/u-boot, as you know.
>
> >
> > > > > The real problem is that some folks developed a certification program
> > > > > that allegedly requires schema verification and now propose adding
> > > > > code to U-Boot that doesn't really solve any problem.  My proposed
> > > > > solution would be to change said certification program to allow
> > > > > firmware to augment the device tree with properties and nodes with
> > > > > compatibles that are in the namespace controlled by the firmware.
> > > >
> > > > I don't think we should decide what to do here based on said
> > > > certification program. It can adapt to what's decided. Probably, the
> > > > /option nodes will be stripped out or ignored for certification.
> > > >
> > > > I accepted u-boot options node schema into dtschema, but now have
> > > > second thoughts on that. Now I'm getting more u-boot specific
> > > > (perhaps, not clear) stuff and widevine stuff internal to a TEE.
> > >
> > > Where should these bindings go such that ARM / Linaro are not trying
> > > to remove them? I would be OK with moving them out somewhere else, but
> > > how are people supposed to deal with such fragmentation? My
> > > understanding was that dt-schema was an attempt to set up a neutral
> > > area where bindings could be accepted that were not just for
> > > Linux...did that change?
> >
> > No change, just not communicated I guess. And again, bindings are not
> > "just for Linux". They are hosted there because that is where *all*
> > the hardware support is (by far). But we'll probably never get past
> > the "Linux binding" perception no matter what we do or how many times
> > I say it.
> >
> > To rephrase things a bit, I'm happy to host anything that's
> > multi-project, not a large number of bindings, and not a
> > device/hardware specific binding. The device specific bindings live in
> > the kernel tree primarily. For any new binding (foos/#foo-cell type
> > ones), I want to see multiple users. Really for these, probably best
> > to start with them in Linux repo (or elsewhere) and then promote them
> > to dtschema. That gives a little flexibility in changing/abandoning
> > them.
>
> So now I am not sure what you are suggesting. Are you wanting bindings
> in many places (yours, Linux, U-Boot, TF-A, ...)? I am sure you are
> aware that if we put bindings in U-Boot they will not be considered as
> bindings at all, including by Linaro.

If the instances of a binding are only produced and consumed in
$project, then it is perfectly fine to host the bindings in $project.
It's also not something fixed. It can start out in $project and get
moved out if it proves useful elsewhere.

> To restate the problem, we need (and until your previous email I
> thought we had) a unified repo where cross-project, firmware-targeted
> bindings can be accepted and agreed by interested projects.

I'm happy to take those (assuming I'm not bombarded with 100s). So
far, nothing I've seen is clearly cross project. Send me stuff that
has acks from multiple projects.

> > Removing nodes and/or properties and where things live are mostly
> > independent discussions. SystemReady can adapt to whatever is decided
> > for the former. In general, IMO, when passing DT from stage N to N+1,
> > the N+1 stage should remove things which only apply to N+2 stage. For
> > example, kexec removes /chosen and recreates it for the next kernel.
>
> Sounds good. But note that the reason for that is a conflict, since
> the node is used for different things - i.e. the two kernels need
> different settings. That is quite different from the case I am talking
> about, where the settings either apply globally (to interested
> projects) or to a single project (for all boot phases of that
> project). In my case, I believe there is no need to remove anything.

There is also no need to keep it. We can always decide to keep it
later if there is a need. We can't decide later to remove it because
someone may be relying on it.

> Regards,
> Simon
>
> PS Zephyr is absolutely doing its own thing. Apart from the fact that
> it doesn't even have a runtime DT, the bindings are entirely within
> the project. and bear little relation to Linux bindings. I was not
> around for that decision, but I suspect part of the rationale was that
> many of the MCUs which Zephyr targets are not supported by Linux. Of
> course, that is not fully true and I believe it will become less true
> with time. Then, perhaps, in 5 years it will be Zephyr's turn to think
> about bindings more deeply.

Little overlap is part of it, but a large part of the reason is Zephyr
needed something machine parsable and it was before we came up with
dtschema. There's desire to adopt dtschema, but at this point I
imagine with the first reason, there is little pressing need. They've
got something that works for them.

Rob

Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties

[Simon Glass]

Hi Rob,

On Thu, 21 Sept 2023 at 07:59, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Sep 19, 2023 at 3:26 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > On Mon, 18 Sept 2023 at 11:00, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Sep 14, 2023 at 5:42 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Rob,
> > > >
> > > > On Wed, 13 Sept 2023 at 16:39, Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Fri, Sep 8, 2023 at 1:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > >
> > > > > > > From: Jassi Brar <jassisinghbrar@gmail.com>
> > > > > > > Date: Fri, 8 Sep 2023 09:37:12 -0500
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Thu, Sep 7, 2023 at 3:08 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas
> > > > > > > > >
> > > > > > > > > > > I beg to differ. Devicetree is more than just hardware and always has
> > > > > > > > > > > been. See, for example the /chosen and /options nodes.
> > > > > > > > > >
> > > > > > > > > > There are exceptions...
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > We've been this over and over again and frankly it gets a bit annoying.
> > > > > > > > > It's called *DEVICE* tree for a reason.  As Rob pointed out there are
> > > > > > > > > exceptions, but those made a lot of sense.  Having arbitrary internal ABI
> > > > > > > > > stuff of various projects in the schema just defeats the definition of a
> > > > > > > > > spec.
> > > > > > > >
> > > > > > > > Our efforts should not just be about internal ABI, but working to
> > > > > > > > provide a consistent configuration system for all firmware elements.
> > > > > > > >
> > > > > > > Sure, programmatically we can pass any data/info via DT, however it is
> > > > > > > only meant to map hardware features onto data structures.
> > > > > > >
> > > > > > > devicetree.org  landing page
> > > > > > >     "The devicetree is a data structure for describing hardware."
> > > > > > >
> > > > > > > devicetree-specification-v0.3.pdf  Chapter-2 Line-1
> > > > > > >    "DTSpec specifies a construct called a devicetree to describe
> > > > > > > system hardware."
> > > > > >
> > > > > > But it doesn't say that it describes *only* hardware.  And it does
> > > > > > describe more than just hardware.  There is /chosen to specify
> > > > > > firmware configuration and there are several examples of device tree
> > > > > > bindings that describe non-discoverable firmware interfaces in the
> > > > > > Linux tree.  And it has been that way since the days of IEEE 1275.
> > > > > > There are also bindings describing things like the firmware partition
> > > > > > layout.
> > > > >
> > > > > Yes. The exact title for 1275[1] is: IEEE Standard for Boot
> > > > > (Initialization Configuration)
> > > > > Firmware: Core Requirements and Practices
> > > > >
> > > > > I see "configuration" in there. However, in the OF case, it's
> > > > > generally how firmware configured the hardware and what it discovered.
> > > > > That's a little different than telling the OS how to configure the
> > > > > hardware which is what we do with FDT.
> > > >
> > > > For the /options node it says "The properties of this node are the
> > > > system’s configuration variables."
> > > >
> > > > Then there is section 7.4.4 which has a large list of options which
> > > > don't seem to be so narrowly defined.
> > > >
> > > > In any case, this is not quite the point, which IMO is that we need DT
> > > > to support firmware use cases, whether or not a 29-year-old spec
> > > > thought of it or not. In fact it is amazing to me how forward-looking
> > > > Open Firmware was.
> > > >
> > > > > Then there's stuff that's how
> > > > > to configure Linux which we try to reject.
> > > >
> > > > Fair enough: Linux has user-space for that.
> > >
> > > Yep, the question I usually ask is who needs to configure something
> > > and what determines the config. Changing things with sysfs is much
> > > easier than changing the DT provided by firmware.
> > >
> > > > >
> > > > > Once we get into configuration of the OS/client (including u-boot),
> > > > > making that an ABI is a lot harder and if we use DT for that, I don't
> > > > > think it should be an ABI.
> > > > >
> > > > > > > If we want to digress from the spec, we need the majority of
> > > > > > > developers to switch sides :)  which is unlikely to happen and rightly
> > > > > > > so, imho.
> > > > > >
> > > > > > It isn't even clear what the spec is.  There is the document you
> > > > > > reference above, there are the yaml files at
> > > > > > https://github.com/devicetree-org/dt-schema and then there are
> > > > > > additional yaml files in the Linux tree.  As far as I know it isn't
> > > > > > written down anywhere that those are the only places where device tree
> > > > > > bindings can live.
> > > > >
> > > > > There's not any restriction.
> > > > >
> > > > > My intention with dtschema schemas is to only have "spec level"
> > > > > schemas. (Stuff we'd add to DTSpec (but don't because no one wants to
> > > > > write specs).) Hardware specific stuff lives somewhere else. That
> > > > > happens to be the Linux tree because that is where all the h/w support
> > > > > is (nothing else is close (by far)). Last I checked, we've got ~3700
> > > > > schemas and ~1500 unconverted bindings.
> > > >
> > > > That scope is quite a bit narrower than I understood it to be. It
> > > > seems to leave a significant gap between the Linux repo and yours.
> > > >
> > > > >
> > > > > > Anyway, let's face it, there is some consensus among developers that
> > > > > > what Simon has done in U-Boot is pushing the use of devicetree beyond
> > > > > > the point where a significant fraction of developers thinks it makes
> > > > > > sense.  And I think I agree with that.  But you can't beat him with
> > > > > > the spec to make your point.
> > > > > >
> > > > > > Now the devicetree is cleverly constructed such that it is possible to
> > > > > > define additional bindings without the risk of conflicting with
> > > > > > bindings developed by other parties.  In particular if U-Boot is
> > > > > > augmenting a device tree with properties that are prefixed with
> > > > > > "u-boot," this isn't going to hurt an operating system that uses such
> > > > > > an augmented device tree.
> > > > >
> > > > > Until someone has some great idea to start using them. If the OS
> > > > > doesn't need something, then why pass it in the first place? What
> > > > > purpose does it serve? It's an invitation for someone somewhere to
> > > > > start using them.
> > > > >
> > > > > The downside of keeping the nodes is it creates an ABI where there
> > > > > doesn't need to be one necessarily.
> > > >
> > > > I'd love to get away from the idea that DT is just for the OS. We are
> > > > trying to use it for firmware-to-firmware communication, too. The
> > > > programs on each side of that interface may be different projects. For
> > > > that, we do need to have a binding, otherwise we end up with series
> > > > like this one.
> > >
> > > I don't think DT is just for the OS, but DT for the OS is an ABI and
> > > other cases may not be. Or they just don't need to follow all the same
> > > rules. Zephyr is doing their own thing for example.
> > >
> > > I don't think /options/u-boot is an ABI. AIUI, u-boot populates the
> > > node and consumes it. No ABI there (or limited to SPL to u-boot
> > > proper). I suppose a prior firmware stage could populate it, but that
> > > doesn't scale as then the prior stage has to know details of the next
> > > stage.
> >
> > I still don't understand the distinction, or really why U-Boot (or
> > u-boot) is different. The main reason for the /options/u-boot node is
> > so that prior phase firmware can tell U-Boot what to do. It is similar
> > to the /chosen node for the OS. If it is not an ABI, how can any
> > project rely on it? It is absolutely not just U-Boot generating
> > something for its own consumption. Where is that idea even coming
> > from? We even have an OF_HAS_PRIOR_STAGE Kconfig option for it in
> > U-Boot, specifically for TF-A, etc.
>
> Okay, I misunderstood the intent since I thought u-boot gets its DT
> from the one in the u-boot tree (at least usually).

That sounds right, although it isn't the intended destination. But in
any case, we want to use the same DT as is in Linux, so Linux needs to
accept DTs with these properties in them.

>
> Though I just looked at TF-A source and see no evidence of it doing
> anything with /options or /options/u-boot. I'm really not that
> interested in what is possible and only used by 1 board and fork
> somewhere. I'm interested in supporting what the community defines as
> best practice (e.g. DT (for OS) comes from firmware), but so far that
> hasn't been defined below the firmware-OS level. Yes, there's efforts
> addressing aspects of it, but it's not really clear to me what the
> high-level goals are and what projects are onboard.

So, chicken and egg? U-Boot cannot define /options until other
projects add them, but they won't until U-Boot defines them?

>
> Note that I did find "/secure-chosen" though. And if they want to use
> that, then fine, but I don't want to see it.

I wonder what that is? Is it in the bindings anywhere?

>
> > As to the scaling, I agree. That suggests we try to make things
> > generic, i.e. have an /options node with these sorts of generic
> > settings. Candidates for that are console and logging controls, for
> > example. I would prefer that to /options/u-boot, as you know.
> >
> > >
> > > > > > The real problem is that some folks developed a certification program
> > > > > > that allegedly requires schema verification and now propose adding
> > > > > > code to U-Boot that doesn't really solve any problem.  My proposed
> > > > > > solution would be to change said certification program to allow
> > > > > > firmware to augment the device tree with properties and nodes with
> > > > > > compatibles that are in the namespace controlled by the firmware.
> > > > >
> > > > > I don't think we should decide what to do here based on said
> > > > > certification program. It can adapt to what's decided. Probably, the
> > > > > /option nodes will be stripped out or ignored for certification.
> > > > >
> > > > > I accepted u-boot options node schema into dtschema, but now have
> > > > > second thoughts on that. Now I'm getting more u-boot specific
> > > > > (perhaps, not clear) stuff and widevine stuff internal to a TEE.
> > > >
> > > > Where should these bindings go such that ARM / Linaro are not trying
> > > > to remove them? I would be OK with moving them out somewhere else, but
> > > > how are people supposed to deal with such fragmentation? My
> > > > understanding was that dt-schema was an attempt to set up a neutral
> > > > area where bindings could be accepted that were not just for
> > > > Linux...did that change?
> > >
> > > No change, just not communicated I guess. And again, bindings are not
> > > "just for Linux". They are hosted there because that is where *all*
> > > the hardware support is (by far). But we'll probably never get past
> > > the "Linux binding" perception no matter what we do or how many times
> > > I say it.
> > >
> > > To rephrase things a bit, I'm happy to host anything that's
> > > multi-project, not a large number of bindings, and not a
> > > device/hardware specific binding. The device specific bindings live in
> > > the kernel tree primarily. For any new binding (foos/#foo-cell type
> > > ones), I want to see multiple users. Really for these, probably best
> > > to start with them in Linux repo (or elsewhere) and then promote them
> > > to dtschema. That gives a little flexibility in changing/abandoning
> > > them.
> >
> > So now I am not sure what you are suggesting. Are you wanting bindings
> > in many places (yours, Linux, U-Boot, TF-A, ...)? I am sure you are
> > aware that if we put bindings in U-Boot they will not be considered as
> > bindings at all, including by Linaro.
>
> If the instances of a binding are only produced and consumed in
> $project, then it is perfectly fine to host the bindings in $project.
> It's also not something fixed. It can start out in $project and get
> moved out if it proves useful elsewhere.

Does Linaro agree with that, though? We have this thread which suggests not.

I really think we need to resolve this somehow. It is just causing so
much churn and confusion.

How about we agree that firmware can have bindings and that the DTs
with those bindings can be in Linux, with no conditions other than
binding review?

>
> > To restate the problem, we need (and until your previous email I
> > thought we had) a unified repo where cross-project, firmware-targeted
> > bindings can be accepted and agreed by interested projects.
>
> I'm happy to take those (assuming I'm not bombarded with 100s). So
> far, nothing I've seen is clearly cross project. Send me stuff that
> has acks from multiple projects.

Part of the disconnect here is that you seem to be assuming that each
project has its own DTs and hacks them up as much as it likes, but
that is not the world I advocate. I would like to have one DT which
can support both firmware and OS.

>
> > > Removing nodes and/or properties and where things live are mostly
> > > independent discussions. SystemReady can adapt to whatever is decided
> > > for the former. In general, IMO, when passing DT from stage N to N+1,
> > > the N+1 stage should remove things which only apply to N+2 stage. For
> > > example, kexec removes /chosen and recreates it for the next kernel.
> >
> > Sounds good. But note that the reason for that is a conflict, since
> > the node is used for different things - i.e. the two kernels need
> > different settings. That is quite different from the case I am talking
> > about, where the settings either apply globally (to interested
> > projects) or to a single project (for all boot phases of that
> > project). In my case, I believe there is no need to remove anything.
>
> There is also no need to keep it. We can always decide to keep it
> later if there is a need. We can't decide later to remove it because
> someone may be relying on it.

Is this an open source project relying on it, or a closed-source one?
Bear in mind that if someone removes a firmware feature and the OS
doesn't boot, they will fix the firmware.

Anyway, I don't mind what people do here. I was just pointing out that
using the same node for two different things seems awkward, to say the
least.

>
> > Regards,
> > Simon
> >
> > PS Zephyr is absolutely doing its own thing. Apart from the fact that
> > it doesn't even have a runtime DT, the bindings are entirely within
> > the project. and bear little relation to Linux bindings. I was not
> > around for that decision, but I suspect part of the rationale was that
> > many of the MCUs which Zephyr targets are not supported by Linux. Of
> > course, that is not fully true and I believe it will become less true
> > with time. Then, perhaps, in 5 years it will be Zephyr's turn to think
> > about bindings more deeply.
>
> Little overlap is part of it, but a large part of the reason is Zephyr
> needed something machine parsable and it was before we came up with
> dtschema. There's desire to adopt dtschema, but at this point I
> imagine with the first reason, there is little pressing need. They've
> got something that works for them.

Right, which sounds like where U-Boot started and was for years forced
to stay. That is what I am trying to change.

Regards,
Simon