| Message ID | 20211025144718.157794-3-marcan@marcan.st (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Apple SoC PMGR device power states driver | expand |
On Mon, 25 Oct 2021 23:47:12 +0900, Hector Martin wrote: > The PMGR block in Apple Silicon SoCs is responsible for SoC power > management. There are two PMGRs in T8103, with different register > layouts but compatible registers. In order to support this as well > as future SoC generations with backwards-compatible registers, we > declare these blocks as syscons and bind to individual registers > in child nodes. Each register controls one SoC device. > > The respective apple compatibles are defined in case device-specific > quirks are necessary in the future, but currently these nodes are > expected to be bound by the generic syscon driver. > > Reviewed-by: Mark Kettenis <kettenis@openbsd.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../bindings/arm/apple/apple,pmgr.yaml | 149 ++++++++++++++++++ > 1 file changed, 149 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/power/apple,pmgr-pwrstate.yaml' xargs: dt-doc-validate: exited with status 255; aborting make[1]: *** Deleting file 'Documentation/devicetree/bindings/arm/apple/apple,pmgr.example.dt.yaml' Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/power/apple,pmgr-pwrstate.yaml' make[1]: *** [scripts/Makefile.lib:385: Documentation/devicetree/bindings/arm/apple/apple,pmgr.example.dt.yaml] Error 255 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1441: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1545799 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On 26/10/2021 03.16, Rob Herring wrote: > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/power/apple,pmgr-pwrstate.yaml' > xargs: dt-doc-validate: exited with status 255; aborting > make[1]: *** Deleting file 'Documentation/devicetree/bindings/arm/apple/apple,pmgr.example.dt.yaml' > Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/power/apple,pmgr-pwrstate.yaml' > make[1]: *** [scripts/Makefile.lib:385: Documentation/devicetree/bindings/arm/apple/apple,pmgr.example.dt.yaml] Error 255 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1441: dt_binding_check] Error 2 Ah, I guess this is just an order issue. Patches 2/3 should've been swapped... sorry about that, I only ran the checker on the final state, not the intermediate ones.
On Mon, Oct 25, 2021 at 11:47:12PM +0900, Hector Martin wrote: > The PMGR block in Apple Silicon SoCs is responsible for SoC power > management. There are two PMGRs in T8103, with different register > layouts but compatible registers. In order to support this as well > as future SoC generations with backwards-compatible registers, we > declare these blocks as syscons and bind to individual registers > in child nodes. Each register controls one SoC device. > > The respective apple compatibles are defined in case device-specific > quirks are necessary in the future, but currently these nodes are > expected to be bound by the generic syscon driver. > > Reviewed-by: Mark Kettenis <kettenis@openbsd.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../bindings/arm/apple/apple,pmgr.yaml | 149 ++++++++++++++++++ > 1 file changed, 149 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml > > diff --git a/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml > new file mode 100644 > index 000000000000..e8b7776163fc > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml > @@ -0,0 +1,149 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/arm/apple/apple,pmgr.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Apple SoC Power Manager (PMGR) > + > +maintainers: > + - Hector Martin <marcan@marcan.st> > + > +description: | > + Apple SoCs include a PMGR block responsible for power management, > + which can control various clocks, resets, power states, and > + performance features. This node represents the PMGR as a syscon, > + with sub-nodes representing individual features. > + > + Apple SoCs may have a secondary "mini-PMGR"; it is represented > + separately in the device tree, but works the same way. > + > +select: > + properties: > + compatible: > + contains: > + enum: > + - apple,t8103-pmgr > + - apple,t8103-minipmgr > + - apple,pmgr You shouldn't need this. The default select will filter out syscon and simple-mfd. > + > + required: > + - compatible > + > +properties: > + $nodename: > + pattern: "^power-management@[0-9a-f]+$" > + > + compatible: > + items: > + - enum: > + - apple,t8103-pmgr > + - apple,t8103-minipmgr > + - const: apple,pmgr > + - const: syscon > + - const: simple-mfd 'simple-mfd' means 'there's nothing in this node that any of the child nodes depend on'. You should be somewhat certain as dropping it later creates compatibility issues. > + > + reg: > + maxItems: 1 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 1 > + > +patternProperties: > + "power-controller@[0-9a-f]+$": > + description: | Don't need '|' if no formatting to preserve. > + The individual power management domains within this controller > + type: object > + $ref: /power/apple,pmgr-pwrstate.yaml# > + > +required: > + - compatible > + - reg > + > +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 = <1>; > + reg = <0x2 0x3b700000 0x0 0x14000>; > + > + ps_sio: power-controller@1c0 { > + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; > + reg = <0x1c0 8>; > + #power-domain-cells = <0>; > + #reset-cells = <0>; > + label = "sio"; > + apple,always-on; > + }; > + > + ps_uart_p: power-controller@220 { > + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; > + reg = <0x220 8>; > + #power-domain-cells = <0>; > + #reset-cells = <0>; > + label = "uart_p"; > + power-domains = <&ps_sio>; > + }; > + > + ps_uart0: power-controller@270 { > + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; > + reg = <0x270 8>; > + #power-domain-cells = <0>; > + #reset-cells = <0>; > + label = "uart0"; > + power-domains = <&ps_uart_p>; > + }; > + }; > + > + power-management@23d280000 { > + compatible = "apple,t8103-minipmgr", "apple,pmgr", "syscon", "simple-mfd"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x2 0x3d280000 0x0 0xc000>; > + > + ps_aop_filter: power-controller@4000 { > + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; > + reg = <0x4000 8>; > + #power-domain-cells = <0>; > + #reset-cells = <0>; > + label = "aop_filter"; > + }; > + > + ps_aop_base: power-controller@4010 { > + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; > + reg = <0x4010 8>; > + #power-domain-cells = <0>; > + #reset-cells = <0>; > + label = "aop_base"; > + power-domains = <&ps_aop_filter>; > + }; > + > + ps_aop_shim: power-controller@4038 { > + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; > + reg = <0x4038 8>; > + #power-domain-cells = <0>; > + #reset-cells = <0>; > + label = "aop_shim"; > + power-domains = <&ps_aop_base>; > + }; > + > + ps_aop_uart0: power-controller@4048 { > + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; > + reg = <0x4048 8>; > + #power-domain-cells = <0>; > + #reset-cells = <0>; > + label = "aop_uart0"; > + power-domains = <&ps_aop_shim>; > + }; > + }; > + }; > -- > 2.33.0 > >
On 27/10/2021 03.25, Rob Herring wrote: > On Mon, Oct 25, 2021 at 11:47:12PM +0900, Hector Martin wrote: >> + compatible: >> + items: >> + - enum: >> + - apple,t8103-pmgr >> + - apple,t8103-minipmgr >> + - const: apple,pmgr >> + - const: syscon >> + - const: simple-mfd > > > 'simple-mfd' means 'there's nothing in this node that any of the child > nodes depend on'. You should be somewhat certain as dropping it later > creates compatibility issues. Hmm, I see simple-mfd turns this into a bus which I guess allows child nodes to be probed without the parent node doing anything special (then we use syscon_node_to_regmap to get the syscon instantiated). Do you have a example use case for doing this without simple-mfd? At this point I can't think of anything we'd need from the parent node, especially if we end up using this syscon strictly for pwrstate subnodes (which seems likely at this point). One thing that comes to mind is telling the PMP (a coprocessor in charge of power metrics/management) about some domains being turned on/off, which is apparently a thing, but that wouldn't even be in this node; that'd have to be a phandle property in the child nodes referencing a PMP/coprocessor node elsewhere (none of which is implemented right now, and which should be backwards compatible once it is). If it turns out we do have a dep of some sort in the end, could we just have the child node driver return -EPROBE_DEFER until the parent is probed and has made whatever service available? That would allow us to keep simple-mfd, right? If it works for you, I'll also just squash the two bindings into one commit for the next spin, since there is a direct dependency at this point and it should make things easier. Otherwise, I can just swap the order if you prefer it that way. Ack on the other formatting changes; if the rest of the series looks good to the other folks I'll try to respin this into a v3 soon, to see if we can sneak it in by 5.16, since it'd be nice to have the power domain stuff in there :)
On Tue, Oct 26, 2021 at 10:38 PM Hector Martin <marcan@marcan.st> wrote: > > On 27/10/2021 03.25, Rob Herring wrote: > > On Mon, Oct 25, 2021 at 11:47:12PM +0900, Hector Martin wrote: > >> + compatible: > >> + items: > >> + - enum: > >> + - apple,t8103-pmgr > >> + - apple,t8103-minipmgr > >> + - const: apple,pmgr > >> + - const: syscon > >> + - const: simple-mfd > > > > > > 'simple-mfd' means 'there's nothing in this node that any of the child > > nodes depend on'. You should be somewhat certain as dropping it later > > creates compatibility issues. > > Hmm, I see simple-mfd turns this into a bus which I guess allows child > nodes to be probed without the parent node doing anything special (then > we use syscon_node_to_regmap to get the syscon instantiated). Do you > have a example use case for doing this without simple-mfd? Drivers calling of_platform_populate or devm_of_platform_populate. That of course does mean you need a driver. We could probably make the syscon driver call these if needed. > At this point I can't think of anything we'd need from the parent node, > especially if we end up using this syscon strictly for pwrstate subnodes > (which seems likely at this point). One thing that comes to mind is > telling the PMP (a coprocessor in charge of power metrics/management) > about some domains being turned on/off, which is apparently a thing, but > that wouldn't even be in this node; that'd have to be a phandle property > in the child nodes referencing a PMP/coprocessor node elsewhere (none of > which is implemented right now, and which should be backwards compatible > once it is). > > If it turns out we do have a dep of some sort in the end, could we just > have the child node driver return -EPROBE_DEFER until the parent is > probed and has made whatever service available? That would allow us to > keep simple-mfd, right? That would have saved you, but deferred probe is now a fallback to fw_devlink and it makes sure parent driver probes first. That works unless there isn't a parent driver which is often the case for simple-bus[1]. I think you are okay since 'syscon' means there is a driver. > If it works for you, I'll also just squash the two bindings into one > commit for the next spin, since there is a direct dependency at this > point and it should make things easier. Otherwise, I can just swap the > order if you prefer it that way. Just swapping seems like less work, but either way. Rob [1] https://lore.kernel.org/all/CAL_JsqJcsqjJBe8aULYYMkFtx8OTj2wHANZ=83VMMyJ=AEgReg@mail.gmail.com/
On Wed, 27 Oct 2021 at 16:44, Rob Herring <robh@kernel.org> wrote: > > On Tue, Oct 26, 2021 at 10:38 PM Hector Martin <marcan@marcan.st> wrote: > > > > On 27/10/2021 03.25, Rob Herring wrote: > > > On Mon, Oct 25, 2021 at 11:47:12PM +0900, Hector Martin wrote: > > >> + compatible: > > >> + items: > > >> + - enum: > > >> + - apple,t8103-pmgr > > >> + - apple,t8103-minipmgr > > >> + - const: apple,pmgr > > >> + - const: syscon > > >> + - const: simple-mfd > > > > > > > > > 'simple-mfd' means 'there's nothing in this node that any of the child > > > nodes depend on'. You should be somewhat certain as dropping it later > > > creates compatibility issues. > > > > Hmm, I see simple-mfd turns this into a bus which I guess allows child > > nodes to be probed without the parent node doing anything special (then > > we use syscon_node_to_regmap to get the syscon instantiated). Do you > > have a example use case for doing this without simple-mfd? > > Drivers calling of_platform_populate or devm_of_platform_populate. > > That of course does mean you need a driver. We could probably make the > syscon driver call these if needed. > Hi Hector, I thought I mentioned this with your v1, maybe the comment got lost. We have it for Exynos PMU: drivers/soc/samsung/exynos-pmu.c arch/arm/boot/dts/exynos-syscon-restart.dtsi (extending node from arch/arm/boot/dts/exynos5420.dtsi) Maybe you can base on that. Best regards, Krzysztof
On 27/10/2021 23.51, Krzysztof Kozlowski wrote: > On Wed, 27 Oct 2021 at 16:44, Rob Herring <robh@kernel.org> wrote: >> >> On Tue, Oct 26, 2021 at 10:38 PM Hector Martin <marcan@marcan.st> wrote: >>> >>> On 27/10/2021 03.25, Rob Herring wrote: >>>> On Mon, Oct 25, 2021 at 11:47:12PM +0900, Hector Martin wrote: >>>>> + compatible: >>>>> + items: >>>>> + - enum: >>>>> + - apple,t8103-pmgr >>>>> + - apple,t8103-minipmgr >>>>> + - const: apple,pmgr >>>>> + - const: syscon >>>>> + - const: simple-mfd >>>> >>>> >>>> 'simple-mfd' means 'there's nothing in this node that any of the child >>>> nodes depend on'. You should be somewhat certain as dropping it later >>>> creates compatibility issues. >>> >>> Hmm, I see simple-mfd turns this into a bus which I guess allows child >>> nodes to be probed without the parent node doing anything special (then >>> we use syscon_node_to_regmap to get the syscon instantiated). Do you >>> have a example use case for doing this without simple-mfd? >> >> Drivers calling of_platform_populate or devm_of_platform_populate. >> >> That of course does mean you need a driver. We could probably make the >> syscon driver call these if needed. >> > > Hi Hector, > > I thought I mentioned this with your v1, maybe the comment got lost. > We have it for Exynos PMU: > drivers/soc/samsung/exynos-pmu.c > arch/arm/boot/dts/exynos-syscon-restart.dtsi (extending node from > arch/arm/boot/dts/exynos5420.dtsi) > Maybe you can base on that. Ah, I remember the discrete power domains but I missed this syscon. I see this is mostly used for poweroff/reboot, which makes sense in this context. For pmgr though, the binding only describes the uniform power state registers, so I think I'm comfortable leaving it as a simple-mfd. Other pmgr sub-blocks will probably end up as separate nodes with different bindings anyway (e.g. whatever I do for the clock muxes, need to see how that ties in with audio which I think is the only consumer so far). If things get more complicated in future SoCs then we can change how we do it on those, of course :)
diff --git a/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml new file mode 100644 index 000000000000..e8b7776163fc --- /dev/null +++ b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml @@ -0,0 +1,149 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/arm/apple/apple,pmgr.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple SoC Power Manager (PMGR) + +maintainers: + - Hector Martin <marcan@marcan.st> + +description: | + Apple SoCs include a PMGR block responsible for power management, + which can control various clocks, resets, power states, and + performance features. This node represents the PMGR as a syscon, + with sub-nodes representing individual features. + + Apple SoCs may have a secondary "mini-PMGR"; it is represented + separately in the device tree, but works the same way. + +select: + properties: + compatible: + contains: + enum: + - apple,t8103-pmgr + - apple,t8103-minipmgr + - apple,pmgr + + required: + - compatible + +properties: + $nodename: + pattern: "^power-management@[0-9a-f]+$" + + compatible: + items: + - enum: + - apple,t8103-pmgr + - apple,t8103-minipmgr + - const: apple,pmgr + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 1 + +patternProperties: + "power-controller@[0-9a-f]+$": + description: | + The individual power management domains within this controller + type: object + $ref: /power/apple,pmgr-pwrstate.yaml# + +required: + - compatible + - reg + +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 = <1>; + reg = <0x2 0x3b700000 0x0 0x14000>; + + ps_sio: power-controller@1c0 { + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; + reg = <0x1c0 8>; + #power-domain-cells = <0>; + #reset-cells = <0>; + label = "sio"; + apple,always-on; + }; + + ps_uart_p: power-controller@220 { + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; + reg = <0x220 8>; + #power-domain-cells = <0>; + #reset-cells = <0>; + label = "uart_p"; + power-domains = <&ps_sio>; + }; + + ps_uart0: power-controller@270 { + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; + reg = <0x270 8>; + #power-domain-cells = <0>; + #reset-cells = <0>; + label = "uart0"; + power-domains = <&ps_uart_p>; + }; + }; + + power-management@23d280000 { + compatible = "apple,t8103-minipmgr", "apple,pmgr", "syscon", "simple-mfd"; + #address-cells = <1>; + #size-cells = <1>; + reg = <0x2 0x3d280000 0x0 0xc000>; + + ps_aop_filter: power-controller@4000 { + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; + reg = <0x4000 8>; + #power-domain-cells = <0>; + #reset-cells = <0>; + label = "aop_filter"; + }; + + ps_aop_base: power-controller@4010 { + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; + reg = <0x4010 8>; + #power-domain-cells = <0>; + #reset-cells = <0>; + label = "aop_base"; + power-domains = <&ps_aop_filter>; + }; + + ps_aop_shim: power-controller@4038 { + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; + reg = <0x4038 8>; + #power-domain-cells = <0>; + #reset-cells = <0>; + label = "aop_shim"; + power-domains = <&ps_aop_base>; + }; + + ps_aop_uart0: power-controller@4048 { + compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate"; + reg = <0x4048 8>; + #power-domain-cells = <0>; + #reset-cells = <0>; + label = "aop_uart0"; + power-domains = <&ps_aop_shim>; + }; + }; + };