Re: [PATCH v5] schemas: Add schema for U-Boot driver model 'phase tags'
Hi Rob,
On Wed, 11 Jan 2023 at 14:40, Rob Herring wrote:
>
> On Thu, Nov 10, 2022 at 10:59 AM Simon Glass wrote:
> >
> > U-Boot has some particular challenges with device tree and devices:
> >
> > - 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. It is not
> > always possible to handle these tags entirely at build time, since
> > U-Boot proper must have the full device tree, even though we do not
> > want it to process all nodes until after relocation.
> > - 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
> > - 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@ff76 {
> > phase-all;
> > compatible = "rockchip,rk3399-cru";
> > reg = <0x0 0xff76 0x0 0x1000>;
> > rockchip,grf = <>;
> > #clock-cells = <1>;
> > #reset-cells = <1>;
> > ...
> >};
> >
> > Here the "phase-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 {
> > phase-all = <> <_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. For this version, an attempt is made to name the phases in a
> > generic manner.
> >
> > 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.
>
> I guess if no one else has any input at all on alternatives to a
> boolean per phase, then we should just stick with that as that's the
> easiest.
>
> However, I do still think parent nodes need to be implicitly included
> rather than explicitly. Otherwise, the result can be invalid DTs
> (primarily with 'reg' and addressing)
[..]
> > diff --git a/dtschema/schemas/phase.yaml b/dtschema/schemas/phase.yaml
> > new file mode 100644
> > index 000..a6f090d
> > --- /dev/null
> > +++ b/dtschema/schemas/phase.yaml
> > @@ -0,0 +1,74 @@
> > +# SPDX-License-Identifier: BSD-2-Clause
> > +# Copyright 2022 Google LLC
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phase.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Boot-phase-specific device nodes
> > +
> > +maintainers:
> > + - Simon Glass
> > +
> > +patternProperties:
> > + "^phase-(pre-sram,verify,pre-ram,some-ram,all)$":
>
> s/,/|/
>
> To bikeshed the name again, I find 'phase' a bit vague. Perhaps
> 'boot-phase-'? Or 'boot-ph-'?
I really don't like boot-phase-some-ram as we have hyphens in the
'root' property name and in the leaf.
I've gone with bootph as a compromise - I hope you like it.
[..]
Regards,
Simon
Re: [PATCH v5] schemas: Add schema for U-Boot driver model 'phase tags'
On Thu, Nov 10, 2022 at 10:59 AM Simon Glass wrote:
>
> U-Boot has some particular challenges with device tree and devices:
>
> - 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. It is not
> always possible to handle these tags entirely at build time, since
> U-Boot proper must have the full device tree, even though we do not
> want it to process all nodes until after relocation.
> - 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
> - 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@ff76 {
> phase-all;
> compatible = "rockchip,rk3399-cru";
> reg = <0x0 0xff76 0x0 0x1000>;
> rockchip,grf = <>;
> #clock-cells = <1>;
> #reset-cells = <1>;
> ...
>};
>
> Here the "phase-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 {
> phase-all = <> <_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. For this version, an attempt is made to name the phases in a
> generic manner.
>
> 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.
I guess if no one else has any input at all on alternatives to a
boolean per phase, then we should just stick with that as that's the
easiest.
However, I do still think parent nodes need to be implicitly included
rather than explicitly. Otherwise, the result can be invalid DTs
(primarily with 'reg' and addressing)
> 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
> ---
>
> Changes in v5:
> - Fix instructions to run test
> - Update binding title
> - Use 'phase-' instead of 'phase,'
>
> Changes in v4:
> - Drop some unnecessary context from the commit message
> - Explain why parent nodes do not automatically inherit their children's
> tags
> - Rename the tags to use a phase,xxx format, explaining each one
>
> 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/lib.py | 5 +++
> dtschema/schemas/phase.yaml | 74 +
>
[PATCH v5] schemas: Add schema for U-Boot driver model 'phase tags'
U-Boot has some particular challenges with device tree and devices:
- 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. It is not
always possible to handle these tags entirely at build time, since
U-Boot proper must have the full device tree, even though we do not
want it to process all nodes until after relocation.
- 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
- 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@ff76 {
phase-all;
compatible = "rockchip,rk3399-cru";
reg = <0x0 0xff76 0x0 0x1000>;
rockchip,grf = <>;
#clock-cells = <1>;
#reset-cells = <1>;
...
};
Here the "phase-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 {
phase-all = <> <_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. For this version, an attempt is made to name the phases in a
generic manner.
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
---
Changes in v5:
- Fix instructions to run test
- Update binding title
- Use 'phase-' instead of 'phase,'
Changes in v4:
- Drop some unnecessary context from the commit message
- Explain why parent nodes do not automatically inherit their children's
tags
- Rename the tags to use a phase,xxx format, explaining each one
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/lib.py | 5 +++
dtschema/schemas/phase.yaml | 74 +
test/phases.dts | 23
3 files changed, 102 insertions(+)
create mode 100644 dtschema/schemas/phase.yaml
create mode 100644 test/phases.dts
diff --git a/dtschema/lib.py b/dtschema/lib.py
index 1bce070..d65a0fc 100644
--- a/dtschema/lib.py
+++ b/dtschema/lib.py
@@ -514,6 +514,11 @@ def fixup_node_props(schema):
schema['properties'].setdefault('status', True)
schema['properties'].setdefault('secure-status', True)
schema['properties'].setdefault('$nodename', True)
+schema['properties'].setdefault('phase-pre-sram', True)
+
