[PATCH v3] schemas: Add schema for U-Boot driver model 'phase tags'

[PATCH v3] schemas: Add schema for U-Boot driver model 'phase tags'

[Simon Glass]

Until recently it has not been possible to add any U-Boot-specific
properties to the device tree schema. Now that it is somewhat separated
from Linux and people are feeling the enormous pain of the bifurcated
schema, it seems like a good time to add this and other U-Boot-specific
bindings.

U-Boot has some particular challenges with device tree and devices which
are not faced with Linux:

- U-Boot has multiple build phases, such as a Secondary Program Loader
  (SPL) phase which typically runs in a pre-SDRAM environment where code
  and data space are limited. In particular, there may not be enough
  space for the full device tree blob. U-Boot uses various automated
  techniques to reduce the size from perhaps 40KB to 3KB.
- Some U-Boot phases needs to run before the clocks are properly set up,
  where the CPU may be running very slowly. Therefore it is important to
  bind only those devices which are actually needed in that phase
- Unlike Linux or UEFI, U-Boot uses lazy initialisation for its devices,
  with 'bind' and 'probe' being separate steps. Even if a device is bound,
  it is not actually probed until it is used. This is necessary to keep
  the boot time reasonable, e.g. to under a second

The phases of U-Boot in order are: TPL, VPL, SPL, U-Boot (first
pre-relocation, then post-relocation). ALl but the last two are optional.

For the above reasons, U-Boot only includes the full device tree in the
final 'U-Boot proper' build. Even then, before relocation U-Boot only
processes nodes which are marked as being needed.

For this to work, U-Boot's driver model[1] provides a way to mark device
tree nodes as applicable for a particular phase. This works by adding a
tag to the node, e.g.:

   cru: clock-controller@ff760000 {
      u-boot,dm-all;
      compatible = "rockchip,rk3399-cru";
      reg = <0x0 0xff760000 0x0 0x1000>;
      rockchip,grf = <&grf>;
      #clock-cells = <1>;
      #reset-cells = <1>;
      ...
   };

Here the "u-boot,dm-all" tag indicates that the node must be present in
all phases, since the clock driver is required

There has been discussion over the years about whether this could be done
in a property instead, e.g.

   options {
      u-boot,dm-all = <&cru> <&gpio_a> ...;
      ...
   };

Some problems with this:

- we need to be able to merge several such tags from different .dtsi files
  since many boards have their own specific requirements
- it is hard to find and cross-reference the affected nodes
- it is more error-prone
- it requires significant tool rework in U-Boot, including fdtgrep and
  the build system
- is harder (slower, more code) to process since it involves scanning
  another node/property to find out what to do with a particular node
- we don't want to add phandle arguments to the above since we are
  referring, e.g., to the clock device as a whole, not a paricular clock
- the of-platdata feature[2], which converts device tree to C for even
  more constrained environments, would need to become aware of the
  /options node

There is also the question about whether this needs to be U-Boot-specific,
or whether the tags could be generic. From what I can tell, U-Boot is the
only bootloader which seriously attempts to use a runtime device tree in
all cases. We could perhaps adopt U-Boot's naming for the phases and drop
the "u-boot," prefix, but that might cause confusion.

It should also be noted that the approach provided here has stood the test
of time, used in U-Boot for 8 years so far.

So add the schema for this. This will allow a major class of schema
exceptions to be dropped from the U-Boot source tree.

This being sent to the mailing list since it might attract more review.
A PR will be sent when this has had some review. That is why the file
path is set up for https://github.com/devicetree-org/dt-schema rather
than the Linux kernel.

[1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/index.html
[2] https://u-boot.readthedocs.io/en/latest/develop/driver-model/of-plat.html

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v3:
- Fix an incorrect schema path in $id

Changes in v2:
- Expand docs to include a description of each tag
- Fix some typos and unclear wording

 dtschema/schemas/u-boot.yaml | 66 ++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 dtschema/schemas/u-boot.yaml

diff --git a/dtschema/schemas/u-boot.yaml b/dtschema/schemas/u-boot.yaml
new file mode 100644
index 0000000..b888b3e
--- /dev/null
+++ b/dtschema/schemas/u-boot.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: BSD-2-Clause
+# Copyright 2022 Google LLC
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/u-boot.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bindings required by U-Boot driver model
+
+maintainers:
+  - Simon Glass <sjg@chromium.org>
+
+patternProperties:
+  "^u-boot,dm-(tpl|vpl|spl|all|pre-reloc)$":
+    type: boolean
+    description: |
+      The device tree is often quite large (e.g. 40KB) and cannot fit into xPL
+      phases like SPL, TPL. Even with U-Boot proper, we normally only want to
+      init a small subset of devices before relocation.
+
+      U-Boot supports adding tags to device tree nodes to allow them to be
+      marked according to the U-Boot phases where they should be included.
+
+      Without any tags, nodes are included only in U-Boot proper after
+      relocation. Any untagged nodes are dropped from xPL and are ignored before
+      relocation in U-Boot proper.
+
+      The xPL builds use fdtgrep drop any nodes which are not needed for that
+      build. For example, TPL will drop any nodes which are not marked with
+      u-boot,dm-tpl or u-boot,dm-all tags.
+
+      The available tags are as follows:
+
+        u-boot,dm-all:
+          Include this node in all phases (see enum u_boot_phase)
+
+        u-boot,dm-pre-reloc:
+          Enable this node before relocation in U-Boot proper
+
+        u-boot,dm-tpl:
+          Enable this node in the Tertiary Program Loader (TPL)
+
+        u-boot,dm-vpl:
+          Enable this node in the Verifying Program Loader (VPL)
+
+        u-boot,dm-spl:
+          Enable this node in the Secondary Program Loader (SPL)
+
+      Note that xPL builds also drop the tags, since they have served their
+      purpose by that point. So when looking at xPL device tree files you will
+      not see these tags. This means, for example, that ofnode_pre_reloc()
+      always returns true in xPL builds. This is done to save space.
+
+      Multiple tags can be used in the same node.
+
+      One complication is that tags apply only to the node they are added to,
+      not to any parents. This means that you often need to add the same tag to
+      parent nodes so that any properties needed by the parent driver are
+      included. Without that, the parent node may have no properties, or may not
+      be bound before relocation (meaning that its child will not be bound
+      either).
+
+      The phases of U-Boot in order are: TPL, VPL, SPL, U-Boot (first
+      pre-relocation, then post-relocation). ALl but the last two are optional.
+
+additionalProperties: true
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH v3] schemas: Add schema for U-Boot driver model 'phase tags'

[Sean Anderson]

On 10/4/22 19:22, Simon Glass wrote:
> Until recently it has not been possible to add any U-Boot-specific
> properties to the device tree schema. Now that it is somewhat separated
> from Linux and people are feeling the enormous pain of the bifurcated
> schema, it seems like a good time to add this and other U-Boot-specific
> bindings.
> 
> U-Boot has some particular challenges with device tree and devices which
> are not faced with Linux:
> 
> - U-Boot has multiple build phases, such as a Secondary Program Loader
>    (SPL) phase which typically runs in a pre-SDRAM environment where code
>    and data space are limited. In particular, there may not be enough
>    space for the full device tree blob. U-Boot uses various automated
>    techniques to reduce the size from perhaps 40KB to 3KB.
> - Some U-Boot phases needs to run before the clocks are properly set up,
>    where the CPU may be running very slowly. Therefore it is important to
>    bind only those devices which are actually needed in that phase
> - Unlike Linux or UEFI, U-Boot uses lazy initialisation for its devices,
>    with 'bind' and 'probe' being separate steps. Even if a device is bound,
>    it is not actually probed until it is used. This is necessary to keep
>    the boot time reasonable, e.g. to under a second
> 
> The phases of U-Boot in order are: TPL, VPL, SPL, U-Boot (first
> pre-relocation, then post-relocation). ALl but the last two are optional.

nit: All

