diff mbox series

[v5,02/15] dt: psci: Update DT bindings to support hierarchical PSCI states

Message ID 20191230144402.30195-3-ulf.hansson@linaro.org (mailing list archive)
State Mainlined
Commit a3f048b5424e68f0f7218c51e84d7b84ab0143cb
Headers show
Series cpuidle: psci: Support hierarchical CPU arrangement | expand

Commit Message

Ulf Hansson Dec. 30, 2019, 2:43 p.m. UTC
Update PSCI DT bindings to allow to represent idle states for CPUs and the
CPU topology, by using a hierarchical layout. Primarily this is done by
re-using the existing DT bindings for PM domains [1] and for PM domain idle
states [2].

Let's also add an example into the document for the PSCI DT bindings, to
clearly show the new hierarchical based layout. The currently supported
flattened layout, is already described in the ARM idle states bindings [3],
so let's leave that as is.

[1] Documentation/devicetree/bindings/power/power_domain.txt
[2] Documentation/devicetree/bindings/power/domain-idle-state.txt
[3] Documentation/devicetree/bindings/arm/idle-states.txt

Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v5:
	- None.

---
 .../devicetree/bindings/arm/cpus.yaml         |  15 +++
 .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
 2 files changed, 119 insertions(+)

Comments

Rob Herring Jan. 13, 2020, 7:53 p.m. UTC | #1
On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Update PSCI DT bindings to allow to represent idle states for CPUs and the
> CPU topology, by using a hierarchical layout. Primarily this is done by
> re-using the existing DT bindings for PM domains [1] and for PM domain idle
> states [2].
>
> Let's also add an example into the document for the PSCI DT bindings, to
> clearly show the new hierarchical based layout. The currently supported
> flattened layout, is already described in the ARM idle states bindings [3],
> so let's leave that as is.
>
> [1] Documentation/devicetree/bindings/power/power_domain.txt
> [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> [3] Documentation/devicetree/bindings/arm/idle-states.txt
>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v5:
>         - None.

First I'm seeing this as the DT list was not copied. The example has
problems when running 'make dt_binding_check':

Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
compatible: Additional items are not allowed ('arm,armv8' was
unexpected)
Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
compatible: Additional items are not allowed ('arm,armv8' was
unexpected)
Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
compatible: ['arm,cortex-a57', 'arm,armv8'] is too long

'arm,armv8' is only valid for s/w models.

Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
expected
Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
expected

The last 2 are due to my conversion of the idle-states binding which
is in my tree now. Probably need to add 'domain-idle-state' as a
compatible at a minimum. It looks like domain-idle-state.txt is pretty
much the same as arm/idle-state.txt, so we should perhaps merge them.

There's some bigger issues though.

> ---
>  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
>  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
>  2 files changed, 119 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> index c23c24ff7575..7a9c3ce2dbef 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> @@ -242,6 +242,21 @@ properties:
>
>        where voltage is in V, frequency is in MHz.
>
> +  power-domains:
> +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> +    description:
> +      List of phandles and PM domain specifiers, as defined by bindings of the
> +      PM domain provider (see also ../power_domain.txt).
> +
> +  power-domain-names:
> +    $ref: '/schemas/types.yaml#/definitions/string-array'
> +    description:
> +      A list of power domain name strings sorted in the same order as the
> +      power-domains property.
> +
> +      For PSCI based platforms, the name corresponding to the index of the PSCI
> +      PM domain provider, must be "psci".
> +
>    qcom,saw:
>      $ref: '/schemas/types.yaml#/definitions/phandle'
>      description: |
> diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> index 7abdf58b335e..8ef85420b2ab 100644
> --- a/Documentation/devicetree/bindings/arm/psci.yaml
> +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> @@ -102,6 +102,34 @@ properties:
>        [1] Kernel documentation - ARM idle states bindings
>          Documentation/devicetree/bindings/arm/idle-states.txt
>
> +  "#power-domain-cells":

This is wrong because you are saying the /psci node should have these
properties. You need to define the child nodes (at least a pattern you
can match on) and put these properties there.

> +    description:
> +      The number of cells in a PM domain specifier as per binding in [3].
> +      Must be 0 as to represent a single PM domain.
> +
> +      ARM systems can have multiple cores, sometimes in an hierarchical
> +      arrangement. This often, but not always, maps directly to the processor
> +      power topology of the system. Individual nodes in a topology have their
> +      own specific power states and can be better represented hierarchically.
> +
> +      For these cases, the definitions of the idle states for the CPUs and the
> +      CPU topology, must conform to the binding in [3]. The idle states
> +      themselves must conform to the binding in [4] and must specify the
> +      arm,psci-suspend-param property.
> +
> +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> +      helps to implement support for OSI mode and OS implementations may choose
> +      to mandate it.
> +
> +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> +
> +  power-domains:
> +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> +    description:
> +      List of phandles and PM domain specifiers, as defined by bindings of the
> +      PM domain provider.

A schema for 'domain-idle-states' property is missing.

>
>  required:
>    - compatible
> @@ -160,4 +188,80 @@ examples:
>        cpu_on = <0x95c10002>;
>        cpu_off = <0x95c10001>;
>      };
> +
> +  - |+
> +
> +    // Case 4: CPUs and CPU idle states described using the hierarchical model.
> +
> +    cpus {
> +      #size-cells = <0>;
> +      #address-cells = <1>;
> +
> +      CPU0: cpu@0 {
> +        device_type = "cpu";
> +        compatible = "arm,cortex-a53", "arm,armv8";
> +        reg = <0x0>;
> +        enable-method = "psci";
> +        power-domains = <&CPU_PD0>;
> +        power-domain-names = "psci";
> +      };
> +
> +      CPU1: cpu@1 {
> +        device_type = "cpu";
> +        compatible = "arm,cortex-a57", "arm,armv8";
> +        reg = <0x100>;
> +        enable-method = "psci";
> +        power-domains = <&CPU_PD1>;
> +        power-domain-names = "psci";
> +      };
> +
> +      idle-states {
> +
> +        CPU_PWRDN: cpu-power-down {
> +          compatible = "arm,idle-state";
> +          arm,psci-suspend-param = <0x0000001>;
> +          entry-latency-us = <10>;
> +          exit-latency-us = <10>;
> +          min-residency-us = <100>;
> +        };
> +
> +        CLUSTER_RET: cluster-retention {
> +          compatible = "domain-idle-state";
> +          arm,psci-suspend-param = <0x1000011>;
> +          entry-latency-us = <500>;
> +          exit-latency-us = <500>;
> +          min-residency-us = <2000>;
> +        };
> +
> +        CLUSTER_PWRDN: cluster-power-down {
> +          compatible = "domain-idle-state";
> +          arm,psci-suspend-param = <0x1000031>;
> +          entry-latency-us = <2000>;
> +          exit-latency-us = <2000>;
> +          min-residency-us = <6000>;
> +        };
> +      };
> +    };
> +
> +    psci {
> +      compatible = "arm,psci-1.0";
> +      method = "smc";
> +
> +      CPU_PD0: cpu-pd0 {
> +        #power-domain-cells = <0>;
> +        domain-idle-states = <&CPU_PWRDN>;
> +        power-domains = <&CLUSTER_PD>;
> +      };
> +
> +      CPU_PD1: cpu-pd1 {
> +        #power-domain-cells = <0>;
> +        domain-idle-states =  <&CPU_PWRDN>;
> +        power-domains = <&CLUSTER_PD>;
> +      };
> +
> +      CLUSTER_PD: cluster-pd {
> +        #power-domain-cells = <0>;
> +        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
> +      };
> +    };
>  ...
> --
> 2.17.1
>
Ulf Hansson Jan. 14, 2020, 5:54 p.m. UTC | #2
On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > CPU topology, by using a hierarchical layout. Primarily this is done by
> > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > states [2].
> >
> > Let's also add an example into the document for the PSCI DT bindings, to
> > clearly show the new hierarchical based layout. The currently supported
> > flattened layout, is already described in the ARM idle states bindings [3],
> > so let's leave that as is.
> >
> > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> >
> > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v5:
> >         - None.
>
> First I'm seeing this as the DT list was not copied. The example has
> problems when running 'make dt_binding_check':
>
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> compatible: Additional items are not allowed ('arm,armv8' was
> unexpected)
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> compatible: Additional items are not allowed ('arm,armv8' was
> unexpected)
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
>
> 'arm,armv8' is only valid for s/w models.

