diff mbox series

[2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

Message ID 20211005155923.173399-3-marcan@marcan.st
State Changes Requested, archived
Headers show
Series Apple SoC PMGR device power states driver | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Hector Martin Oct. 5, 2021, 3:59 p.m. UTC
This syscon child node represents a single SoC device controlled by the
PMGR block. This layout allows us to declare all device power state
controls (power/clock gating and reset) in the device tree, including
dependencies, instead of hardcoding it into the driver. The register
layout is uniform.

Each pmgr-pwrstate node provides genpd and reset features, to be
consumed by downstream device nodes.

Future SoCs are expected to use backwards compatible registers, and the
"apple,pmgr-pwrstate" represents any such interfaces (possibly with
additional features gated by the more specific compatible), allowing
them to be bound without driver updates. If a backwards incompatible
change is introduced in future SoCs, it will require a new compatible,
such as "apple,pmgr-pwrstate-v2".

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml

Comments

Mark Kettenis Oct. 5, 2021, 8:16 p.m. UTC | #1
> From: Hector Martin <marcan@marcan.st>
> Date: Wed,  6 Oct 2021 00:59:18 +0900
> 
> This syscon child node represents a single SoC device controlled by the
> PMGR block. This layout allows us to declare all device power state
> controls (power/clock gating and reset) in the device tree, including
> dependencies, instead of hardcoding it into the driver. The register
> layout is uniform.
> 
> Each pmgr-pwrstate node provides genpd and reset features, to be
> consumed by downstream device nodes.
> 
> Future SoCs are expected to use backwards compatible registers, and the
> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
> additional features gated by the more specific compatible), allowing
> them to be bound without driver updates. If a backwards incompatible
> change is introduced in future SoCs, it will require a new compatible,
> such as "apple,pmgr-pwrstate-v2".

Or we drop the apple,mpgr-pwrstate and go with only SoC-specific
compatibles from that point onwards.

> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml

This works for U-Boot.  Didn't write an OpenBSD driver yet but it
should work there as well.

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>


> diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> new file mode 100644
> index 000000000000..a14bf5f30ff0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/apple,pmgr-pwrstate.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC PMGR Power States
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +allOf:
> +  - $ref: "power-domain.yaml#"
> +
> +description: |
> +  Apple SoCs include a PMGR block responsible for power management,
> +  which can control various clocks, resets, power states, and
> +  performance features. This binding describes the device power
> +  state registers, which control power states and resets.
> +
> +  Each instance of a power controller within the PMGR syscon node
> +  represents a generic power domain provider, as documented in
> +  Documentation/devicetree/bindings/power/power-domain.yaml.
> +  The provider controls a single SoC block. The power hierarchy is
> +  represented via power-domains relationships between these nodes.
> +
> +  See Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> +  for the top-level PMGR node documentation.
> +
> +  IP cores belonging to a power domain should contain a
> +  "power-domains" property that is a phandle for the
> +  power domain node representing the domain.
> +
> +properties:
> +  $nodename:
> +    pattern: "^power-controller@[0-9a-f]+$"
> +
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-pmgr-pwrstate
> +      - const: apple,pmgr-pwrstate
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 0
> +
> +  "#reset-cells":
> +    const: 0
> +
> +  power-domains:
> +    description:
> +      Reference to parent power domains. A domain may have multiple parents,
> +      and all will be powered up when it is powered.
> +
> +  apple,domain-name:
> +    description: |
> +      Specifies the name of the SoC device being controlled. This is used to
> +      name the power/reset domains.
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  apple,always-on:
> +    description: |
> +      Forces this power domain to always be powered up.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#power-domain-cells"
> +  - "#reset-cells"
> +  - "apple,domain-name"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        power-management@23b700000 {
> +            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x2 0x3b700000 0x0 0x14000>;
> +
> +            ps_sio: power-controller@1c0 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x1c0>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "sio";
> +                apple,always-on;
> +            };
> +
> +            ps_uart_p: power-controller@220 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x220>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "uart_p";
> +                power-domains = <&ps_sio>;
> +            };
> +
> +            ps_uart0: power-controller@270 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x270>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "uart0";
> +                power-domains = <&ps_uart_p>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d25598842d15..5fe53d9a2956 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1722,6 +1722,7 @@ F:	Documentation/devicetree/bindings/arm/apple.yaml
>  F:	Documentation/devicetree/bindings/arm/apple/*
>  F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
>  F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> +F:	Documentation/devicetree/bindings/power/apple*
>  F:	arch/arm64/boot/dts/apple/
>  F:	drivers/irqchip/irq-apple-aic.c
>  F:	include/dt-bindings/interrupt-controller/apple-aic.h
> -- 
> 2.33.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Rob Herring Oct. 6, 2021, 12:58 a.m. UTC | #2
On Tue, Oct 5, 2021 at 10:59 AM Hector Martin <marcan@marcan.st> wrote:
>
> This syscon child node represents a single SoC device controlled by the
> PMGR block. This layout allows us to declare all device power state
> controls (power/clock gating and reset) in the device tree, including
> dependencies, instead of hardcoding it into the driver. The register
> layout is uniform.
>
> Each pmgr-pwrstate node provides genpd and reset features, to be
> consumed by downstream device nodes.
>
> Future SoCs are expected to use backwards compatible registers, and the
> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
> additional features gated by the more specific compatible), allowing
> them to be bound without driver updates. If a backwards incompatible
> change is introduced in future SoCs, it will require a new compatible,
> such as "apple,pmgr-pwrstate-v2".

Is that because past SoCs used the same registers? I don't see how
else you have any insight to what future SoCs will do.

Normally we don't do 1 node per register type bindings, so I'm a bit
leery about doing 1 node per domain.

>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
>
> diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> new file mode 100644
> index 000000000000..a14bf5f30ff0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/apple,pmgr-pwrstate.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC PMGR Power States
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +allOf:
> +  - $ref: "power-domain.yaml#"
> +
> +description: |
> +  Apple SoCs include a PMGR block responsible for power management,
> +  which can control various clocks, resets, power states, and
> +  performance features. This binding describes the device power
> +  state registers, which control power states and resets.
> +
> +  Each instance of a power controller within the PMGR syscon node
> +  represents a generic power domain provider, as documented in
> +  Documentation/devicetree/bindings/power/power-domain.yaml.
> +  The provider controls a single SoC block. The power hierarchy is
> +  represented via power-domains relationships between these nodes.
> +
> +  See Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> +  for the top-level PMGR node documentation.
> +
> +  IP cores belonging to a power domain should contain a
> +  "power-domains" property that is a phandle for the
> +  power domain node representing the domain.
> +
> +properties:
> +  $nodename:
> +    pattern: "^power-controller@[0-9a-f]+$"

Drop this and define this node in the syscon schema with a $ref to this schema.

> +
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-pmgr-pwrstate
> +      - const: apple,pmgr-pwrstate
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 0
> +
> +  "#reset-cells":
> +    const: 0
> +
> +  power-domains:
> +    description:
> +      Reference to parent power domains. A domain may have multiple parents,
> +      and all will be powered up when it is powered.
> +
> +  apple,domain-name:
> +    description: |
> +      Specifies the name of the SoC device being controlled. This is used to
> +      name the power/reset domains.
> +    $ref: /schemas/types.yaml#/definitions/string

No other power domain binding needs this, why do you?

> +
> +  apple,always-on:
> +    description: |
> +      Forces this power domain to always be powered up.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#power-domain-cells"
> +  - "#reset-cells"
> +  - "apple,domain-name"
> +
> +additionalProperties: false
> +
> +examples:

I prefer 1 complete example in the MFD schema rather than piecemeal examples.

> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        power-management@23b700000 {
> +            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
> +            #address-cells = <1>;
> +            #size-cells = <0>;

As the child nodes are memory mapped devices, size should be 1. Then
address translation works (though Linux doesn't care (currently)).

> +            reg = <0x2 0x3b700000 0x0 0x14000>;
> +
> +            ps_sio: power-controller@1c0 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x1c0>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "sio";
> +                apple,always-on;
> +            };
> +
> +            ps_uart_p: power-controller@220 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x220>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "uart_p";
> +                power-domains = <&ps_sio>;
> +            };
> +
> +            ps_uart0: power-controller@270 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x270>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "uart0";
> +                power-domains = <&ps_uart_p>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d25598842d15..5fe53d9a2956 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1722,6 +1722,7 @@ F:        Documentation/devicetree/bindings/arm/apple.yaml
>  F:     Documentation/devicetree/bindings/arm/apple/*
>  F:     Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
>  F:     Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> +F:     Documentation/devicetree/bindings/power/apple*
>  F:     arch/arm64/boot/dts/apple/
>  F:     drivers/irqchip/irq-apple-aic.c
>  F:     include/dt-bindings/interrupt-controller/apple-aic.h
> --
> 2.33.0
>
Krzysztof Kozlowski Oct. 6, 2021, 7:05 a.m. UTC | #3
On 05/10/2021 17:59, Hector Martin wrote:
> This syscon child node represents a single SoC device controlled by the
> PMGR block. This layout allows us to declare all device power state
> controls (power/clock gating and reset) in the device tree, including
> dependencies, instead of hardcoding it into the driver. The register
> layout is uniform.
> 
> Each pmgr-pwrstate node provides genpd and reset features, to be
> consumed by downstream device nodes.
> 
> Future SoCs are expected to use backwards compatible registers, and the
> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
> additional features gated by the more specific compatible), allowing
> them to be bound without driver updates. If a backwards incompatible
> change is introduced in future SoCs, it will require a new compatible,
> such as "apple,pmgr-pwrstate-v2".
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> new file mode 100644
> index 000000000000..a14bf5f30ff0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/apple,pmgr-pwrstate.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC PMGR Power States
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +allOf:
> +  - $ref: "power-domain.yaml#"
> +
> +description: |
> +  Apple SoCs include a PMGR block responsible for power management,
> +  which can control various clocks, resets, power states, and
> +  performance features. This binding describes the device power
> +  state registers, which control power states and resets.
> +
> +  Each instance of a power controller within the PMGR syscon node
> +  represents a generic power domain provider, as documented in
> +  Documentation/devicetree/bindings/power/power-domain.yaml.
> +  The provider controls a single SoC block. The power hierarchy is
> +  represented via power-domains relationships between these nodes.
> +
> +  See Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> +  for the top-level PMGR node documentation.
> +
> +  IP cores belonging to a power domain should contain a
> +  "power-domains" property that is a phandle for the
> +  power domain node representing the domain.

Skip this last paragraph - it is obvious in usage of power domains.
Specific bindings should not duplicate generic knowledge.

> +
> +properties:
> +  $nodename:
> +    pattern: "^power-controller@[0-9a-f]+$"

Usually we call nodes as power-domain.

> +
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-pmgr-pwrstate
> +      - const: apple,pmgr-pwrstate
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 0
> +
> +  "#reset-cells":
> +    const: 0
> +
> +  power-domains:
> +    description:
> +      Reference to parent power domains. A domain may have multiple parents,
> +      and all will be powered up when it is powered.

How many items?

> +
> +  apple,domain-name:

Use existing binding "label".

> +    description: |
> +      Specifies the name of the SoC device being controlled. This is used to
> +      name the power/reset domains.
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  apple,always-on:
> +    description: |
> +      Forces this power domain to always be powered up.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#power-domain-cells"
> +  - "#reset-cells"
> +  - "apple,domain-name"
> +
> +additionalProperties: false

Your parent schema should include this one for evaluating children.



Best regards,
Krzysztof
Hector Martin Oct. 6, 2021, 3:27 p.m. UTC | #4
On 06/10/2021 05.16, Mark Kettenis wrote:
> Or we drop the apple,mpgr-pwrstate and go with only SoC-specific
> compatibles from that point onwards.

I think if Apple has discrete compat breaks and several SoCs still share 
compatibility, it'd make sense to encode those as pmgr-pwrstate-v2, v3, 
etc. That way compatible SoCs can continue to benefit from 
forwards-compatible kernels, and we only end up with a hard dep on a new 
kernel for incompatible SoCs.
Hector Martin Oct. 6, 2021, 3:52 p.m. UTC | #5
On 06/10/2021 09.58, Rob Herring wrote:
> On Tue, Oct 5, 2021 at 10:59 AM Hector Martin <marcan@marcan.st> wrote:
>> Future SoCs are expected to use backwards compatible registers, and the
>> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
>> additional features gated by the more specific compatible), allowing
>> them to be bound without driver updates. If a backwards incompatible
>> change is introduced in future SoCs, it will require a new compatible,
>> such as "apple,pmgr-pwrstate-v2".
> 
> Is that because past SoCs used the same registers? I don't see how
> else you have any insight to what future SoCs will do.
> 
> Normally we don't do 1 node per register type bindings, so I'm a bit
> leery about doing 1 node per domain.

Yes, we can trace a pretty clear lineage from past SoCs. Plus Apple 
folks themselves have confirmed that this is an explicit policy:

https://twitter.com/stuntpants/status/1442276493669724160

Already within this SoC we have two PMGR instances with different 
register sets (one covers all devices that stay on during system sleep), 
although I am only instantiating one in our devicetree at the moment. 
And of course there is the A14, which is the same generation as the M1 
and has exactly the same register format, but a different set of domains.

Having sub-nodes describing individual power domains has some prior art 
(e.g. power/rockchip,power-controller.yaml). In that case the nodes are 
all managed by the parent node, and use the hierarchical format, but the 
hierarchical format cannot handle multi-parent nodes (which we do have, 
or at least Apple describes them that way). Since we really have no 
"top-level" implementation specifics to worry about, I think it makes 
sense to just bind to single nodes at this point, which makes the driver 
very simple since it doesn't have to perform any bookkeeping for 
multiple domains.

I realize this is all kind of "not the way things are usually done", but 
I don't want to pass up on the opportunity to have one driver last us 
multiple SoCs if we have the chance, and it's looking like it should :-)

Note that as new features are implemented (e.g. auto-PM, which I will 
add to this driver later), that also naturally lends itself to 
forwards-compat, as SoCs without those features at all simply wouldn't 
request them in the DT. In this case an "apple,auto-pm" flag would 
enable that for domains where we want it, and those that don't support 
it (or a hypothetical past SoC without the feature at all) would simply 
not use it.

>> +properties:
>> +  $nodename:
>> +    pattern: "^power-controller@[0-9a-f]+$"
> 
> Drop this and define this node in the syscon schema with a $ref to this schema.

Ack, makes sense.

>> +  apple,domain-name:
>> +    description: |
>> +      Specifies the name of the SoC device being controlled. This is used to
>> +      name the power/reset domains.
>> +    $ref: /schemas/types.yaml#/definitions/string
> 
> No other power domain binding needs this, why do you?

Because they all hardcode the domain names in the drivers for every SoC :-)

Without a name of some sort in the devicetree, all our genpds would have 
to use numeric register offsets or the like, which seems quite ugly.

> I prefer 1 complete example in the MFD schema rather than piecemeal examples.

Sure. Would we leave this schema without examples then?

> As the child nodes are memory mapped devices, size should be 1. Then
> address translation works (though Linux doesn't care (currently)).

This requires all the reg properties to also declare the reg size, right?

One thing I wonder is whether it would make sense to allow 
#power-domain-cells = <1> and then be able to declare consecutive ranges 
of related power registers in one node. This would be useful for e.g. 
the 9 UARTs, the 4 SPI controllers, the 6 MCAs, the 5 I2C controllers, 
the 5 PWM controllers, etc (which all have uniform parents and features 
and are consecutive, so could be described together). I'm not sure if 
it's worth it, thoughts?
Hector Martin Oct. 6, 2021, 3:55 p.m. UTC | #6
On 07/10/2021 00.52, Hector Martin wrote:
> I realize this is all kind of "not the way things are usually done", but
> I don't want to pass up on the opportunity to have one driver last us
> multiple SoCs if we have the chance, and it's looking like it should :-)

Addendum: just found some prior art for this. See power/pd-samsung.yaml, 
which is another single-PD binding (though in that case they put them in 
the SoC node directly, not under a syscon).
Hector Martin Oct. 6, 2021, 3:59 p.m. UTC | #7
On 06/10/2021 16.05, Krzysztof Kozlowski wrote:
>> +  IP cores belonging to a power domain should contain a
>> +  "power-domains" property that is a phandle for the
>> +  power domain node representing the domain.
> 
> Skip this last paragraph - it is obvious in usage of power domains.
> Specific bindings should not duplicate generic knowledge.

Ack, I'll drop it.

>> +properties:
>> +  $nodename:
>> +    pattern: "^power-controller@[0-9a-f]+$"
> 
> Usually we call nodes as power-domain.

I had it as that originally, but these aren't power domains. These are 
power management domains (they can clock *and* power gate separately, 
where supported) plus also do reset management. So I wasn't sure if it 
was really fair calling them "power-domain" at that point.

>> +  power-domains:
>> +    description:
>> +      Reference to parent power domains. A domain may have multiple parents,
>> +      and all will be powered up when it is powered.
> 
> How many items?

One or more (if there are none the property should not exist). I guess 
that should be encoded.

>> +
>> +  apple,domain-name:
> 
> Use existing binding "label".

Ah, I'd missed that one! I'm glad there's an existing binding for this 
already.

> Your parent schema should include this one for evaluating children.

Yup, I'll do it like that for v2.

Thanks!
Krzysztof Kozlowski Oct. 7, 2021, 1:12 p.m. UTC | #8
On 06/10/2021 17:59, Hector Martin wrote:
> On 06/10/2021 16.05, Krzysztof Kozlowski wrote:
>>> +  IP cores belonging to a power domain should contain a
>>> +  "power-domains" property that is a phandle for the
>>> +  power domain node representing the domain.
>>
>> Skip this last paragraph - it is obvious in usage of power domains.
>> Specific bindings should not duplicate generic knowledge.
> 
> Ack, I'll drop it.
> 
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^power-controller@[0-9a-f]+$"
>>
>> Usually we call nodes as power-domain.
> 
> I had it as that originally, but these aren't power domains. These are 
> power management domains (they can clock *and* power gate separately, 
> where supported) plus also do reset management. So I wasn't sure if it 
> was really fair calling them "power-domain" at that point.

OK, thanks for explanation.

> 
>>> +  power-domains:
>>> +    description:
>>> +      Reference to parent power domains. A domain may have multiple parents,
>>> +      and all will be powered up when it is powered.
>>
>> How many items?
> 
> One or more (if there are none the property should not exist). I guess 
> that should be encoded.

Probably this should not go without any constraints. Are you sure it
could have more than one? It would mean more than one parent.



Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 8, 2021, 7:50 a.m. UTC | #9
On Wed, 6 Oct 2021 at 17:56, Hector Martin <marcan@marcan.st> wrote:
>
> On 07/10/2021 00.52, Hector Martin wrote:
> > I realize this is all kind of "not the way things are usually done", but
> > I don't want to pass up on the opportunity to have one driver last us
> > multiple SoCs if we have the chance, and it's looking like it should :-)
>
> Addendum: just found some prior art for this. See power/pd-samsung.yaml,
> which is another single-PD binding (though in that case they put them in
> the SoC node directly, not under a syscon).

Maybe the design is actually similar. In the Exynos there is a entire
subblock managing power - called Power Management Unit (PMU). It
controls most of power-related parts, except clock gating. For example
it covers registers related to entering deep-sleep modes or power
domains. However we split this into two:
1. Actual PMU driver which controls system-level power (and provides
syscon for other drivers needing to poke its registers... eh, life).
2. Power domain driver which binds multiple devices to a small address
spaces (three registers) inside PMU address space.

The address spaces above overlap, so the (1) PMU driver takes for
example 1004_0000 - 1004_5000 and power domain devices bind to e.g.
1004_4000, 1004_4020, 1004_4040.

Best regards,
Krzysztof
Hector Martin Oct. 11, 2021, 4:42 a.m. UTC | #10
On 07/10/2021 22.12, Krzysztof Kozlowski wrote:
>>>> +  power-domains:
>>>> +    description:
>>>> +      Reference to parent power domains. A domain may have multiple parents,
>>>> +      and all will be powered up when it is powered.
>>>
>>> How many items?
>>
>> One or more (if there are none the property should not exist). I guess
>> that should be encoded.
> 
> Probably this should not go without any constraints. Are you sure it
> could have more than one? It would mean more than one parent.

Yes, at least that is the way Apple describes it in their device tree. 
As I understand it, this is basically a dependency tree of SoC blocks 
that need to be powered up/clocked for a downstream device to work. In 
other words, it's not just a pure clock/power tree, but also represents 
blocks of logic that are shared between devices. So, for example, the 
ADT has relationships like these:

SIO_BUSIF parents: (none)
SIO       parents: SIO_BUSIF
PMS_BUSIF parents: (none)
PMS       parents: (none)
AUDIO_P   parents: SIO
SIO_ADMA  parents: SIO, PMS
MCA0      parents: AUDIO_P, SIO_ADMA

That said, we know some of the data from the ADT is dodgy and doesn't 
match the true hardware (see also the dependency from SIO to SIO_BUSIF 
there, but not from PMS to PMS_BUSIF, which feels wrong), so as we learn 
more about the real relationships between these domains we'll adjust the 
devicetree to better reflect the hardware layout.

There is also the case that even if technically a downstream device 
depends on two domains (directly), the existing genpd infrastructure 
doesn't handle that automatically like it does the single domain case, 
so it ends up making sense to just have some extra domain-domain 
dependencies to keep a bunch of boilerplate manual genpd handling code 
out of device drivers.
Hector Martin Oct. 11, 2021, 5:17 a.m. UTC | #11
On 08/10/2021 16.50, Krzysztof Kozlowski wrote:
> On Wed, 6 Oct 2021 at 17:56, Hector Martin <marcan@marcan.st> wrote:
>> Addendum: just found some prior art for this. See power/pd-samsung.yaml,
>> which is another single-PD binding (though in that case they put them in
>> the SoC node directly, not under a syscon).
> 
> Maybe the design is actually similar. In the Exynos there is a entire
> subblock managing power - called Power Management Unit (PMU). It
> controls most of power-related parts, except clock gating. For example
> it covers registers related to entering deep-sleep modes or power
> domains. However we split this into two:
> 1. Actual PMU driver which controls system-level power (and provides
> syscon for other drivers needing to poke its registers... eh, life).
> 2. Power domain driver which binds multiple devices to a small address
> spaces (three registers) inside PMU address space.
> 
> The address spaces above overlap, so the (1) PMU driver takes for
> example 1004_0000 - 1004_5000 and power domain devices bind to e.g.
> 1004_4000, 1004_4020, 1004_4040.

It's similar, except Apple tends to group registers into uniform arrays, 
sometimes with gaps. They definitely do some ugly overlap stuff in their 
devicetree/OS which we'll try to avoid (e.g. the audio driver directly 
poking clock select registers...).

Here's an incomplete memory map of the PMGR-related stuff in this SoC:

2_3b00_0000:  Clocking
      0_0000:   PLLs
      4_0000:   Clock selects / dividers
         000:    86 selects x 32bit (device clocks)
         200:    8 selects x 32bit (aux clocks)
         280:    2 selects x 32bit
      4_4000:   NCOs (used for audio) (5 x one 16KiB page each)
2_3b70_0000:  PMGR
      0_0000:   Pwr-state registers
        0000:    10 controls x 64bit  (CPUs)
        0100:    143 controls x 64bit (general devices)
        0c00:    2 controls x 64bit   (SEP)
        4000:    13 controls x 64bit  (ISP)
        8000:    5 controls x 64bit   (VENC)
        c000:    7 controls x 64bit   (ANE)
      1_0000:    10 controls x 64bit  (DISP0)
      1_c000:   Power gates
          10:    74 power gates (24 bytes each?)
      3_4100:   Performance counters? (16 bytes each, big array)
      5_4000:   Secondary CPU spin-up controls

I believe the weird groupings into page-sized areas have to do with 
security, so they can map only those ranges to certain coprocessors and 
the like via the IOMMUs.

There is also a MiniPMGR that uses the same register formats, but 
different counts/offsets, at 2_3d28_0000 (this one stays up in sleep 
mode, AIUI)

So I think we won't need any overlaps, since e.g. the whole 00000-14000 
subrange is all power state controls, so a driver doing system-level 
stuff wouldn't have to care about those. Those would just be bound by 
the syscon in this patchset. I chose to use a syscon to avoid having raw 
ioremaps for each domain instance, since each one of those eats up a 
whole page of mapping AIUI (and shows up in /proc/iomem individually).

One question is whether if we need to poke at power gates directly 
(which isn't clear right now), we should have a separate top-level 
pmgr-pwrgate syscon as a parent, or just instantiate power gate subnodes 
under the same pmgr syscon at 1c000.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
new file mode 100644
index 000000000000..a14bf5f30ff0
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
@@ -0,0 +1,117 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/apple,pmgr-pwrstate.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple SoC PMGR Power States
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+allOf:
+  - $ref: "power-domain.yaml#"
+
+description: |
+  Apple SoCs include a PMGR block responsible for power management,
+  which can control various clocks, resets, power states, and
+  performance features. This binding describes the device power
+  state registers, which control power states and resets.
+
+  Each instance of a power controller within the PMGR syscon node
+  represents a generic power domain provider, as documented in
+  Documentation/devicetree/bindings/power/power-domain.yaml.
+  The provider controls a single SoC block. The power hierarchy is
+  represented via power-domains relationships between these nodes.
+
+  See Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
+  for the top-level PMGR node documentation.
+
+  IP cores belonging to a power domain should contain a
+  "power-domains" property that is a phandle for the
+  power domain node representing the domain.
+
+properties:
+  $nodename:
+    pattern: "^power-controller@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - enum:
+          - apple,t8103-pmgr-pwrstate
+      - const: apple,pmgr-pwrstate
+
+  reg:
+    maxItems: 1
+
+  "#power-domain-cells":
+    const: 0
+
+  "#reset-cells":
+    const: 0
+
+  power-domains:
+    description:
+      Reference to parent power domains. A domain may have multiple parents,
+      and all will be powered up when it is powered.
+
+  apple,domain-name:
+    description: |
+      Specifies the name of the SoC device being controlled. This is used to
+      name the power/reset domains.
+    $ref: /schemas/types.yaml#/definitions/string
+
+  apple,always-on:
+    description: |
+      Forces this power domain to always be powered up.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - "#power-domain-cells"
+  - "#reset-cells"
+  - "apple,domain-name"
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        power-management@23b700000 {
+            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <0x2 0x3b700000 0x0 0x14000>;
+
+            ps_sio: power-controller@1c0 {
+                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+                reg = <0x1c0>;
+                #power-domain-cells = <0>;
+                #reset-cells = <0>;
+                apple,domain-name = "sio";
+                apple,always-on;
+            };
+
+            ps_uart_p: power-controller@220 {
+                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+                reg = <0x220>;
+                #power-domain-cells = <0>;
+                #reset-cells = <0>;
+                apple,domain-name = "uart_p";
+                power-domains = <&ps_sio>;
+            };
+
+            ps_uart0: power-controller@270 {
+                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+                reg = <0x270>;
+                #power-domain-cells = <0>;
+                #reset-cells = <0>;
+                apple,domain-name = "uart0";
+                power-domains = <&ps_uart_p>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d25598842d15..5fe53d9a2956 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1722,6 +1722,7 @@  F:	Documentation/devicetree/bindings/arm/apple.yaml
 F:	Documentation/devicetree/bindings/arm/apple/*
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
+F:	Documentation/devicetree/bindings/power/apple*
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h