> 
> For the above reasons, U-Boot only includes the full device tree in the
> final 'U-Boot proper' build. Even then, before relocation U-Boot only
> processes nodes which are marked as being needed.
> 
> For this to work, U-Boot's driver model[1] provides a way to mark device
> tree nodes as applicable for a particular phase. This works by adding a
> tag to the node, e.g.:
> 
>     cru: clock-controller@ff760000 {
>        u-boot,dm-all;
>        compatible = "rockchip,rk3399-cru";
>        reg = <0x0 0xff760000 0x0 0x1000>;
>        rockchip,grf = <&grf>;
>        #clock-cells = <1>;
>        #reset-cells = <1>;
>        ...
>     };
> 
> Here the "u-boot,dm-all" tag indicates that the node must be present in
> all phases, since the clock driver is required
> 
> There has been discussion over the years about whether this could be done
> in a property instead, e.g.
> 
>     options {
>        u-boot,dm-all = <&cru> <&gpio_a> ...;
>        ...
>     };
> 
> Some problems with this:
> 
> - we need to be able to merge several such tags from different .dtsi files
>    since many boards have their own specific requirements
> - it is hard to find and cross-reference the affected nodes
> - it is more error-prone
> - it requires significant tool rework in U-Boot, including fdtgrep and
>    the build system
> - is harder (slower, more code) to process since it involves scanning
>    another node/property to find out what to do with a particular node
> - we don't want to add phandle arguments to the above since we are
>    referring, e.g., to the clock device as a whole, not a paricular clock
> - the of-platdata feature[2], which converts device tree to C for even
>    more constrained environments, would need to become aware of the
>    /options node
> 
> There is also the question about whether this needs to be U-Boot-specific,
> or whether the tags could be generic. From what I can tell, U-Boot is the
> only bootloader which seriously attempts to use a runtime device tree in
> all cases. We could perhaps adopt U-Boot's naming for the phases and drop
> the "u-boot," prefix, but that might cause confusion.
> 
> It should also be noted that the approach provided here has stood the test
> of time, used in U-Boot for 8 years so far.
> 
> So add the schema for this. This will allow a major class of schema
> exceptions to be dropped from the U-Boot source tree.
> 
> This being sent to the mailing list since it might attract more review.
> A PR will be sent when this has had some review. That is why the file
> path is set up for https://github.com/devicetree-org/dt-schema rather
> than the Linux kernel.
> 
> [1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/index.html
> [2] https://u-boot.readthedocs.io/en/latest/develop/driver-model/of-plat.html
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v3:
> - Fix an incorrect schema path in $id
> 
> Changes in v2:
> - Expand docs to include a description of each tag
> - Fix some typos and unclear wording
> 
>   dtschema/schemas/u-boot.yaml | 66 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 66 insertions(+)
>   create mode 100644 dtschema/schemas/u-boot.yaml
> 
> diff --git a/dtschema/schemas/u-boot.yaml b/dtschema/schemas/u-boot.yaml
> new file mode 100644
> index 0000000..b888b3e
> --- /dev/null
> +++ b/dtschema/schemas/u-boot.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: BSD-2-Clause
> +# Copyright 2022 Google LLC
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/u-boot.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bindings required by U-Boot driver model
> +
> +maintainers:
> +  - Simon Glass <sjg@chromium.org>
> +
> +patternProperties:
> +  "^u-boot,dm-(tpl|vpl|spl|all|pre-reloc)$":
> +    type: boolean
> +    description: |
> +      The device tree is often quite large (e.g. 40KB) and cannot fit into xPL
> +      phases like SPL, TPL. Even with U-Boot proper, we normally only want to
> +      init a small subset of devices before relocation.
> +
> +      U-Boot supports adding tags to device tree nodes to allow them to be
> +      marked according to the U-Boot phases where they should be included.
> +
> +      Without any tags, nodes are included only in U-Boot proper after
> +      relocation. Any untagged nodes are dropped from xPL and are ignored before
> +      relocation in U-Boot proper.
> +
> +      The xPL builds use fdtgrep drop any nodes which are not needed for that
> +      build. For example, TPL will drop any nodes which are not marked with
> +      u-boot,dm-tpl or u-boot,dm-all tags.
> +
> +      The available tags are as follows:
> +
> +        u-boot,dm-all:
> +          Include this node in all phases (see enum u_boot_phase)
> +
> +        u-boot,dm-pre-reloc:
> +          Enable this node before relocation in U-Boot proper
> +
> +        u-boot,dm-tpl:
> +          Enable this node in the Tertiary Program Loader (TPL)
> +
> +        u-boot,dm-vpl:
> +          Enable this node in the Verifying Program Loader (VPL)
> +
> +        u-boot,dm-spl:
> +          Enable this node in the Secondary Program Loader (SPL)
> +
> +      Note that xPL builds also drop the tags, since they have served their
> +      purpose by that point. So when looking at xPL device tree files you will
> +      not see these tags. This means, for example, that ofnode_pre_reloc()
> +      always returns true in xPL builds. This is done to save space.
> +
> +      Multiple tags can be used in the same node.
> +
> +      One complication is that tags apply only to the node they are added to,
> +      not to any parents. This means that you often need to add the same tag to
> +      parent nodes so that any properties needed by the parent driver are
> +      included. Without that, the parent node may have no properties, or may not
> +      be bound before relocation (meaning that its child will not be bound
> +      either).
> +
> +      The phases of U-Boot in order are: TPL, VPL, SPL, U-Boot (first
> +      pre-relocation, then post-relocation). ALl but the last two are optional.

nit: All

> +
> +additionalProperties: true

(The rest of this LGTM)

Re: [PATCH v3] schemas: Add schema for U-Boot driver model 'phase tags'

[Rob Herring]

On Tue, Oct 4, 2022 at 6:22 PM Simon Glass <sjg@chromium.org> wrote:
>
> Until recently it has not been possible to add any U-Boot-specific
> properties to the device tree schema. Now that it is somewhat separated
> from Linux and people are feeling the enormous pain of the bifurcated
> schema, it seems like a good time to add this and other U-Boot-specific
> bindings.

It's been possible provided there was agreement on the properties.
There just wasn't in this case.

What's the pain point precisely? I can think of several. Syncing dts
files from Linux tree, running schema validation on a dtb from u-boot,
or ...?

> U-Boot has some particular challenges with device tree and devices which
> are not faced with Linux:
>
> - U-Boot has multiple build phases, such as a Secondary Program Loader
>   (SPL) phase which typically runs in a pre-SDRAM environment where code
>   and data space are limited. In particular, there may not be enough
>   space for the full device tree blob. U-Boot uses various automated
>   techniques to reduce the size from perhaps 40KB to 3KB.

As this is a build time thing, would this be better handled as a
source annotation rather than properties? We already have a 'delete if
unreferenced' tag for similar reasons. We could add something more
flexible like '/tag spl, tpl/' and dtc could have command line options
to keep or delete nodes based on tag names. (Added David G for his
thoughts on this)

Yes, there is also a runtime need for a portion of this, but I think
it's 2 different problems AIUI.

> - Some U-Boot phases needs to run before the clocks are properly set up,
>   where the CPU may be running very slowly. Therefore it is important to
>   bind only those devices which are actually needed in that phase
> - Unlike Linux or UEFI, U-Boot uses lazy initialisation for its devices,
>   with 'bind' and 'probe' being separate steps. Even if a device is bound,
>   it is not actually probed until it is used. This is necessary to keep
>   the boot time reasonable, e.g. to under a second

Linux could do this now if we wanted. There's a full dependency graph.
Once you have that, it's just an implementation decision whether you
probe top down or bottom up. We have this graph because Linux specific
probing hint properties in DT was rejected. (Not saying u-boot needs
to go implement a dependency graph, but rather u-boot is not unique
here and there's more than one way to solve it.)

> The phases of U-Boot in order are: TPL, VPL, SPL, U-Boot (first
> pre-relocation, then post-relocation). ALl but the last two are optional.
>
> For the above reasons, U-Boot only includes the full device tree in the
> final 'U-Boot proper' build. Even then, before relocation U-Boot only
> processes nodes which are marked as being needed.
>
> For this to work, U-Boot's driver model[1] provides a way to mark device
> tree nodes as applicable for a particular phase. This works by adding a
> tag to the node, e.g.:
>
>    cru: clock-controller@ff760000 {
>       u-boot,dm-all;
>       compatible = "rockchip,rk3399-cru";
>       reg = <0x0 0xff760000 0x0 0x1000>;
>       rockchip,grf = <&grf>;
>       #clock-cells = <1>;
>       #reset-cells = <1>;
>       ...
>    };
>
> Here the "u-boot,dm-all" tag indicates that the node must be present in
> all phases, since the clock driver is required
>
> There has been discussion over the years about whether this could be done
> in a property instead, e.g.
>
>    options {
>       u-boot,dm-all = <&cru> <&gpio_a> ...;
>       ...
>    };
>
> Some problems with this:
>
> - we need to be able to merge several such tags from different .dtsi files
>   since many boards have their own specific requirements

A source annotation to append/prepend properties could solve that.

> - it is hard to find and cross-reference the affected nodes
> - it is more error-prone
> - it requires significant tool rework in U-Boot, including fdtgrep and
>   the build system
> - is harder (slower, more code) to process since it involves scanning
>   another node/property to find out what to do with a particular node
> - we don't want to add phandle arguments to the above since we are
>   referring, e.g., to the clock device as a whole, not a paricular clock
> - the of-platdata feature[2], which converts device tree to C for even
>   more constrained environments, would need to become aware of the
>   /options node
>
> There is also the question about whether this needs to be U-Boot-specific,
> or whether the tags could be generic. From what I can tell, U-Boot is the
> only bootloader which seriously attempts to use a runtime device tree in
> all cases. We could perhaps adopt U-Boot's naming for the phases and drop
> the "u-boot," prefix, but that might cause confusion.

I am concerned about the u-boot adding your own properties, and then
the next consumer wanting to add something similar. Even dropping
'u-boot,' the properties seem pretty u-boot specific. Of them, SPL
seems like the least specific as that's a pre-DRAM state. VPL/TPL
don't appear to be widely used. Pre/post relocation seems pretty
implementation specific unless you can define what state that implies.
So maybe a mixture with and without 'u-boot'?

> It should also be noted that the approach provided here has stood the test
> of time, used in U-Boot for 8 years so far.
>
> So add the schema for this. This will allow a major class of schema
> exceptions to be dropped from the U-Boot source tree.

This schema won't actually work for validation. The issue is for any
given node, we need a schema that includes all possible properties for
that node. That's necessary to detect any unknown properties.
Typically, we have the device's schema with references to any common
'class' schemas. There are some exceptions of properties allowed on
any node (or allowed when some other property is present). That is all
handled as a schema fixup within the tool currently, but I've been
thinking of changing that to an 'all nodes' schema file. Minimally,
we'd just need to add '^u-boot,dm-' to the list. Look for 'status' or
'pinctrl-' in the dtschema.

> This being sent to the mailing list since it might attract more review.
> A PR will be sent when this has had some review. That is why the file
> path is set up for https://github.com/devicetree-org/dt-schema rather
> than the Linux kernel.
>
> [1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/index.html
> [2] https://u-boot.readthedocs.io/en/latest/develop/driver-model/of-plat.html
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3:
> - Fix an incorrect schema path in $id
>
> Changes in v2:
> - Expand docs to include a description of each tag
> - Fix some typos and unclear wording
>
>  dtschema/schemas/u-boot.yaml | 66 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 dtschema/schemas/u-boot.yaml
>
> diff --git a/dtschema/schemas/u-boot.yaml b/dtschema/schemas/u-boot.yaml
> new file mode 100644
> index 0000000..b888b3e
> --- /dev/null
> +++ b/dtschema/schemas/u-boot.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: BSD-2-Clause
> +# Copyright 2022 Google LLC
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/u-boot.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bindings required by U-Boot driver model
> +
> +maintainers:
> +  - Simon Glass <sjg@chromium.org>
> +
> +patternProperties:
> +  "^u-boot,dm-(tpl|vpl|spl|all|pre-reloc)$":
> +    type: boolean
> +    description: |
> +      The device tree is often quite large (e.g. 40KB) and cannot fit into xPL
> +      phases like SPL, TPL. Even with U-Boot proper, we normally only want to
> +      init a small subset of devices before relocation.
> +
> +      U-Boot supports adding tags to device tree nodes to allow them to be
> +      marked according to the U-Boot phases where they should be included.
> +
> +      Without any tags, nodes are included only in U-Boot proper after
> +      relocation. Any untagged nodes are dropped from xPL and are ignored before
> +      relocation in U-Boot proper.
> +
> +      The xPL builds use fdtgrep drop any nodes which are not needed for that
> +      build. For example, TPL will drop any nodes which are not marked with
> +      u-boot,dm-tpl or u-boot,dm-all tags.
> +
> +      The available tags are as follows:
> +
> +        u-boot,dm-all:
> +          Include this node in all phases (see enum u_boot_phase)
> +
> +        u-boot,dm-pre-reloc:
> +          Enable this node before relocation in U-Boot proper
> +
> +        u-boot,dm-tpl:
> +          Enable this node in the Tertiary Program Loader (TPL)
> +
> +        u-boot,dm-vpl:
> +          Enable this node in the Verifying Program Loader (VPL)
> +
> +        u-boot,dm-spl:
> +          Enable this node in the Secondary Program Loader (SPL)
> +
> +      Note that xPL builds also drop the tags, since they have served their
> +      purpose by that point. So when looking at xPL device tree files you will
> +      not see these tags. This means, for example, that ofnode_pre_reloc()
> +      always returns true in xPL builds. This is done to save space.
> +
> +      Multiple tags can be used in the same node.
> +
> +      One complication is that tags apply only to the node they are added to,
> +      not to any parents. This means that you often need to add the same tag to
> +      parent nodes so that any properties needed by the parent driver are
> +      included. Without that, the parent node may have no properties, or may not
> +      be bound before relocation (meaning that its child will not be bound
> +      either).

Why is this? That seems at best redundant and at worst pretty broken.
For example, If you dropped 'ranges' and the associated address
properties, then decoding 'reg' is broken. It would only kind of work
if you just assume 1:1 address translation. The resulting dtb would
also not pass validation.

Rob

Re: [PATCH v3] schemas: Add schema for U-Boot driver model 'phase tags'

[Tom Rini]

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

On Mon, Oct 10, 2022 at 11:34:00AM -0500, Rob Herring wrote:
> On Tue, Oct 4, 2022 at 6:22 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Until recently it has not been possible to add any U-Boot-specific
> > properties to the device tree schema. Now that it is somewhat separated
> > from Linux and people are feeling the enormous pain of the bifurcated
> > schema, it seems like a good time to add this and other U-Boot-specific
> > bindings.
> 
> It's been possible provided there was agreement on the properties.
> There just wasn't in this case.

To jump on this part at least, I feel (but human memory is not the
most reliable) it being a bit more than that, but, that's history and
we're all moving forward now.  I think for v4 the commit message needs
to be edited to remove the historical sentiment and just state what
we're adding and why / who it's useful for.

-- 
Tom

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

Re: [PATCH v3] schemas: Add schema for U-Boot driver model 'phase tags'

[Tom Rini]

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

On Mon, Oct 10, 2022 at 11:34:00AM -0500, Rob Herring wrote:
> On Tue, Oct 4, 2022 at 6:22 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Until recently it has not been possible to add any U-Boot-specific
> > properties to the device tree schema. Now that it is somewhat separated
> > from Linux and people are feeling the enormous pain of the bifurcated
> > schema, it seems like a good time to add this and other U-Boot-specific
> > bindings.
> 
> It's been possible provided there was agreement on the properties.
> There just wasn't in this case.
> 
> What's the pain point precisely? I can think of several. Syncing dts
> files from Linux tree, running schema validation on a dtb from u-boot,
> or ...?

So, we have a few related pain points. As background, one thing we do
today is have a "-u-boot.dtsi" file and then cmd_dtc in
scripts/Makefile.lib (which is +- the same as Linux) has logic to find
and -include it. This file is supposed to include all of the properties
that U-Boot needs added (and this patch schema is intended to start
addressing) to produce the dtb we need. This information can't be
accepted in the upstream dts files as there's no binding schema
associated with it. It's only a pain point during re-sync when trees
have been fixed upstream and now node names change and so forth. But it
does mean that we can't just drop in new dts files. We aren't attempting
to run validation ourselves right now, but getting something like this
merged means we can change our new dts requirement from "must be in
Linus' tree or at least linux-next" to "must be in Linus' tree or at
least linux-next and passing the dtschema check".

[snip]
> > - Some U-Boot phases needs to run before the clocks are properly set up,
> >   where the CPU may be running very slowly. Therefore it is important to
> >   bind only those devices which are actually needed in that phase
> > - Unlike Linux or UEFI, U-Boot uses lazy initialisation for its devices,
> >   with 'bind' and 'probe' being separate steps. Even if a device is bound,
> >   it is not actually probed until it is used. This is necessary to keep
> >   the boot time reasonable, e.g. to under a second
> 
> Linux could do this now if we wanted. There's a full dependency graph.
> Once you have that, it's just an implementation decision whether you
> probe top down or bottom up. We have this graph because Linux specific
> probing hint properties in DT was rejected. (Not saying u-boot needs
> to go implement a dependency graph, but rather u-boot is not unique
> here and there's more than one way to solve it.)

Further points in the commit message that need to be reworded as what
U-Boot does and not what other projects might not be doing.

-- 
Tom

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

Re: [PATCH v3] schemas: Add schema for U-Boot driver model 'phase tags'

[Simon Glass]

Hi Rob,

On Mon, 10 Oct 2022 at 10:34, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Oct 4, 2022 at 6:22 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Until recently it has not been possible to add any U-Boot-specific
> > properties to the device tree schema. Now that it is somewhat separated
> > from Linux and people are feeling the enormous pain of the bifurcated
> > schema, it seems like a good time to add this and other U-Boot-specific
> > bindings.
>
> It's been possible provided there was agreement on the properties.
> There just wasn't in this case.
>
> What's the pain point precisely? I can think of several. Syncing dts
> files from Linux tree, running schema validation on a dtb from u-boot,
> or ...?

U-Boot

See Tom's response.

As Tom says, I am not bothered about history and can change anything
in the commit message. Just so long as history never repeats!

>
> > U-Boot has some particular challenges with device tree and devices which
> > are not faced with Linux:
> >
> > - U-Boot has multiple build phases, such as a Secondary Program Loader
> >   (SPL) phase which typically runs in a pre-SDRAM environment where code
> >   and data space are limited. In particular, there may not be enough
> >   space for the full device tree blob. U-Boot uses various automated
> >   techniques to reduce the size from perhaps 40KB to 3KB.
>
> As this is a build time thing, would this be better handled as a
> source annotation rather than properties? We already have a 'delete if
> unreferenced' tag for similar reasons. We could add something more
> flexible like '/tag spl, tpl/' and dtc could have command line options
> to keep or delete nodes based on tag names. (Added David G for his
> thoughts on this)

If you like. I imagine that would work.

This would involve recompiling the DT from source multiple times,
which we don't currently do. Of all the DTs that are built for a
particular U-Boot board, the 'default' one is selected and used for
SPL.  I cannot think of any problem it would cause, though.

>
> Yes, there is also a runtime need for a portion of this, but I think
> it's 2 different problems AIUI.

It's a little tricky. At present we can pass the full DT to xPL and it
can do the right thing, in principle. We use this sort of thing with
sandbox, which is the test/CI environment running natively on Linux.
If we split this up it would have impacts on that, but it would need
to be tried.

The removal of the tags in SPL (by fdtgrep) and the ignoring of them
in xPL builds is an optimisation to save space:

https://lxr.missinglinkelectronics.com/uboot+v2022.07/drivers/core/ofnode.c#L977

https://lxr.missinglinkelectronics.com/uboot+v2022.07/scripts/Makefile.lib#L584

We need to be careful not to make this impossible to test / debug.

>
> > - Some U-Boot phases needs to run before the clocks are properly set up,
> >   where the CPU may be running very slowly. Therefore it is important to
> >   bind only those devices which are actually needed in that phase
> > - Unlike Linux or UEFI, U-Boot uses lazy initialisation for its devices,
> >   with 'bind' and 'probe' being separate steps. Even if a device is bound,
> >   it is not actually probed until it is used. This is necessary to keep
> >   the boot time reasonable, e.g. to under a second
>
> Linux could do this now if we wanted. There's a full dependency graph.
> Once you have that, it's just an implementation decision whether you
> probe top down or bottom up. We have this graph because Linux specific
> probing hint properties in DT was rejected. (Not saying u-boot needs
> to go implement a dependency graph, but rather u-boot is not unique
> here and there's more than one way to solve it.)

OK good.

As Tom notes, I can remove any references to other projects from the
commit message. They are not relevant to operation, but I wanted to
save a dozen back-and-forward emails by trying to cover the obvious
points up front, so I seem to have included things that are
irrelevant.

BTW, does Linux separate bind() from probe()? I had not seen that up to now.

I'm also interested in the rejected hint-properties patch, if you can
point to it.

>
> > The phases of U-Boot in order are: TPL, VPL, SPL, U-Boot (first
> > pre-relocation, then post-relocation). ALl but the last two are optional.
> >
> > For the above reasons, U-Boot only includes the full device tree in the
> > final 'U-Boot proper' build. Even then, before relocation U-Boot only
> > processes nodes which are marked as being needed.
> >
> > For this to work, U-Boot's driver model[1] provides a way to mark device
> > tree nodes as applicable for a particular phase. This works by adding a
> > tag to the node, e.g.:
> >
> >    cru: clock-controller@ff760000 {
> >       u-boot,dm-all;
> >       compatible = "rockchip,rk3399-cru";
> >       reg = <0x0 0xff760000 0x0 0x1000>;
> >       rockchip,grf = <&grf>;
> >       #clock-cells = <1>;
> >       #reset-cells = <1>;
> >       ...
> >    };
> >
> > Here the "u-boot,dm-all" tag indicates that the node must be present in
> > all phases, since the clock driver is required
> >
> > There has been discussion over the years about whether this could be done
> > in a property instead, e.g.
> >
> >    options {
> >       u-boot,dm-all = <&cru> <&gpio_a> ...;
> >       ...
> >    };
> >
> > Some problems with this:
> >
> > - we need to be able to merge several such tags from different .dtsi files
> >   since many boards have their own specific requirements
>
> A source annotation to append/prepend properties could solve that.

Yes, it has been discussed. I have oscillated back and forth on this
one, but in recent years have settled pretty firmly on what we have.
At least once I suggested that someone implement it to try it out, but
he/she didn't bite. It seems a bit hard to justify to me so I've never
tried implementing it myself. See below though:

>
> > - it is hard to find and cross-reference the affected nodes
> > - it is more error-prone
> > - it requires significant tool rework in U-Boot, including fdtgrep and
> >   the build system
> > - is harder (slower, more code) to process since it involves scanning
> >   another node/property to find out what to do with a particular node
> > - we don't want to add phandle arguments to the above since we are
> >   referring, e.g., to the clock device as a whole, not a paricular clock
> > - the of-platdata feature[2], which converts device tree to C for even
> >   more constrained environments, would need to become aware of the
> >   /options node
> >
> > There is also the question about whether this needs to be U-Boot-specific,
> > or whether the tags could be generic. From what I can tell, U-Boot is the
> > only bootloader which seriously attempts to use a runtime device tree in
> > all cases. We could perhaps adopt U-Boot's naming for the phases and drop
> > the "u-boot," prefix, but that might cause confusion.
>
> I am concerned about the u-boot adding your own properties, and then
> the next consumer wanting to add something similar. Even dropping
> 'u-boot,' the properties seem pretty u-boot specific. Of them, SPL
> seems like the least specific as that's a pre-DRAM state. VPL/TPL
> don't appear to be widely used. Pre/post relocation seems pretty
> implementation specific unless you can define what state that implies.
> So maybe a mixture with and without 'u-boot'?

We could define these generically if you like. I just thought it would
be easier to get U-Boot tags since I believe projects should be able
to add their own tags now without too much challenge from Linux.

Here is how I would define them:

TPL - SoC setup (least possible init needed to run VPL)
VPL - verification / selection of firmware
SPL - set up RAM
U-Boot pre-reloc - prepare for relocation
U-Boot - main program

If you select which ones you think are generic, then we could do that.
I would still want U-Boot aliases though, since it is confusing if we
have to flip between different naming conventions in different parts
of the device tree.

>
> > It should also be noted that the approach provided here has stood the test
> > of time, used in U-Boot for 8 years so far.
> >
> > So add the schema for this. This will allow a major class of schema
> > exceptions to be dropped from the U-Boot source tree.
>
> This schema won't actually work for validation. The issue is for any
> given node, we need a schema that includes all possible properties for
> that node. That's necessary to detect any unknown properties.
> Typically, we have the device's schema with references to any common
> 'class' schemas. There are some exceptions of properties allowed on
> any node (or allowed when some other property is present). That is all
> handled as a schema fixup within the tool currently, but I've been
> thinking of changing that to an 'all nodes' schema file. Minimally,
> we'd just need to add '^u-boot,dm-' to the list. Look for 'status' or
> 'pinctrl-' in the dtschema.

OK I see. I  saw pinctrl and wasn't sure if there was magic there.

>
> > This being sent to the mailing list since it might attract more review.
> > A PR will be sent when this has had some review. That is why the file
> > path is set up for https://github.com/devicetree-org/dt-schema rather
> > than the Linux kernel.
> >
> > [1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/index.html
> > [2] https://u-boot.readthedocs.io/en/latest/develop/driver-model/of-plat.html
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Fix an incorrect schema path in $id
> >
> > Changes in v2:
> > - Expand docs to include a description of each tag
> > - Fix some typos and unclear wording
> >
> >  dtschema/schemas/u-boot.yaml | 66 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >  create mode 100644 dtschema/schemas/u-boot.yaml
> >
> > diff --git a/dtschema/schemas/u-boot.yaml b/dtschema/schemas/u-boot.yaml
> > new file mode 100644
> > index 0000000..b888b3e
> > --- /dev/null
> > +++ b/dtschema/schemas/u-boot.yaml
> > @@ -0,0 +1,66 @@
> > +# SPDX-License-Identifier: BSD-2-Clause
> > +# Copyright 2022 Google LLC
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/u-boot.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Bindings required by U-Boot driver model
> > +
> > +maintainers:
> > +  - Simon Glass <sjg@chromium.org>
> > +
> > +patternProperties:
> > +  "^u-boot,dm-(tpl|vpl|spl|all|pre-reloc)$":
> > +    type: boolean
> > +    description: |
> > +      The device tree is often quite large (e.g. 40KB) and cannot fit into xPL
> > +      phases like SPL, TPL. Even with U-Boot proper, we normally only want to
> > +      init a small subset of devices before relocation.
> > +
> > +      U-Boot supports adding tags to device tree nodes to allow them to be
> > +      marked according to the U-Boot phases where they should be included.
> > +
> > +      Without any tags, nodes are included only in U-Boot proper after
> > +      relocation. Any untagged nodes are dropped from xPL and are ignored before
> > +      relocation in U-Boot proper.
> > +
> > +      The xPL builds use fdtgrep drop any nodes which are not needed for that
> > +      build. For example, TPL will drop any nodes which are not marked with
> > +      u-boot,dm-tpl or u-boot,dm-all tags.
> > +
> > +      The available tags are as follows:
> > +
> > +        u-boot,dm-all:
> > +          Include this node in all phases (see enum u_boot_phase)
> > +
> > +        u-boot,dm-pre-reloc:
> > +          Enable this node before relocation in U-Boot proper
> > +
> > +        u-boot,dm-tpl:
> > +          Enable this node in the Tertiary Program Loader (TPL)
> > +
> > +        u-boot,dm-vpl:
> > +          Enable this node in the Verifying Program Loader (VPL)
> > +
> > +        u-boot,dm-spl:
> > +          Enable this node in the Secondary Program Loader (SPL)
> > +
> > +      Note that xPL builds also drop the tags, since they have served their
> > +      purpose by that point. So when looking at xPL device tree files you will
> > +      not see these tags. This means, for example, that ofnode_pre_reloc()
> > +      always returns true in xPL builds. This is done to save space.
> > +
> > +      Multiple tags can be used in the same node.
> > +
> > +      One complication is that tags apply only to the node they are added to,
> > +      not to any parents. This means that you often need to add the same tag to
> > +      parent nodes so that any properties needed by the parent driver are
> > +      included. Without that, the parent node may have no properties, or may not
> > +      be bound before relocation (meaning that its child will not be bound
> > +      either).
>
> Why is this? That seems at best redundant and at worst pretty broken.
> For example, If you dropped 'ranges' and the associated address
> properties, then decoding 'reg' is broken. It would only kind of work
> if you just assume 1:1 address translation. The resulting dtb would
> also not pass validation.

It is mostly due to the complexity of dealing with it. At present
fdtgrep happily works 'forwards' and knows that it must include parent
nodes if a child is present, but it does not automatically include the
properties of that parent node.

I suspect it could be updated to handle this without too much grief.
But as you know, even the current fdtgrep was too complex for David to
consider :-)

I'll await your thoughts on some of the above and aim to send v4 in a week

Regards,
Simon