Perhaps you have a different version of the tools than I have (I have
tried both on v.5.5-rc5 and todays linux-next), because I can't
reproduce these errors at my side when running "make
dt_binding_check".

Can you please check again?

>
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> expected
> Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> expected
>
> The last 2 are due to my conversion of the idle-states binding which
> is in my tree now. Probably need to add 'domain-idle-state' as a
> compatible at a minimum. It looks like domain-idle-state.txt is pretty
> much the same as arm/idle-state.txt, so we should perhaps merge them.

Ahh, so maybe *all* of the above problems are caused by conflicts in
the arm-soc tree with changes from your tree!?

In regards to merging files, I am fine by that if that helps.

>
> There's some bigger issues though.
>
> > ---
> >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> >  2 files changed, 119 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > index c23c24ff7575..7a9c3ce2dbef 100644
> > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > @@ -242,6 +242,21 @@ properties:
> >
> >        where voltage is in V, frequency is in MHz.
> >
> > +  power-domains:
> > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > +    description:
> > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > +      PM domain provider (see also ../power_domain.txt).
> > +
> > +  power-domain-names:
> > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > +    description:
> > +      A list of power domain name strings sorted in the same order as the
> > +      power-domains property.
> > +
> > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > +      PM domain provider, must be "psci".
> > +
> >    qcom,saw:
> >      $ref: '/schemas/types.yaml#/definitions/phandle'
> >      description: |
> > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > index 7abdf58b335e..8ef85420b2ab 100644
> > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > @@ -102,6 +102,34 @@ properties:
> >        [1] Kernel documentation - ARM idle states bindings
> >          Documentation/devicetree/bindings/arm/idle-states.txt
> >
> > +  "#power-domain-cells":
>
> This is wrong because you are saying the /psci node should have these
> properties. You need to define the child nodes (at least a pattern you
> can match on) and put these properties there.

Right, good point.

I searched for some similar examples for how to encode this, but
couldn't really find something useful. One more thing, it seems like
this change is also needed for the common power-domain bindings, as
that also specifies parent/childs domains.

Anyway, I would really appreciate if you can suggest something more
detailed for you think this should be done!?

>
> > +    description:
> > +      The number of cells in a PM domain specifier as per binding in [3].
> > +      Must be 0 as to represent a single PM domain.
> > +
> > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > +      arrangement. This often, but not always, maps directly to the processor
> > +      power topology of the system. Individual nodes in a topology have their
> > +      own specific power states and can be better represented hierarchically.
> > +
> > +      For these cases, the definitions of the idle states for the CPUs and the
> > +      CPU topology, must conform to the binding in [3]. The idle states
> > +      themselves must conform to the binding in [4] and must specify the
> > +      arm,psci-suspend-param property.
> > +
> > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > +      helps to implement support for OSI mode and OS implementations may choose
> > +      to mandate it.
> > +
> > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > +
> > +  power-domains:
> > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > +    description:
> > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > +      PM domain provider.
>
> A schema for 'domain-idle-states' property is missing.

Right, let's figure out the best way for how to add that.

>
> >
> >  required:
> >    - compatible
> > @@ -160,4 +188,80 @@ examples:
> >        cpu_on = <0x95c10002>;
> >        cpu_off = <0x95c10001>;
> >      };
> > +
> > +  - |+
> > +
> > +    // Case 4: CPUs and CPU idle states described using the hierarchical model.
> > +
> > +    cpus {
> > +      #size-cells = <0>;
> > +      #address-cells = <1>;
> > +
> > +      CPU0: cpu@0 {
> > +        device_type = "cpu";
> > +        compatible = "arm,cortex-a53", "arm,armv8";
> > +        reg = <0x0>;
> > +        enable-method = "psci";
> > +        power-domains = <&CPU_PD0>;
> > +        power-domain-names = "psci";
> > +      };
> > +
> > +      CPU1: cpu@1 {
> > +        device_type = "cpu";
> > +        compatible = "arm,cortex-a57", "arm,armv8";
> > +        reg = <0x100>;
> > +        enable-method = "psci";
> > +        power-domains = <&CPU_PD1>;
> > +        power-domain-names = "psci";
> > +      };
> > +
> > +      idle-states {
> > +
> > +        CPU_PWRDN: cpu-power-down {
> > +          compatible = "arm,idle-state";
> > +          arm,psci-suspend-param = <0x0000001>;
> > +          entry-latency-us = <10>;
> > +          exit-latency-us = <10>;
> > +          min-residency-us = <100>;
> > +        };
> > +
> > +        CLUSTER_RET: cluster-retention {
> > +          compatible = "domain-idle-state";
> > +          arm,psci-suspend-param = <0x1000011>;
> > +          entry-latency-us = <500>;
> > +          exit-latency-us = <500>;
> > +          min-residency-us = <2000>;
> > +        };
> > +
> > +        CLUSTER_PWRDN: cluster-power-down {
> > +          compatible = "domain-idle-state";
> > +          arm,psci-suspend-param = <0x1000031>;
> > +          entry-latency-us = <2000>;
> > +          exit-latency-us = <2000>;
> > +          min-residency-us = <6000>;
> > +        };
> > +      };
> > +    };
> > +
> > +    psci {
> > +      compatible = "arm,psci-1.0";
> > +      method = "smc";
> > +
> > +      CPU_PD0: cpu-pd0 {
> > +        #power-domain-cells = <0>;
> > +        domain-idle-states = <&CPU_PWRDN>;
> > +        power-domains = <&CLUSTER_PD>;
> > +      };
> > +
> > +      CPU_PD1: cpu-pd1 {
> > +        #power-domain-cells = <0>;
> > +        domain-idle-states =  <&CPU_PWRDN>;
> > +        power-domains = <&CLUSTER_PD>;
> > +      };
> > +
> > +      CLUSTER_PD: cluster-pd {
> > +        #power-domain-cells = <0>;
> > +        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
> > +      };
> > +    };
> >  ...
> > --
> > 2.17.1
> >

Thanks for your feedback!

Kind regards
Uffe
Rob Herring Jan. 16, 2020, 6:19 p.m. UTC | #3
On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > states [2].
> > >
> > > Let's also add an example into the document for the PSCI DT bindings, to
> > > clearly show the new hierarchical based layout. The currently supported
> > > flattened layout, is already described in the ARM idle states bindings [3],
> > > so let's leave that as is.
> > >
> > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > >
> > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >
> > > Changes in v5:
> > >         - None.
> >
> > First I'm seeing this as the DT list was not copied. The example has
> > problems when running 'make dt_binding_check':
> >
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > compatible: Additional items are not allowed ('arm,armv8' was
> > unexpected)
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > compatible: Additional items are not allowed ('arm,armv8' was
> > unexpected)
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> >
> > 'arm,armv8' is only valid for s/w models.
>
> Perhaps you have a different version of the tools than I have (I have
> tried both on v.5.5-rc5 and todays linux-next), because I can't
> reproduce these errors at my side when running "make
> dt_binding_check".
>
> Can you please check again?

Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
other schema) isn't loaded and used for validation. That schema is the
source of this error.

It is failing in my CI job:
https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185

Is dt-schema up to date? Though I can't think of any recent changes
that would impact this. This check has been there a while and I fixed
all the dts files.

Do you see psci.example.dt.yaml getting built?

> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > expected
> > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > expected
> >
> > The last 2 are due to my conversion of the idle-states binding which
> > is in my tree now. Probably need to add 'domain-idle-state' as a
> > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > much the same as arm/idle-state.txt, so we should perhaps merge them.
>
> Ahh, so maybe *all* of the above problems are caused by conflicts in
> the arm-soc tree with changes from your tree!?

Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.

