| Message ID | 20201207142756.17819-3-michael@fossekall.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | BPi M2 Zero poweroff support via new regulator-poweroff driver | expand |
On Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote: > Add devicetree binding documentation for regulator-poweroff driver. > > Signed-off-by: Michael Klein <michael@fossekall.de> > --- > .../power/reset/regulator-poweroff.yaml | 53 +++++++++++++++++++ > 1 file changed, 53 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > > diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > new file mode 100644 > index 000000000000..8c8ce6bb031a > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > @@ -0,0 +1,53 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Force-disable power regulators to turn the power off. > + > +maintainers: > + - Michael Klein <michael@fossekall.de> > + > +description: | > + When the power-off handler is called, one more regulators are disabled > + by calling regulator_force_disable(). If the power is still on and the > + CPU still running after a 3000ms delay, a WARN_ON(1) is emitted. > + > +properties: > + compatible: > + const: "regulator-poweroff" > + > + regulator-names: > + description: > + Array of regulator names > + $ref: /schemas/types.yaml#/definitions/string-array > + > + REGULATOR-supply: This should be a patternProperties > + description: > + For any REGULATOR listed in regulator-names, a phandle > + to the corresponding regulator node > + $ref: /schemas/types.yaml#/definitions/phandle > + > + timeout-ms: > + description: > + Time to wait before asserting a WARN_ON(1). If nothing is > + specified, 3000 ms is used. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > +required: > + - compatible > + - regulator-names > + - REGULATOR-supply > + > +additionalProperties: false > + > +examples: > + - | > + regulator-poweroff { > + compatible = "regulator-poweroff"; > + regulator-names = "vcc1v2", "vcc-dram"; > + vcc1v2-supply = <®_vcc1v2>; > + vcc-dram-supply = <®_vcc_dram>; > + }; I'm not entirely sure how multiple regulators would work here. I guess the ordering is board/purpose sensitive. In this particular case, I assume that vcc1v2 would be shut down before vcc-dram? If so, I would expect that one regulator_force_disable is run, the CPU is disabled and you never get the chance to cut vcc-dram. Similarly, cutting the RAM regulator first would probably be fine if you're running code from the cache / SRAM, but I don't see anything making sure it's the case in the driver? Maxime
Thanks for reviewing! On Tue, Dec 08, 2020 at 11:13:58AM +0100, Maxime Ripard wrote: >On Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote: >> Add devicetree binding documentation for regulator-poweroff driver. >> >> Signed-off-by: Michael Klein <michael@fossekall.de> >> --- >> .../power/reset/regulator-poweroff.yaml | 53 +++++++++++++++++++ >> 1 file changed, 53 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml >> >> diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml >> new file mode 100644 >> index 000000000000..8c8ce6bb031a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml >> @@ -0,0 +1,53 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Force-disable power regulators to turn the power off. >> + >> +maintainers: >> + - Michael Klein <michael@fossekall.de> >> + >> +description: | >> + When the power-off handler is called, one more regulators are disabled >> + by calling regulator_force_disable(). If the power is still on and the >> + CPU still running after a 3000ms delay, a WARN_ON(1) is emitted. >> + >> +properties: >> + compatible: >> + const: "regulator-poweroff" >> + >> + regulator-names: >> + description: >> + Array of regulator names >> + $ref: /schemas/types.yaml#/definitions/string-array >> + >> + REGULATOR-supply: > >This should be a patternProperties > >> + description: >> + For any REGULATOR listed in regulator-names, a phandle >> + to the corresponding regulator node >> + $ref: /schemas/types.yaml#/definitions/phandle >> + >> + timeout-ms: >> + description: >> + Time to wait before asserting a WARN_ON(1). If nothing is >> + specified, 3000 ms is used. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> +required: >> + - compatible >> + - regulator-names >> + - REGULATOR-supply >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + regulator-poweroff { >> + compatible = "regulator-poweroff"; >> + regulator-names = "vcc1v2", "vcc-dram"; >> + vcc1v2-supply = <®_vcc1v2>; >> + vcc-dram-supply = <®_vcc_dram>; >> + }; > >I'm not entirely sure how multiple regulators would work here. I guess >the ordering is board/purpose sensitive. In this particular case, I >assume that vcc1v2 would be shut down before vcc-dram? yes, the regulators are shut down from left to right. >If so, I would expect that one regulator_force_disable is run, the CPU >is disabled and you never get the chance to cut vcc-dram. I assume that any relevant regulator here has enough capacitance on the output that provides enough charge to disable any remaining regulators (my board has 3*10µF for vcc1v2 and 1*10µF for vcc-dram). But there is of course no guarantee, so I'm shutting down the most relevant (in terms of current consumption) regulator first. In any case, if it's deemed unnecessary to allow more than one regulator in the driver I could remove the regulator-names property altogether and reduce the DT node to: regulator-poweroff { compatible = "regulator-poweroff"; poweroff-supply = <®_vcc1v2>; };
On Mon, 07 Dec 2020 15:27:55 +0100, Michael Klein wrote: > Add devicetree binding documentation for regulator-poweroff driver. > > Signed-off-by: Michael Klein <michael@fossekall.de> > --- > .../power/reset/regulator-poweroff.yaml | 53 +++++++++++++++++++ > 1 file changed, 53 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/reset/regulator-poweroff.example.dt.yaml: regulator-poweroff: 'REGULATOR-supply' is a required property From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/reset/regulator-poweroff.example.dt.yaml: regulator-poweroff: 'vcc-dram-supply', 'vcc1v2-supply' do not match any of the regexes: 'pinctrl-[0-9]+' From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml See https://patchwork.ozlabs.org/patch/1412084 The base for the patch is generally the last rc1. Any dependencies should be noted. 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 Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote: > Add devicetree binding documentation for regulator-poweroff driver. > > Signed-off-by: Michael Klein <michael@fossekall.de> > --- > .../power/reset/regulator-poweroff.yaml | 53 +++++++++++++++++++ > 1 file changed, 53 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > > diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > new file mode 100644 > index 000000000000..8c8ce6bb031a > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > @@ -0,0 +1,53 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Force-disable power regulators to turn the power off. > + > +maintainers: > + - Michael Klein <michael@fossekall.de> > + > +description: | > + When the power-off handler is called, one more regulators are disabled > + by calling regulator_force_disable(). If the power is still on and the > + CPU still running after a 3000ms delay, a WARN_ON(1) is emitted. WARN_ON is a Linux thing. Bindings are independent from Linux. > + > +properties: > + compatible: > + const: "regulator-poweroff" > + > + regulator-names: We already have 'regulator-name' which is something different, and *-names already has a defined usage as a companion to other properties ('foo-names' goes with 'foos'). More on this below... > + description: > + Array of regulator names > + $ref: /schemas/types.yaml#/definitions/string-array > + > + REGULATOR-supply: > + description: > + For any REGULATOR listed in regulator-names, a phandle > + to the corresponding regulator node > + $ref: /schemas/types.yaml#/definitions/phandle *-supply already has a type. > + > + timeout-ms: > + description: > + Time to wait before asserting a WARN_ON(1). If nothing is > + specified, 3000 ms is used. > + $ref: /schemas/types.yaml#/definitions/uint32 Do we really need to tune the timeout just for an error message? > + > +required: > + - compatible > + - regulator-names > + - REGULATOR-supply > + > +additionalProperties: false > + > +examples: > + - | > + regulator-poweroff { > + compatible = "regulator-poweroff"; > + regulator-names = "vcc1v2", "vcc-dram"; > + vcc1v2-supply = <®_vcc1v2>; > + vcc-dram-supply = <®_vcc_dram>; -supply names are supposed to be named based on the consumer names (e.g. LDO1 regulator supplies vcc-supply). To avoid 'regulator-names' and simplifier the driver, I'd just define fixed, known names. Something like: power1-supply power2-supply ... Rob
Hi On Tue, Dec 08, 2020 at 01:52:14PM +0100, Michael Klein wrote: > Thanks for reviewing! > > On Tue, Dec 08, 2020 at 11:13:58AM +0100, Maxime Ripard wrote: > > On Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote: > > > Add devicetree binding documentation for regulator-poweroff driver. > > > > > > Signed-off-by: Michael Klein <michael@fossekall.de> > > > --- > > > .../power/reset/regulator-poweroff.yaml | 53 +++++++++++++++++++ > > > 1 file changed, 53 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > > > new file mode 100644 > > > index 000000000000..8c8ce6bb031a > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml > > > @@ -0,0 +1,53 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Force-disable power regulators to turn the power off. > > > + > > > +maintainers: > > > + - Michael Klein <michael@fossekall.de> > > > + > > > +description: | > > > + When the power-off handler is called, one more regulators are disabled > > > + by calling regulator_force_disable(). If the power is still on and the > > > + CPU still running after a 3000ms delay, a WARN_ON(1) is emitted. > > > + > > > +properties: > > > + compatible: > > > + const: "regulator-poweroff" > > > + > > > + regulator-names: > > > + description: > > > + Array of regulator names > > > + $ref: /schemas/types.yaml#/definitions/string-array > > > + > > > + REGULATOR-supply: > > > > This should be a patternProperties > > > > > + description: > > > + For any REGULATOR listed in regulator-names, a phandle > > > + to the corresponding regulator node > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > + > > > + timeout-ms: > > > + description: > > > + Time to wait before asserting a WARN_ON(1). If nothing is > > > + specified, 3000 ms is used. > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + > > > +required: > > > + - compatible > > > + - regulator-names > > > + - REGULATOR-supply > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + regulator-poweroff { > > > + compatible = "regulator-poweroff"; > > > + regulator-names = "vcc1v2", "vcc-dram"; > > > + vcc1v2-supply = <®_vcc1v2>; > > > + vcc-dram-supply = <®_vcc_dram>; > > > + }; > > > > I'm not entirely sure how multiple regulators would work here. I guess > > the ordering is board/purpose sensitive. In this particular case, I > > assume that vcc1v2 would be shut down before vcc-dram? > > yes, the regulators are shut down from left to right. > > > If so, I would expect that one regulator_force_disable is run, the CPU > > is disabled and you never get the chance to cut vcc-dram. > > I assume that any relevant regulator here has enough capacitance on the > output that provides enough charge to disable any remaining regulators (my > board has 3*10µF for vcc1v2 and 1*10µF for vcc-dram). But there is of course > no guarantee, so I'm shutting down the most relevant (in terms of current > consumption) regulator first. > > In any case, if it's deemed unnecessary to allow more than one regulator in > the driver I could remove the regulator-names property altogether and reduce > the DT node to: > > regulator-poweroff { > compatible = "regulator-poweroff"; > poweroff-supply = <®_vcc1v2>; > }; It's mostly about semantics at this point but there's value in shutting down the RAM as well if we're taking some precautions I mentionned. Maybe we can name that regulator cpu-supply, so that we can easily add the DRAM one if needed, and the semantics are clear? Maxime
diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml new file mode 100644 index 000000000000..8c8ce6bb031a --- /dev/null +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Force-disable power regulators to turn the power off. + +maintainers: + - Michael Klein <michael@fossekall.de> + +description: | + When the power-off handler is called, one more regulators are disabled + by calling regulator_force_disable(). If the power is still on and the + CPU still running after a 3000ms delay, a WARN_ON(1) is emitted. + +properties: + compatible: + const: "regulator-poweroff" + + regulator-names: + description: + Array of regulator names + $ref: /schemas/types.yaml#/definitions/string-array + + REGULATOR-supply: + description: + For any REGULATOR listed in regulator-names, a phandle + to the corresponding regulator node + $ref: /schemas/types.yaml#/definitions/phandle + + timeout-ms: + description: + Time to wait before asserting a WARN_ON(1). If nothing is + specified, 3000 ms is used. + $ref: /schemas/types.yaml#/definitions/uint32 + +required: + - compatible + - regulator-names + - REGULATOR-supply + +additionalProperties: false + +examples: + - | + regulator-poweroff { + compatible = "regulator-poweroff"; + regulator-names = "vcc1v2", "vcc-dram"; + vcc1v2-supply = <®_vcc1v2>; + vcc-dram-supply = <®_vcc_dram>; + }; +...
Add devicetree binding documentation for regulator-poweroff driver. Signed-off-by: Michael Klein <michael@fossekall.de> --- .../power/reset/regulator-poweroff.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml