| Message ID | 20230315110736.35506-2-jpanis@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | TI TPS6594 PMIC support (Core, ESM, PFSM) | expand |
On 3/20/23 16:53, Rob Herring wrote: > On Wed, Mar 15, 2023 at 12:07:33PM +0100, Julien Panis wrote: >> TPS6594 is a Power Management IC which provides regulators and others >> features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and >> PFSM (Pre-configurable Finite State Machine) managing the state of the >> device. >> TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. > As mentioned, the binding needs to be complete. It's missing GPIO at > least. RTC and watchdog may or may not need binding changes. Thank you for your feedback. About GPIO, do you speak about 'gpio-controller' and/or '#gpio-cells' properties ? For RTC (and for watchdog, once the driver will be implemented), our driver do not require any node to work. What could make an explicit instantiation necessary in DT ? > >> Signed-off-by: Julien Panis <jpanis@baylibre.com> >> --- >> .../devicetree/bindings/mfd/ti,tps6594.yaml | 191 ++++++++++++++++++ >> 1 file changed, 191 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >> >> diff --git a/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >> new file mode 100644 >> index 000000000000..18f47cd6a2f9 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >> @@ -0,0 +1,191 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/mfd/ti,tps6594.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: TI TPS6594 Power Management Integrated Circuit >> + >> +maintainers: >> + - Julien Panis <jpanis@baylibre.com> >> + >> +description: | > Don't need '|'. > >> + TPS6594 is a Power Management IC which provides regulators and others >> + features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and >> + PFSM (Pre-configurable Finite State Machine) managing the state of the device. >> + TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. >> + >> +properties: >> + compatible: >> + enum: >> + - ti,lp8764x >> + - ti,tps6593 >> + - ti,tps6594 >> + >> + reg: >> + description: I2C slave address or SPI chip select number. >> + maxItems: 1 >> + >> + ti,spmi-controller: >> + type: boolean >> + description: | >> + Identify the primary PMIC on SPMI bus. > Perhaps the property name should include 'primary' and 'pmic'. > Otherwise, it looks like it is just marked as 'a SPMI controller'. Including 'primary' and 'pmic' will be more understandable indeed. I will change that in v3. > > >> + A multi-PMIC synchronization scheme is implemented in the PMIC device >> + to synchronize the power state changes with other PMIC devices. This is >> + accomplished through a SPMI bus: the primary PMIC is the controller >> + device on the SPMI bus, and the secondary PMICs are the target devices >> + on the SPMI bus. > Is this a TI specific feature? I don't think so. I will double-check that. If not, shall I remove the 'ti,' prefix ? > >> + >> + system-power-controller: true >> + >> + interrupts: >> + maxItems: 1 >> + >> + ti,multi-phase-id: >> + description: | >> + Describes buck multi-phase configuration, if any. For instance, XY id means >> + that outputs of buck converters X and Y are combined in multi-phase mode. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [12, 34, 123, 1234] > coupled regulator stuff doesn't work here? Coupled regulator stuff works here. Is it also necessary to specify some 'allOf' logic here to ensure that mutual exclusions described below (for regulators) will be applied ? > >> + >> + regulators: >> + type: object >> + description: List of regulators provided by this controller. >> + >> + patternProperties: >> + "^buck([1-5]|12|34|123|1234)$": >> + type: object >> + $ref: /schemas/regulator/regulator.yaml# >> + >> + unevaluatedProperties: false >> + >> + "^ldo[1-4]$": >> + type: object >> + $ref: /schemas/regulator/regulator.yaml# >> + >> + unevaluatedProperties: false >> + >> + allOf: >> + - if: >> + required: >> + - buck12 >> + then: >> + properties: >> + buck123: false >> + buck1234: false >> + - if: >> + required: >> + - buck123 >> + then: >> + properties: >> + buck34: false >> + - if: >> + required: >> + - buck1234 >> + then: >> + properties: >> + buck34: false >> + >> + additionalProperties: false >> + >> +patternProperties: >> + "^buck([1-5]|12|34|123|1234)-supply$": >> + description: Input supply phandle for each buck. >> + >> + "^ldo[1-4]-supply$": >> + description: Input supply phandle for each ldo. >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + tps6593: pmic@48 { >> + compatible = "ti,tps6593"; >> + reg = <0x48>; >> + ti,spmi-controller; >> + system-power-controller; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pmic_irq_pins_default>; >> + interrupt-parent = <&mcu_gpio0>; >> + interrupts = <0 IRQ_TYPE_EDGE_FALLING>; >> + >> + ti,multi-phase-id = <123>; >> + >> + buck123-supply = <&vcc_3v3_sys>; >> + buck4-supply = <&vcc_3v3_sys>; >> + buck5-supply = <&vcc_3v3_sys>; >> + ldo1-supply = <&vcc_3v3_sys>; >> + ldo2-supply = <&vcc_3v3_sys>; >> + ldo3-supply = <&buck5>; >> + ldo4-supply = <&vcc_3v3_sys>; >> + >> + regulators { >> + buck123: buck123 { >> + regulator-name = "vcc_core"; >> + regulator-min-microvolt = <750000>; >> + regulator-max-microvolt = <850000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + buck4: buck4 { >> + regulator-name = "vcc_1v1"; >> + regulator-min-microvolt = <1100000>; >> + regulator-max-microvolt = <1100000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + buck5: buck5 { >> + regulator-name = "vcc_1v8_sys"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo1: ldo1 { >> + regulator-name = "vddshv5_sdio"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo2: ldo2 { >> + regulator-name = "vpp_1v8"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo3: ldo3 { >> + regulator-name = "vcc_0v85"; >> + regulator-min-microvolt = <850000>; >> + regulator-max-microvolt = <850000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo4: ldo4 { >> + regulator-name = "vdda_1v8"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + }; >> + }; >> + }; >> -- >> 2.37.3 >>
On 20/03/2023 17:35, Julien Panis wrote: > > > On 3/20/23 16:53, Rob Herring wrote: >> On Wed, Mar 15, 2023 at 12:07:33PM +0100, Julien Panis wrote: >>> TPS6594 is a Power Management IC which provides regulators and others >>> features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and >>> PFSM (Pre-configurable Finite State Machine) managing the state of the >>> device. >>> TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. >> As mentioned, the binding needs to be complete. It's missing GPIO at >> least. RTC and watchdog may or may not need binding changes. > > Thank you for your feedback. > > About GPIO, do you speak about 'gpio-controller' > and/or '#gpio-cells' properties ? Yes. > For RTC (and for watchdog, once the driver will be > implemented), our driver do not require any node > to work. What could make an explicit instantiation > necessary in DT ? Properties from RTC schema, e.g. start-year, wakeup etc. >>> + ti,spmi-controller: >>> + type: boolean >>> + description: | >>> + Identify the primary PMIC on SPMI bus. >> Perhaps the property name should include 'primary' and 'pmic'. >> Otherwise, it looks like it is just marked as 'a SPMI controller'. > > Including 'primary' and 'pmic' will be more understandable indeed. > I will change that in v3. > >> >> >>> + A multi-PMIC synchronization scheme is implemented in the PMIC device >>> + to synchronize the power state changes with other PMIC devices. This is >>> + accomplished through a SPMI bus: the primary PMIC is the controller >>> + device on the SPMI bus, and the secondary PMICs are the target devices >>> + on the SPMI bus. >> Is this a TI specific feature? > > I don't think so. I will double-check that. > If not, shall I remove the 'ti,' prefix ? Somehow reminds me qcom,bus-id, but the wording and code are not exactly the same. The question here is whether this is generic feature of all SPMI devices or PMICs, or device specific. If it is generic, then naming and type should be chosen a bit more carefully and then indeed skip "ti," prefix. > >> >>> + >>> + system-power-controller: true >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + ti,multi-phase-id: >>> + description: | >>> + Describes buck multi-phase configuration, if any. For instance, XY id means >>> + that outputs of buck converters X and Y are combined in multi-phase mode. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [12, 34, 123, 1234] >> coupled regulator stuff doesn't work here? > > Coupled regulator stuff works here. > Is it also necessary to specify some 'allOf' logic here to ensure > that mutual exclusions described below (for regulators) will be > applied ? None of other regulators do it but you could add something. Best regards, Krzysztof
On 21/03/2023 10:03, Julien Panis wrote: > > > On 3/21/23 08:36, Krzysztof Kozlowski wrote: >> On 20/03/2023 17:35, Julien Panis wrote: >>> >>> On 3/20/23 16:53, Rob Herring wrote: >>>> On Wed, Mar 15, 2023 at 12:07:33PM +0100, Julien Panis wrote: >>>>> TPS6594 is a Power Management IC which provides regulators and others >>>>> features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and >>>>> PFSM (Pre-configurable Finite State Machine) managing the state of the >>>>> device. >>>>> TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. >>>> As mentioned, the binding needs to be complete. It's missing GPIO at >>>> least. RTC and watchdog may or may not need binding changes. >>> Thank you for your feedback. >>> >>> About GPIO, do you speak about 'gpio-controller' >>> and/or '#gpio-cells' properties ? >> Yes. >> >>> For RTC (and for watchdog, once the driver will be >>> implemented), our driver do not require any node >>> to work. What could make an explicit instantiation >>> necessary in DT ? >> Properties from RTC schema, e.g. start-year, wakeup etc. > > TPS6594 RTC driver is being reviewed (this is another patch > series, not merged yet). These properties are not used by our > driver, that's why we did not have to add some RTC node in > the DT (until now, using such properties in our driver was not > requested by RTC sub-system maintainers). Bindings should be complete, regardless whether you now need this in driver or not. Does your comment mean that you will never need these, because hardware does not support them, and never going to add? Otherwise I don't get why you refer to driver when we talk about bindings... Best regards, Krzysztof
On 3/21/23 11:32, Krzysztof Kozlowski wrote: > On 21/03/2023 10:03, Julien Panis wrote: >> >> On 3/21/23 08:36, Krzysztof Kozlowski wrote: >>> On 20/03/2023 17:35, Julien Panis wrote: >>>> On 3/20/23 16:53, Rob Herring wrote: >>>>> On Wed, Mar 15, 2023 at 12:07:33PM +0100, Julien Panis wrote: >>>>>> TPS6594 is a Power Management IC which provides regulators and others >>>>>> features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and >>>>>> PFSM (Pre-configurable Finite State Machine) managing the state of the >>>>>> device. >>>>>> TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. >>>>> As mentioned, the binding needs to be complete. It's missing GPIO at >>>>> least. RTC and watchdog may or may not need binding changes. >>>> Thank you for your feedback. >>>> >>>> About GPIO, do you speak about 'gpio-controller' >>>> and/or '#gpio-cells' properties ? >>> Yes. >>> >>>> For RTC (and for watchdog, once the driver will be >>>> implemented), our driver do not require any node >>>> to work. What could make an explicit instantiation >>>> necessary in DT ? >>> Properties from RTC schema, e.g. start-year, wakeup etc. >> TPS6594 RTC driver is being reviewed (this is another patch >> series, not merged yet). These properties are not used by our >> driver, that's why we did not have to add some RTC node in >> the DT (until now, using such properties in our driver was not >> requested by RTC sub-system maintainers). > Bindings should be complete, regardless whether you now need this in > driver or not. Does your comment mean that you will never need these, > because hardware does not support them, and never going to add? > Otherwise I don't get why you refer to driver when we talk about bindings... OK, I understand now (I misinterpreted "RTC and watchdog may or may not need binding changes"). > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml new file mode 100644 index 000000000000..18f47cd6a2f9 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml @@ -0,0 +1,191 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/ti,tps6594.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI TPS6594 Power Management Integrated Circuit + +maintainers: + - Julien Panis <jpanis@baylibre.com> + +description: | + TPS6594 is a Power Management IC which provides regulators and others + features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and + PFSM (Pre-configurable Finite State Machine) managing the state of the device. + TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. + +properties: + compatible: + enum: + - ti,lp8764x + - ti,tps6593 + - ti,tps6594 + + reg: + description: I2C slave address or SPI chip select number. + maxItems: 1 + + ti,spmi-controller: + type: boolean + description: | + Identify the primary PMIC on SPMI bus. + A multi-PMIC synchronization scheme is implemented in the PMIC device + to synchronize the power state changes with other PMIC devices. This is + accomplished through a SPMI bus: the primary PMIC is the controller + device on the SPMI bus, and the secondary PMICs are the target devices + on the SPMI bus. + + system-power-controller: true + + interrupts: + maxItems: 1 + + ti,multi-phase-id: + description: | + Describes buck multi-phase configuration, if any. For instance, XY id means + that outputs of buck converters X and Y are combined in multi-phase mode. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [12, 34, 123, 1234] + + regulators: + type: object + description: List of regulators provided by this controller. + + patternProperties: + "^buck([1-5]|12|34|123|1234)$": + type: object + $ref: /schemas/regulator/regulator.yaml# + + unevaluatedProperties: false + + "^ldo[1-4]$": + type: object + $ref: /schemas/regulator/regulator.yaml# + + unevaluatedProperties: false + + allOf: + - if: + required: + - buck12 + then: + properties: + buck123: false + buck1234: false + - if: + required: + - buck123 + then: + properties: + buck34: false + - if: + required: + - buck1234 + then: + properties: + buck34: false + + additionalProperties: false + +patternProperties: + "^buck([1-5]|12|34|123|1234)-supply$": + description: Input supply phandle for each buck. + + "^ldo[1-4]-supply$": + description: Input supply phandle for each ldo. + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + tps6593: pmic@48 { + compatible = "ti,tps6593"; + reg = <0x48>; + ti,spmi-controller; + system-power-controller; + + pinctrl-names = "default"; + pinctrl-0 = <&pmic_irq_pins_default>; + interrupt-parent = <&mcu_gpio0>; + interrupts = <0 IRQ_TYPE_EDGE_FALLING>; + + ti,multi-phase-id = <123>; + + buck123-supply = <&vcc_3v3_sys>; + buck4-supply = <&vcc_3v3_sys>; + buck5-supply = <&vcc_3v3_sys>; + ldo1-supply = <&vcc_3v3_sys>; + ldo2-supply = <&vcc_3v3_sys>; + ldo3-supply = <&buck5>; + ldo4-supply = <&vcc_3v3_sys>; + + regulators { + buck123: buck123 { + regulator-name = "vcc_core"; + regulator-min-microvolt = <750000>; + regulator-max-microvolt = <850000>; + regulator-boot-on; + regulator-always-on; + }; + + buck4: buck4 { + regulator-name = "vcc_1v1"; + regulator-min-microvolt = <1100000>; + regulator-max-microvolt = <1100000>; + regulator-boot-on; + regulator-always-on; + }; + + buck5: buck5 { + regulator-name = "vcc_1v8_sys"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo1: ldo1 { + regulator-name = "vddshv5_sdio"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo2: ldo2 { + regulator-name = "vpp_1v8"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo3: ldo3 { + regulator-name = "vcc_0v85"; + regulator-min-microvolt = <850000>; + regulator-max-microvolt = <850000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo4: ldo4 { + regulator-name = "vdda_1v8"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-boot-on; + regulator-always-on; + }; + }; + }; + };
TPS6594 is a Power Management IC which provides regulators and others features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and PFSM (Pre-configurable Finite State Machine) managing the state of the device. TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. Signed-off-by: Julien Panis <jpanis@baylibre.com> --- .../devicetree/bindings/mfd/ti,tps6594.yaml | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/ti,tps6594.yaml