>
> In regards to merging files, I am fine by that if that helps.
>
> >
> > There's some bigger issues though.
> >
> > > ---
> > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > >  2 files changed, 119 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > @@ -242,6 +242,21 @@ properties:
> > >
> > >        where voltage is in V, frequency is in MHz.
> > >
> > > +  power-domains:
> > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > +    description:
> > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > +      PM domain provider (see also ../power_domain.txt).
> > > +
> > > +  power-domain-names:
> > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > +    description:
> > > +      A list of power domain name strings sorted in the same order as the
> > > +      power-domains property.
> > > +
> > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > +      PM domain provider, must be "psci".
> > > +
> > >    qcom,saw:
> > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > >      description: |
> > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > index 7abdf58b335e..8ef85420b2ab 100644
> > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > @@ -102,6 +102,34 @@ properties:
> > >        [1] Kernel documentation - ARM idle states bindings
> > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > >
> > > +  "#power-domain-cells":
> >
> > This is wrong because you are saying the /psci node should have these
> > properties. You need to define the child nodes (at least a pattern you
> > can match on) and put these properties there.
>
> Right, good point.
>
> I searched for some similar examples for how to encode this, but
> couldn't really find something useful.

You need something like:

patternProperties:
  '^(cluster|cpu)-pd[0-9a-f]+$':
    type: object
    properties:
      ... and then the properties in the child nodes

Note that its going to look weird for the 10th PD with 'cpu-pda'. So
maybe add a '-'.

> One more thing, it seems like
> this change is also needed for the common power-domain bindings, as
> that also specifies parent/childs domains.

Normally, we'd have a $ref to power-domain.yaml, but for that to work
here, you'll have to expand the node names ($nodename).

>
> Anyway, I would really appreciate if you can suggest something more
> detailed for you think this should be done!?
>
> >
> > > +    description:
> > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > +      Must be 0 as to represent a single PM domain.
> > > +
> > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > +      arrangement. This often, but not always, maps directly to the processor
> > > +      power topology of the system. Individual nodes in a topology have their
> > > +      own specific power states and can be better represented hierarchically.
> > > +
> > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > +      themselves must conform to the binding in [4] and must specify the
> > > +      arm,psci-suspend-param property.
> > > +
> > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > +      helps to implement support for OSI mode and OS implementations may choose
> > > +      to mandate it.
> > > +
> > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > +
> > > +  power-domains:
> > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > +    description:
> > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > +      PM domain provider.
> >
> > A schema for 'domain-idle-states' property is missing.
>
> Right, let's figure out the best way for how to add that.

If power-domain.yaml is referenced, then don't need anything else
unless you can define the number of phandles (looks like you can't?).

Rob
Ulf Hansson Jan. 17, 2020, 4:41 p.m. UTC | #4
On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > states [2].
> > > >
> > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > clearly show the new hierarchical based layout. The currently supported
> > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > so let's leave that as is.
> > > >
> > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > >
> > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >
> > > > Changes in v5:
> > > >         - None.
> > >
> > > First I'm seeing this as the DT list was not copied. The example has
> > > problems when running 'make dt_binding_check':
> > >
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > compatible: Additional items are not allowed ('arm,armv8' was
> > > unexpected)
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > compatible: Additional items are not allowed ('arm,armv8' was
> > > unexpected)
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > >
> > > 'arm,armv8' is only valid for s/w models.
> >
> > Perhaps you have a different version of the tools than I have (I have
> > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > reproduce these errors at my side when running "make
> > dt_binding_check".
> >
> > Can you please check again?
>
> Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> other schema) isn't loaded and used for validation. That schema is the
> source of this error.

Yes. Aha, that's why then. Perhaps that needs to be clarified
somewhere in the documentation of tool.

Anyway, I used because it was kind of hard to process all the error
output one gets when building all yaml files at once.

>
> It is failing in my CI job:
> https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
>
> Is dt-schema up to date? Though I can't think of any recent changes
> that would impact this. This check has been there a while and I fixed
> all the dts files.
>
> Do you see psci.example.dt.yaml getting built?

Yes, but with using DT_SCHEMA_FILES.

Anyway, now I can re-produced the errors, so then I should be able to
fix them. :-)

>
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > expected
> > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > expected
> > >
> > > The last 2 are due to my conversion of the idle-states binding which
> > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> >
> > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > the arm-soc tree with changes from your tree!?
>
> Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
>
> >
> > In regards to merging files, I am fine by that if that helps.
> >
> > >
> > > There's some bigger issues though.
> > >
> > > > ---
> > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > >  2 files changed, 119 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > @@ -242,6 +242,21 @@ properties:
> > > >
> > > >        where voltage is in V, frequency is in MHz.
> > > >
> > > > +  power-domains:
> > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > +    description:
> > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > +      PM domain provider (see also ../power_domain.txt).
> > > > +
> > > > +  power-domain-names:
> > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > +    description:
> > > > +      A list of power domain name strings sorted in the same order as the
> > > > +      power-domains property.
> > > > +
> > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > +      PM domain provider, must be "psci".
> > > > +
> > > >    qcom,saw:
> > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > >      description: |
> > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > @@ -102,6 +102,34 @@ properties:
> > > >        [1] Kernel documentation - ARM idle states bindings
> > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > >
> > > > +  "#power-domain-cells":
> > >
> > > This is wrong because you are saying the /psci node should have these
> > > properties. You need to define the child nodes (at least a pattern you
> > > can match on) and put these properties there.
> >
> > Right, good point.
> >
> > I searched for some similar examples for how to encode this, but
> > couldn't really find something useful.
>
> You need something like:
>
> patternProperties:
>   '^(cluster|cpu)-pd[0-9a-f]+$':
>     type: object
>     properties:
>       ... and then the properties in the child nodes
>
> Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> maybe add a '-'.
>

Great, I try this! Thanks.

> > One more thing, it seems like
> > this change is also needed for the common power-domain bindings, as
> > that also specifies parent/childs domains.
>
> Normally, we'd have a $ref to power-domain.yaml, but for that to work
> here, you'll have to expand the node names ($nodename).

Not sure I get that, but interpret this as it's not a good idea to use
a $ref to power-domain.yaml. Right?

>
> >
> > Anyway, I would really appreciate if you can suggest something more
> > detailed for you think this should be done!?
> >
> > >
> > > > +    description:
> > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > +      Must be 0 as to represent a single PM domain.
> > > > +
> > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > +      power topology of the system. Individual nodes in a topology have their
> > > > +      own specific power states and can be better represented hierarchically.
> > > > +
> > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > +      themselves must conform to the binding in [4] and must specify the
> > > > +      arm,psci-suspend-param property.
> > > > +
> > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > +      to mandate it.
> > > > +
> > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > +
> > > > +  power-domains:
> > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > +    description:
> > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > +      PM domain provider.
> > >
> > > A schema for 'domain-idle-states' property is missing.
> >
> > Right, let's figure out the best way for how to add that.
>
> If power-domain.yaml is referenced, then don't need anything else
> unless you can define the number of phandles (looks like you can't?).

The number phandles should be one. At least, I think we can start with
that and extend the binding if needed.

How would that look like then? Apologize for my ignorance (I really
need to spend some time learning this better)...

Kind regards
Uffe
Rob Herring Jan. 17, 2020, 5:36 p.m. UTC | #5
On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > states [2].
> > > > >
> > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > so let's leave that as is.
> > > > >
> > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > >
> > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > ---
> > > > >
> > > > > Changes in v5:
> > > > >         - None.
> > > >
> > > > First I'm seeing this as the DT list was not copied. The example has
> > > > problems when running 'make dt_binding_check':
> > > >
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > unexpected)
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > unexpected)
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > >
> > > > 'arm,armv8' is only valid for s/w models.
> > >
> > > Perhaps you have a different version of the tools than I have (I have
> > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > reproduce these errors at my side when running "make
> > > dt_binding_check".
> > >
> > > Can you please check again?
> >
> > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > other schema) isn't loaded and used for validation. That schema is the
> > source of this error.
>
> Yes. Aha, that's why then. Perhaps that needs to be clarified
> somewhere in the documentation of tool.

