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

[Simon Glass]

Hi Rob,

On Wed, 11 Jan 2023 at 14:40, Rob Herring <robh at kernel.org> wrote:
>
> On Thu, Nov 10, 2022 at 10:59 AM Simon Glass <sjg at chromium.org> 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 at ff760000 {
> >       phase-all;
> >       compatible = "rockchip,rk3399-cru";
> >       reg = <0x0 0xff760000 0x0 0x1000>;
> >       rockchip,grf = <&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 = <&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. 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 0000000..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 <sjg at chromium.org>
> > +
> > +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