Patches welcome. :) I'm kind of tired of writing documentation that no
one comments on and and seemingly only sometimes read. </rant> :(

> Anyway, I used because it was kind of hard to process all the error
> output one gets when building all yaml files at once.

dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
primarily useful. dt_binding_check should be error/warning free, but
yes linux-next and rc1/2 are frequently broken.

> > It is failing in my CI job:
> > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> >
> > Is dt-schema up to date? Though I can't think of any recent changes
> > that would impact this. This check has been there a while and I fixed
> > all the dts files.
> >
> > Do you see psci.example.dt.yaml getting built?
>
> Yes, but with using DT_SCHEMA_FILES.
>
> Anyway, now I can re-produced the errors, so then I should be able to
> fix them. :-)
>
> >
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > expected
> > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > expected
> > > >
> > > > The last 2 are due to my conversion of the idle-states binding which
> > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > >
> > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > the arm-soc tree with changes from your tree!?
> >
> > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> >
> > >
> > > In regards to merging files, I am fine by that if that helps.
> > >
> > > >
> > > > There's some bigger issues though.
> > > >
> > > > > ---
> > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > >  2 files changed, 119 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > @@ -242,6 +242,21 @@ properties:
> > > > >
> > > > >        where voltage is in V, frequency is in MHz.
> > > > >
> > > > > +  power-domains:
> > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > +    description:
> > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > +
> > > > > +  power-domain-names:
> > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > +    description:
> > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > +      power-domains property.
> > > > > +
> > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > +      PM domain provider, must be "psci".
> > > > > +
> > > > >    qcom,saw:
> > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > >      description: |
> > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > @@ -102,6 +102,34 @@ properties:
> > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > >
> > > > > +  "#power-domain-cells":
> > > >
> > > > This is wrong because you are saying the /psci node should have these
> > > > properties. You need to define the child nodes (at least a pattern you
> > > > can match on) and put these properties there.
> > >
> > > Right, good point.
> > >
> > > I searched for some similar examples for how to encode this, but
> > > couldn't really find something useful.
> >
> > You need something like:
> >
> > patternProperties:
> >   '^(cluster|cpu)-pd[0-9a-f]+$':
> >     type: object
> >     properties:
> >       ... and then the properties in the child nodes
> >
> > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > maybe add a '-'.
> >
>
> Great, I try this! Thanks.
>
> > > One more thing, it seems like
> > > this change is also needed for the common power-domain bindings, as
> > > that also specifies parent/childs domains.
> >
> > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > here, you'll have to expand the node names ($nodename).
>
> Not sure I get that, but interpret this as it's not a good idea to use
> a $ref to power-domain.yaml. Right?

It means either this binding is odd or power-domain.yaml needs some
more work or both. Ideally, we only have 1 type definition of any
property name.

Probably the easiest thing to do is extend the node name pattern to
something like this:

pattern: "^(power-controller|power-domain)([@\-].*)?$"

And then name your nodes like this:

power-domain-cpu-0
power-domain-cluster

That's more consistent anyways.

> > > Anyway, I would really appreciate if you can suggest something more
> > > detailed for you think this should be done!?
> > >
> > > >
> > > > > +    description:
> > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > +      Must be 0 as to represent a single PM domain.
> > > > > +
> > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > +      own specific power states and can be better represented hierarchically.
> > > > > +
> > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > +      arm,psci-suspend-param property.
> > > > > +
> > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > +      to mandate it.
> > > > > +
> > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > +
> > > > > +  power-domains:
> > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > +    description:
> > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > +      PM domain provider.
> > > >
> > > > A schema for 'domain-idle-states' property is missing.
> > >
> > > Right, let's figure out the best way for how to add that.
> >
> > If power-domain.yaml is referenced, then don't need anything else
> > unless you can define the number of phandles (looks like you can't?).
>
> The number phandles should be one. At least, I think we can start with
> that and extend the binding if needed.

But there's 2 for the cluster in the example.

> How would that look like then? Apologize for my ignorance (I really
> need to spend some time learning this better)...

Basically, anything common that can be an multiple entries is defined as:

power-domains:
  maxItems: 1

if only 1 entry or for multiple you need to define each entry:

power-domains:
  items:
    - description: what the first power domain is
    - description: what the 2nd power domain is

Rob
Ulf Hansson Jan. 20, 2020, 12:56 p.m. UTC | #6
On Fri, 17 Jan 2020 at 18:36, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > > states [2].
> > > > > >
> > > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > > so let's leave that as is.
> > > > > >
> > > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > >
> > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v5:
> > > > > >         - None.
> > > > >
> > > > > First I'm seeing this as the DT list was not copied. The example has
> > > > > problems when running 'make dt_binding_check':
> > > > >
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > unexpected)
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > unexpected)
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > > >
> > > > > 'arm,armv8' is only valid for s/w models.
> > > >
> > > > Perhaps you have a different version of the tools than I have (I have
> > > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > > reproduce these errors at my side when running "make
> > > > dt_binding_check".
> > > >
> > > > Can you please check again?
> > >
> > > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > > other schema) isn't loaded and used for validation. That schema is the
> > > source of this error.
> >
> > Yes. Aha, that's why then. Perhaps that needs to be clarified
> > somewhere in the documentation of tool.
>
> Patches welcome. :) I'm kind of tired of writing documentation that no
> one comments on and and seemingly only sometimes read. </rant> :(

I understand your concerns. A patch is on it's way.

>
> > Anyway, I used because it was kind of hard to process all the error
> > output one gets when building all yaml files at once.
>
> dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
> primarily useful. dt_binding_check should be error/warning free, but
> yes linux-next and rc1/2 are frequently broken.
>
> > > It is failing in my CI job:
> > > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> > >
> > > Is dt-schema up to date? Though I can't think of any recent changes
> > > that would impact this. This check has been there a while and I fixed
> > > all the dts files.
> > >
> > > Do you see psci.example.dt.yaml getting built?
> >
> > Yes, but with using DT_SCHEMA_FILES.
> >
> > Anyway, now I can re-produced the errors, so then I should be able to
> > fix them. :-)
> >
> > >
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > > expected
> > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > > expected
> > > > >
> > > > > The last 2 are due to my conversion of the idle-states binding which
> > > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > > >
> > > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > > the arm-soc tree with changes from your tree!?
> > >
> > > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> > >
> > > >
> > > > In regards to merging files, I am fine by that if that helps.
> > > >
> > > > >
> > > > > There's some bigger issues though.
> > > > >
> > > > > > ---
> > > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > > >  2 files changed, 119 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > @@ -242,6 +242,21 @@ properties:
> > > > > >
> > > > > >        where voltage is in V, frequency is in MHz.
> > > > > >
> > > > > > +  power-domains:
> > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > +    description:
> > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > > +
> > > > > > +  power-domain-names:
> > > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > > +    description:
> > > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > > +      power-domains property.
> > > > > > +
> > > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > > +      PM domain provider, must be "psci".
> > > > > > +
> > > > > >    qcom,saw:
> > > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > > >      description: |
> > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > @@ -102,6 +102,34 @@ properties:
> > > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > >
> > > > > > +  "#power-domain-cells":
> > > > >
> > > > > This is wrong because you are saying the /psci node should have these
> > > > > properties. You need to define the child nodes (at least a pattern you
> > > > > can match on) and put these properties there.
> > > >
> > > > Right, good point.
> > > >
> > > > I searched for some similar examples for how to encode this, but
> > > > couldn't really find something useful.
> > >
> > > You need something like:
> > >
> > > patternProperties:
> > >   '^(cluster|cpu)-pd[0-9a-f]+$':
> > >     type: object
> > >     properties:
> > >       ... and then the properties in the child nodes
> > >
> > > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > > maybe add a '-'.
> > >
> >
> > Great, I try this! Thanks.
> >
> > > > One more thing, it seems like
> > > > this change is also needed for the common power-domain bindings, as
> > > > that also specifies parent/childs domains.
> > >
> > > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > > here, you'll have to expand the node names ($nodename).
> >
> > Not sure I get that, but interpret this as it's not a good idea to use
> > a $ref to power-domain.yaml. Right?
>
> It means either this binding is odd or power-domain.yaml needs some
> more work or both. Ideally, we only have 1 type definition of any
> property name.
>
> Probably the easiest thing to do is extend the node name pattern to
> something like this:
>
> pattern: "^(power-controller|power-domain)([@\-].*)?$"
>
> And then name your nodes like this:
>
> power-domain-cpu-0
> power-domain-cluster
>
> That's more consistent anyways.

Looks like a good idea! I try that.

>
> > > > Anyway, I would really appreciate if you can suggest something more
> > > > detailed for you think this should be done!?
> > > >
> > > > >
> > > > > > +    description:
> > > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > > +      Must be 0 as to represent a single PM domain.
> > > > > > +
> > > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > > +      own specific power states and can be better represented hierarchically.
> > > > > > +
> > > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > > +      arm,psci-suspend-param property.
> > > > > > +
> > > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > > +      to mandate it.
> > > > > > +
> > > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > +
> > > > > > +  power-domains:
> > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > +    description:
> > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > +      PM domain provider.
> > > > >
> > > > > A schema for 'domain-idle-states' property is missing.
> > > >
> > > > Right, let's figure out the best way for how to add that.
> > >
> > > If power-domain.yaml is referenced, then don't need anything else
> > > unless you can define the number of phandles (looks like you can't?).
> >
> > The number phandles should be one. At least, I think we can start with
> > that and extend the binding if needed.
>
> But there's 2 for the cluster in the example.

What example do you refer to?

For each power controller node for psci, only one phandle needs to be
specified in "power-domains", as that should be sufficient to describe
the topology.

When it comes to the CPU node, multiple phandles may need to specified
in "power-domains".

>
> > How would that look like then? Apologize for my ignorance (I really
> > need to spend some time learning this better)...
>
> Basically, anything common that can be an multiple entries is defined as:
>
> power-domains:
>   maxItems: 1
>
> if only 1 entry or for multiple you need to define each entry:
>
> power-domains:
>   items:
>     - description: what the first power domain is
>     - description: what the 2nd power domain is

Okay.

This should be fine for the PSCI power controller node, but for the
generic binding in cpus.yaml I am not sure how to describe this as
that would be SoC specific.

The number of phandles in power-domains could differ between SoCs at
CPU node. However, at most, one would be a phandle for PSCI. If
additional those would be for something different.

Kind regards
Uffe
Rob Herring Jan. 21, 2020, 4:59 p.m. UTC | #7
On Mon, Jan 20, 2020 at 6:57 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 17 Jan 2020 at 18:36, Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > >
> > > > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > > > states [2].
> > > > > > >
> > > > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > > > so let's leave that as is.
> > > > > > >
> > > > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > >
> > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v5:
> > > > > > >         - None.
> > > > > >
> > > > > > First I'm seeing this as the DT list was not copied. The example has
> > > > > > problems when running 'make dt_binding_check':
> > > > > >
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > unexpected)
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > unexpected)
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > > > >
> > > > > > 'arm,armv8' is only valid for s/w models.
> > > > >
> > > > > Perhaps you have a different version of the tools than I have (I have
> > > > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > > > reproduce these errors at my side when running "make
> > > > > dt_binding_check".
> > > > >
> > > > > Can you please check again?
> > > >
> > > > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > > > other schema) isn't loaded and used for validation. That schema is the
> > > > source of this error.
> > >
> > > Yes. Aha, that's why then. Perhaps that needs to be clarified
> > > somewhere in the documentation of tool.
> >
> > Patches welcome. :) I'm kind of tired of writing documentation that no
> > one comments on and and seemingly only sometimes read. </rant> :(
>
> I understand your concerns. A patch is on it's way.
>
> >
> > > Anyway, I used because it was kind of hard to process all the error
> > > output one gets when building all yaml files at once.
> >
> > dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
> > primarily useful. dt_binding_check should be error/warning free, but
> > yes linux-next and rc1/2 are frequently broken.
> >
> > > > It is failing in my CI job:
> > > > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> > > >
> > > > Is dt-schema up to date? Though I can't think of any recent changes
> > > > that would impact this. This check has been there a while and I fixed
> > > > all the dts files.
> > > >
> > > > Do you see psci.example.dt.yaml getting built?
> > >
> > > Yes, but with using DT_SCHEMA_FILES.
> > >
> > > Anyway, now I can re-produced the errors, so then I should be able to
> > > fix them. :-)
> > >
> > > >
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > > > expected
> > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > > > expected
> > > > > >
> > > > > > The last 2 are due to my conversion of the idle-states binding which
> > > > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > > > >
> > > > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > > > the arm-soc tree with changes from your tree!?
> > > >
> > > > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> > > >
> > > > >
> > > > > In regards to merging files, I am fine by that if that helps.
> > > > >
> > > > > >
> > > > > > There's some bigger issues though.
> > > > > >
> > > > > > > ---
> > > > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > > > >  2 files changed, 119 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > @@ -242,6 +242,21 @@ properties:
> > > > > > >
> > > > > > >        where voltage is in V, frequency is in MHz.
> > > > > > >
> > > > > > > +  power-domains:
> > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > +    description:
> > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > > > +
> > > > > > > +  power-domain-names:
> > > > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > > > +    description:
> > > > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > > > +      power-domains property.
> > > > > > > +
> > > > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > > > +      PM domain provider, must be "psci".
> > > > > > > +
> > > > > > >    qcom,saw:
> > > > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > > > >      description: |
> > > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > @@ -102,6 +102,34 @@ properties:
> > > > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > >
> > > > > > > +  "#power-domain-cells":
> > > > > >
> > > > > > This is wrong because you are saying the /psci node should have these
> > > > > > properties. You need to define the child nodes (at least a pattern you
> > > > > > can match on) and put these properties there.
> > > > >
> > > > > Right, good point.
> > > > >
> > > > > I searched for some similar examples for how to encode this, but
> > > > > couldn't really find something useful.
> > > >
> > > > You need something like:
> > > >
> > > > patternProperties:
> > > >   '^(cluster|cpu)-pd[0-9a-f]+$':
> > > >     type: object
> > > >     properties:
> > > >       ... and then the properties in the child nodes
> > > >
> > > > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > > > maybe add a '-'.
> > > >
> > >
> > > Great, I try this! Thanks.
> > >
> > > > > One more thing, it seems like
> > > > > this change is also needed for the common power-domain bindings, as
> > > > > that also specifies parent/childs domains.
> > > >
> > > > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > > > here, you'll have to expand the node names ($nodename).
> > >
> > > Not sure I get that, but interpret this as it's not a good idea to use
> > > a $ref to power-domain.yaml. Right?
> >
> > It means either this binding is odd or power-domain.yaml needs some
> > more work or both. Ideally, we only have 1 type definition of any
> > property name.
> >
> > Probably the easiest thing to do is extend the node name pattern to
> > something like this:
> >
> > pattern: "^(power-controller|power-domain)([@\-].*)?$"
> >
> > And then name your nodes like this:
> >
> > power-domain-cpu-0
> > power-domain-cluster
> >
> > That's more consistent anyways.
>
> Looks like a good idea! I try that.
>
> >
> > > > > Anyway, I would really appreciate if you can suggest something more
> > > > > detailed for you think this should be done!?
> > > > >
> > > > > >
> > > > > > > +    description:
> > > > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > > > +      Must be 0 as to represent a single PM domain.
> > > > > > > +
> > > > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > > > +      own specific power states and can be better represented hierarchically.
> > > > > > > +
> > > > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > > > +      arm,psci-suspend-param property.
> > > > > > > +
> > > > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > > > +      to mandate it.
> > > > > > > +
> > > > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > +
> > > > > > > +  power-domains:
> > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > +    description:
> > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > +      PM domain provider.
> > > > > >
> > > > > > A schema for 'domain-idle-states' property is missing.
> > > > >
> > > > > Right, let's figure out the best way for how to add that.
> > > >
> > > > If power-domain.yaml is referenced, then don't need anything else
> > > > unless you can define the number of phandles (looks like you can't?).
> > >
> > > The number phandles should be one. At least, I think we can start with
> > > that and extend the binding if needed.
> >
> > But there's 2 for the cluster in the example.
>
> What example do you refer to?
>
> For each power controller node for psci, only one phandle needs to be
> specified in "power-domains", as that should be sufficient to describe
> the topology.

I was referring to 'domain-idle-states' in this patch:

+      CLUSTER_PD: cluster-pd {
+        #power-domain-cells = <0>;
+        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
+      };


Rob
Rob Herring Feb. 24, 2020, 9:07 p.m. UTC | #8
On Tue, Jan 21, 2020 at 10:59 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Jan 20, 2020 at 6:57 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 17 Jan 2020 at 18:36, Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > >
> > > > > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > > > > states [2].
> > > > > > > >
> > > > > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > > > > so let's leave that as is.
> > > > > > > >
> > > > > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > > >
> > > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes in v5:
> > > > > > > >         - None.
> > > > > > >
> > > > > > > First I'm seeing this as the DT list was not copied. The example has
> > > > > > > problems when running 'make dt_binding_check':
> > > > > > >
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > > unexpected)
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > > unexpected)
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > > > > >
> > > > > > > 'arm,armv8' is only valid for s/w models.
> > > > > >
> > > > > > Perhaps you have a different version of the tools than I have (I have
> > > > > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > > > > reproduce these errors at my side when running "make
> > > > > > dt_binding_check".
> > > > > >
> > > > > > Can you please check again?
> > > > >
> > > > > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > > > > other schema) isn't loaded and used for validation. That schema is the
> > > > > source of this error.
> > > >
> > > > Yes. Aha, that's why then. Perhaps that needs to be clarified
> > > > somewhere in the documentation of tool.
> > >
> > > Patches welcome. :) I'm kind of tired of writing documentation that no
> > > one comments on and and seemingly only sometimes read. </rant> :(
> >
> > I understand your concerns. A patch is on it's way.
> >
> > >
> > > > Anyway, I used because it was kind of hard to process all the error
> > > > output one gets when building all yaml files at once.
> > >
> > > dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
> > > primarily useful. dt_binding_check should be error/warning free, but
> > > yes linux-next and rc1/2 are frequently broken.
> > >
> > > > > It is failing in my CI job:
> > > > > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> > > > >
> > > > > Is dt-schema up to date? Though I can't think of any recent changes
> > > > > that would impact this. This check has been there a while and I fixed
> > > > > all the dts files.
> > > > >
> > > > > Do you see psci.example.dt.yaml getting built?
> > > >
> > > > Yes, but with using DT_SCHEMA_FILES.
> > > >
> > > > Anyway, now I can re-produced the errors, so then I should be able to
> > > > fix them. :-)
> > > >
> > > > >
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > > > > expected
> > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > > > > expected
> > > > > > >
> > > > > > > The last 2 are due to my conversion of the idle-states binding which
> > > > > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > > > > >
> > > > > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > > > > the arm-soc tree with changes from your tree!?
> > > > >
> > > > > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> > > > >
> > > > > >
> > > > > > In regards to merging files, I am fine by that if that helps.
> > > > > >
> > > > > > >
> > > > > > > There's some bigger issues though.
> > > > > > >
> > > > > > > > ---
> > > > > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > > > > >  2 files changed, 119 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > @@ -242,6 +242,21 @@ properties:
> > > > > > > >
> > > > > > > >        where voltage is in V, frequency is in MHz.
> > > > > > > >
> > > > > > > > +  power-domains:
> > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > > +    description:
> > > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > > > > +
> > > > > > > > +  power-domain-names:
> > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > > > > +    description:
> > > > > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > > > > +      power-domains property.
> > > > > > > > +
> > > > > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > > > > +      PM domain provider, must be "psci".
> > > > > > > > +
> > > > > > > >    qcom,saw:
> > > > > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > > > > >      description: |
> > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > @@ -102,6 +102,34 @@ properties:
> > > > > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > > >
> > > > > > > > +  "#power-domain-cells":
> > > > > > >
> > > > > > > This is wrong because you are saying the /psci node should have these
> > > > > > > properties. You need to define the child nodes (at least a pattern you
> > > > > > > can match on) and put these properties there.
> > > > > >
> > > > > > Right, good point.
> > > > > >
> > > > > > I searched for some similar examples for how to encode this, but
> > > > > > couldn't really find something useful.
> > > > >
> > > > > You need something like:
> > > > >
> > > > > patternProperties:
> > > > >   '^(cluster|cpu)-pd[0-9a-f]+$':
> > > > >     type: object
> > > > >     properties:
> > > > >       ... and then the properties in the child nodes
> > > > >
> > > > > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > > > > maybe add a '-'.
> > > > >
> > > >
> > > > Great, I try this! Thanks.
> > > >
> > > > > > One more thing, it seems like
> > > > > > this change is also needed for the common power-domain bindings, as
> > > > > > that also specifies parent/childs domains.
> > > > >
> > > > > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > > > > here, you'll have to expand the node names ($nodename).
> > > >
> > > > Not sure I get that, but interpret this as it's not a good idea to use
> > > > a $ref to power-domain.yaml. Right?
> > >
> > > It means either this binding is odd or power-domain.yaml needs some
> > > more work or both. Ideally, we only have 1 type definition of any
> > > property name.
> > >
> > > Probably the easiest thing to do is extend the node name pattern to
> > > something like this:
> > >
> > > pattern: "^(power-controller|power-domain)([@\-].*)?$"
> > >
> > > And then name your nodes like this:
> > >
> > > power-domain-cpu-0
> > > power-domain-cluster
> > >
> > > That's more consistent anyways.
> >
> > Looks like a good idea! I try that.
> >
> > >
> > > > > > Anyway, I would really appreciate if you can suggest something more
> > > > > > detailed for you think this should be done!?
> > > > > >
> > > > > > >
> > > > > > > > +    description:
> > > > > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > > > > +      Must be 0 as to represent a single PM domain.
> > > > > > > > +
> > > > > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > > > > +      own specific power states and can be better represented hierarchically.
> > > > > > > > +
> > > > > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > > > > +      arm,psci-suspend-param property.
> > > > > > > > +
> > > > > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > > > > +      to mandate it.
> > > > > > > > +
> > > > > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > > +
> > > > > > > > +  power-domains:
> > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > > +    description:
> > > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > > +      PM domain provider.
> > > > > > >
> > > > > > > A schema for 'domain-idle-states' property is missing.
> > > > > >
> > > > > > Right, let's figure out the best way for how to add that.
> > > > >
> > > > > If power-domain.yaml is referenced, then don't need anything else
> > > > > unless you can define the number of phandles (looks like you can't?).
> > > >
> > > > The number phandles should be one. At least, I think we can start with
> > > > that and extend the binding if needed.
> > >
> > > But there's 2 for the cluster in the example.
> >
> > What example do you refer to?
> >
> > For each power controller node for psci, only one phandle needs to be
> > specified in "power-domains", as that should be sufficient to describe
> > the topology.
>
> I was referring to 'domain-idle-states' in this patch:
>
> +      CLUSTER_PD: cluster-pd {
> +        #power-domain-cells = <0>;
> +        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
> +      };

Going to send a patch for all this? I'd like to not have warnings in v5.6.

Rob
Ulf Hansson Feb. 25, 2020, 8:49 a.m. UTC | #9
On Mon, 24 Feb 2020 at 22:07, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Jan 21, 2020 at 10:59 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Jan 20, 2020 at 6:57 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Fri, 17 Jan 2020 at 18:36, Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Fri, Jan 17, 2020 at 10:42 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Thu, 16 Jan 2020 at 19:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Tue, Jan 14, 2020 at 11:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > >
> > > > > > > On Mon, 13 Jan 2020 at 20:53, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Dec 30, 2019 at 8:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Update PSCI DT bindings to allow to represent idle states for CPUs and the
> > > > > > > > > CPU topology, by using a hierarchical layout. Primarily this is done by
> > > > > > > > > re-using the existing DT bindings for PM domains [1] and for PM domain idle
> > > > > > > > > states [2].
> > > > > > > > >
> > > > > > > > > Let's also add an example into the document for the PSCI DT bindings, to
> > > > > > > > > clearly show the new hierarchical based layout. The currently supported
> > > > > > > > > flattened layout, is already described in the ARM idle states bindings [3],
> > > > > > > > > so let's leave that as is.
> > > > > > > > >
> > > > > > > > > [1] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > > > [2] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > > > [3] Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > > > >
> > > > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > > > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes in v5:
> > > > > > > > >         - None.
> > > > > > > >
> > > > > > > > First I'm seeing this as the DT list was not copied. The example has
> > > > > > > > problems when running 'make dt_binding_check':
> > > > > > > >
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > > > unexpected)
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@0:
> > > > > > > > compatible: ['arm,cortex-a53', 'arm,armv8'] is too long
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > > > compatible: Additional items are not allowed ('arm,armv8' was
> > > > > > > > unexpected)
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml: cpu@1:
> > > > > > > > compatible: ['arm,cortex-a57', 'arm,armv8'] is too long
> > > > > > > >
> > > > > > > > 'arm,armv8' is only valid for s/w models.
> > > > > > >
> > > > > > > Perhaps you have a different version of the tools than I have (I have
> > > > > > > tried both on v.5.5-rc5 and todays linux-next), because I can't
> > > > > > > reproduce these errors at my side when running "make
> > > > > > > dt_binding_check".
> > > > > > >
> > > > > > > Can you please check again?
> > > > > >
> > > > > > Are you setting DT_SCHEMA_FILES? If so, then arm/cpus.yaml (or any
> > > > > > other schema) isn't loaded and used for validation. That schema is the
> > > > > > source of this error.
> > > > >
> > > > > Yes. Aha, that's why then. Perhaps that needs to be clarified
> > > > > somewhere in the documentation of tool.
> > > >
> > > > Patches welcome. :) I'm kind of tired of writing documentation that no
> > > > one comments on and and seemingly only sometimes read. </rant> :(
> > >
> > > I understand your concerns. A patch is on it's way.
> > >
> > > >
> > > > > Anyway, I used because it was kind of hard to process all the error
> > > > > output one gets when building all yaml files at once.
> > > >
> > > > dtbs_check has a lot which is where setting DT_SCHEMA_FILES is
> > > > primarily useful. dt_binding_check should be error/warning free, but
> > > > yes linux-next and rc1/2 are frequently broken.
> > > >
> > > > > > It is failing in my CI job:
> > > > > > https://gitlab.com/robherring/linux-dt-bindings/-/jobs/405298185
> > > > > >
> > > > > > Is dt-schema up to date? Though I can't think of any recent changes
> > > > > > that would impact this. This check has been there a while and I fixed
> > > > > > all the dts files.
> > > > > >
> > > > > > Do you see psci.example.dt.yaml getting built?
> > > > >
> > > > > Yes, but with using DT_SCHEMA_FILES.
> > > > >
> > > > > Anyway, now I can re-produced the errors, so then I should be able to
> > > > > fix them. :-)
> > > > >
> > > > > >
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > > > idle-states: cluster-retention:compatible:0: 'arm,idle-state' was
> > > > > > > > expected
> > > > > > > > Documentation/devicetree/bindings/arm/psci.example.dt.yaml:
> > > > > > > > idle-states: cluster-power-down:compatible:0: 'arm,idle-state' was
> > > > > > > > expected
> > > > > > > >
> > > > > > > > The last 2 are due to my conversion of the idle-states binding which
> > > > > > > > is in my tree now. Probably need to add 'domain-idle-state' as a
> > > > > > > > compatible at a minimum. It looks like domain-idle-state.txt is pretty
> > > > > > > > much the same as arm/idle-state.txt, so we should perhaps merge them.
> > > > > > >
> > > > > > > Ahh, so maybe *all* of the above problems are caused by conflicts in
> > > > > > > the arm-soc tree with changes from your tree!?
> > > > > >
> > > > > > Shouldn't be. arm/cpus.yaml has been in place for a few cycles now.
> > > > > >
> > > > > > >
> > > > > > > In regards to merging files, I am fine by that if that helps.
> > > > > > >
> > > > > > > >
> > > > > > > > There's some bigger issues though.
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >  .../devicetree/bindings/arm/cpus.yaml         |  15 +++
> > > > > > > > >  .../devicetree/bindings/arm/psci.yaml         | 104 ++++++++++++++++++
> > > > > > > > >  2 files changed, 119 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > > index c23c24ff7575..7a9c3ce2dbef 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> > > > > > > > > @@ -242,6 +242,21 @@ properties:
> > > > > > > > >
> > > > > > > > >        where voltage is in V, frequency is in MHz.
> > > > > > > > >
> > > > > > > > > +  power-domains:
> > > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > > > +    description:
> > > > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > > > +      PM domain provider (see also ../power_domain.txt).
> > > > > > > > > +
> > > > > > > > > +  power-domain-names:
> > > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/string-array'
> > > > > > > > > +    description:
> > > > > > > > > +      A list of power domain name strings sorted in the same order as the
> > > > > > > > > +      power-domains property.
> > > > > > > > > +
> > > > > > > > > +      For PSCI based platforms, the name corresponding to the index of the PSCI
> > > > > > > > > +      PM domain provider, must be "psci".
> > > > > > > > > +
> > > > > > > > >    qcom,saw:
> > > > > > > > >      $ref: '/schemas/types.yaml#/definitions/phandle'
> > > > > > > > >      description: |
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > > index 7abdf58b335e..8ef85420b2ab 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > > > > > > > > @@ -102,6 +102,34 @@ properties:
> > > > > > > > >        [1] Kernel documentation - ARM idle states bindings
> > > > > > > > >          Documentation/devicetree/bindings/arm/idle-states.txt
> > > > > > > > >
> > > > > > > > > +  "#power-domain-cells":
> > > > > > > >
> > > > > > > > This is wrong because you are saying the /psci node should have these
> > > > > > > > properties. You need to define the child nodes (at least a pattern you
> > > > > > > > can match on) and put these properties there.
> > > > > > >
> > > > > > > Right, good point.
> > > > > > >
> > > > > > > I searched for some similar examples for how to encode this, but
> > > > > > > couldn't really find something useful.
> > > > > >
> > > > > > You need something like:
> > > > > >
> > > > > > patternProperties:
> > > > > >   '^(cluster|cpu)-pd[0-9a-f]+$':
> > > > > >     type: object
> > > > > >     properties:
> > > > > >       ... and then the properties in the child nodes
> > > > > >
> > > > > > Note that its going to look weird for the 10th PD with 'cpu-pda'. So
> > > > > > maybe add a '-'.
> > > > > >
> > > > >
> > > > > Great, I try this! Thanks.
> > > > >
> > > > > > > One more thing, it seems like
> > > > > > > this change is also needed for the common power-domain bindings, as
> > > > > > > that also specifies parent/childs domains.
> > > > > >
> > > > > > Normally, we'd have a $ref to power-domain.yaml, but for that to work
> > > > > > here, you'll have to expand the node names ($nodename).
> > > > >
> > > > > Not sure I get that, but interpret this as it's not a good idea to use
> > > > > a $ref to power-domain.yaml. Right?
> > > >
> > > > It means either this binding is odd or power-domain.yaml needs some
> > > > more work or both. Ideally, we only have 1 type definition of any
> > > > property name.
> > > >
> > > > Probably the easiest thing to do is extend the node name pattern to
> > > > something like this:
> > > >
> > > > pattern: "^(power-controller|power-domain)([@\-].*)?$"
> > > >
> > > > And then name your nodes like this:
> > > >
> > > > power-domain-cpu-0
> > > > power-domain-cluster
> > > >
> > > > That's more consistent anyways.
> > >
> > > Looks like a good idea! I try that.
> > >
> > > >
> > > > > > > Anyway, I would really appreciate if you can suggest something more
> > > > > > > detailed for you think this should be done!?
> > > > > > >
> > > > > > > >
> > > > > > > > > +    description:
> > > > > > > > > +      The number of cells in a PM domain specifier as per binding in [3].
> > > > > > > > > +      Must be 0 as to represent a single PM domain.
> > > > > > > > > +
> > > > > > > > > +      ARM systems can have multiple cores, sometimes in an hierarchical
> > > > > > > > > +      arrangement. This often, but not always, maps directly to the processor
> > > > > > > > > +      power topology of the system. Individual nodes in a topology have their
> > > > > > > > > +      own specific power states and can be better represented hierarchically.
> > > > > > > > > +
> > > > > > > > > +      For these cases, the definitions of the idle states for the CPUs and the
> > > > > > > > > +      CPU topology, must conform to the binding in [3]. The idle states
> > > > > > > > > +      themselves must conform to the binding in [4] and must specify the
> > > > > > > > > +      arm,psci-suspend-param property.
> > > > > > > > > +
> > > > > > > > > +      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
> > > > > > > > > +      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
> > > > > > > > > +      helps to implement support for OSI mode and OS implementations may choose
> > > > > > > > > +      to mandate it.
> > > > > > > > > +
> > > > > > > > > +      [3] Documentation/devicetree/bindings/power/power_domain.txt
> > > > > > > > > +      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > > > > > > > +
> > > > > > > > > +  power-domains:
> > > > > > > > > +    $ref: '/schemas/types.yaml#/definitions/phandle-array'
> > > > > > > > > +    description:
> > > > > > > > > +      List of phandles and PM domain specifiers, as defined by bindings of the
> > > > > > > > > +      PM domain provider.
> > > > > > > >
> > > > > > > > A schema for 'domain-idle-states' property is missing.
> > > > > > >
> > > > > > > Right, let's figure out the best way for how to add that.
> > > > > >
> > > > > > If power-domain.yaml is referenced, then don't need anything else
> > > > > > unless you can define the number of phandles (looks like you can't?).
> > > > >
> > > > > The number phandles should be one. At least, I think we can start with
> > > > > that and extend the binding if needed.
> > > >
> > > > But there's 2 for the cluster in the example.
> > >
> > > What example do you refer to?
> > >
> > > For each power controller node for psci, only one phandle needs to be
> > > specified in "power-domains", as that should be sufficient to describe
> > > the topology.
> >
> > I was referring to 'domain-idle-states' in this patch:
> >
> > +      CLUSTER_PD: cluster-pd {
> > +        #power-domain-cells = <0>;
> > +        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
> > +      };
>
> Going to send a patch for all this? I'd like to not have warnings in v5.6.

Yes, it's in the pipe, apologize for the delay. I have been chasing
regressions for MMC that broke boot for some boards.

I am very soon switching back to this.

Kind regards
Uffe
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
index c23c24ff7575..7a9c3ce2dbef 100644
--- a/Documentation/devicetree/bindings/arm/cpus.yaml
+++ b/Documentation/devicetree/bindings/arm/cpus.yaml
@@ -242,6 +242,21 @@  properties:
 
       where voltage is in V, frequency is in MHz.
 
+  power-domains:
+    $ref: '/schemas/types.yaml#/definitions/phandle-array'
+    description:
+      List of phandles and PM domain specifiers, as defined by bindings of the
+      PM domain provider (see also ../power_domain.txt).
+
+  power-domain-names:
+    $ref: '/schemas/types.yaml#/definitions/string-array'
+    description:
+      A list of power domain name strings sorted in the same order as the
+      power-domains property.
+
+      For PSCI based platforms, the name corresponding to the index of the PSCI
+      PM domain provider, must be "psci".
+
   qcom,saw:
     $ref: '/schemas/types.yaml#/definitions/phandle'
     description: |
diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
index 7abdf58b335e..8ef85420b2ab 100644
--- a/Documentation/devicetree/bindings/arm/psci.yaml
+++ b/Documentation/devicetree/bindings/arm/psci.yaml
@@ -102,6 +102,34 @@  properties:
       [1] Kernel documentation - ARM idle states bindings
         Documentation/devicetree/bindings/arm/idle-states.txt
 
+  "#power-domain-cells":
+    description:
+      The number of cells in a PM domain specifier as per binding in [3].
+      Must be 0 as to represent a single PM domain.
+
+      ARM systems can have multiple cores, sometimes in an hierarchical
+      arrangement. This often, but not always, maps directly to the processor
+      power topology of the system. Individual nodes in a topology have their
+      own specific power states and can be better represented hierarchically.
+
+      For these cases, the definitions of the idle states for the CPUs and the
+      CPU topology, must conform to the binding in [3]. The idle states
+      themselves must conform to the binding in [4] and must specify the
+      arm,psci-suspend-param property.
+
+      It should also be noted that, in PSCI firmware v1.0 the OS-Initiated
+      (OSI) CPU suspend mode is introduced. Using a hierarchical representation
+      helps to implement support for OSI mode and OS implementations may choose
+      to mandate it.
+
+      [3] Documentation/devicetree/bindings/power/power_domain.txt
+      [4] Documentation/devicetree/bindings/power/domain-idle-state.txt
+
+  power-domains:
+    $ref: '/schemas/types.yaml#/definitions/phandle-array'
+    description:
+      List of phandles and PM domain specifiers, as defined by bindings of the
+      PM domain provider.
 
 required:
   - compatible
@@ -160,4 +188,80 @@  examples:
       cpu_on = <0x95c10002>;
       cpu_off = <0x95c10001>;
     };
+
+  - |+
+
+    // Case 4: CPUs and CPU idle states described using the hierarchical model.
+
+    cpus {
+      #size-cells = <0>;
+      #address-cells = <1>;
+
+      CPU0: cpu@0 {
+        device_type = "cpu";
+        compatible = "arm,cortex-a53", "arm,armv8";
+        reg = <0x0>;
+        enable-method = "psci";
+        power-domains = <&CPU_PD0>;
+        power-domain-names = "psci";
+      };
+
+      CPU1: cpu@1 {
+        device_type = "cpu";
+        compatible = "arm,cortex-a57", "arm,armv8";
+        reg = <0x100>;
+        enable-method = "psci";
+        power-domains = <&CPU_PD1>;
+        power-domain-names = "psci";
+      };
+
+      idle-states {
+
+        CPU_PWRDN: cpu-power-down {
+          compatible = "arm,idle-state";
+          arm,psci-suspend-param = <0x0000001>;
+          entry-latency-us = <10>;
+          exit-latency-us = <10>;
+          min-residency-us = <100>;
+        };
+
+        CLUSTER_RET: cluster-retention {
+          compatible = "domain-idle-state";
+          arm,psci-suspend-param = <0x1000011>;
+          entry-latency-us = <500>;
+          exit-latency-us = <500>;
+          min-residency-us = <2000>;
+        };
+
+        CLUSTER_PWRDN: cluster-power-down {
+          compatible = "domain-idle-state";
+          arm,psci-suspend-param = <0x1000031>;
+          entry-latency-us = <2000>;
+          exit-latency-us = <2000>;
+          min-residency-us = <6000>;
+        };
+      };
+    };
+
+    psci {
+      compatible = "arm,psci-1.0";
+      method = "smc";
+
+      CPU_PD0: cpu-pd0 {
+        #power-domain-cells = <0>;
+        domain-idle-states = <&CPU_PWRDN>;
+        power-domains = <&CLUSTER_PD>;
+      };
+
+      CPU_PD1: cpu-pd1 {
+        #power-domain-cells = <0>;
+        domain-idle-states =  <&CPU_PWRDN>;
+        power-domains = <&CLUSTER_PD>;
+      };
+
+      CLUSTER_PD: cluster-pd {
+        #power-domain-cells = <0>;
+        domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
+      };
+    };
 ...