| Message ID | 20230216114410.183489-2-jpanis@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | TI TPS6594 PMIC support (Core, ESM, PFSM) | expand |
On 2/17/23 10:06, Krzysztof Kozlowski wrote: > On 16/02/2023 12:44, Julien Panis wrote: >> TPS6594 is a Power Management IC which provides regulators and others > Subject: drop second/last, redundant "DT bindings for". The > "dt-bindings" prefix is already stating that these are bindings. > > >> 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 | 164 ++++++++++++++++++ >> 1 file changed, 164 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..37968d6c0420 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >> @@ -0,0 +1,164 @@ >> +# 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,tps6594 >> + - ti,tps6593 >> + - ti,lp8764x > Any particular choice of ordering (different than alphabetical)? Thank you for the review. I chose this ordering because it emphasizes the fact that tps6593 and lp8764x are derivatives of tps6594 : tps6593 is nearly the same (a minor feature is not supported), and lp8764x has less resources (less bucks/LDO, and no RTC). Besides, a multi-PMIC synchronization scheme is implemented in the PMIC device to synchronize the power state changes with other PMIC devices. This is done through a SPMI bus : the master PMIC is the controller device on the SPMI bus, and the slave PMICs are the target devices on the SPMI bus. For the 5 boards we work on (for which device trees will be sent in another patch series): - tps6594 is used on 3 boards and is always master (multi-PMIC config) - tps6593 is used on 1 board and is master (single-PMIC config) - lp8764x is used on 2 boards and is always slave (multi-PMIC config) There might not be situations in which lp8764x would be master and tps6594 or tps6593 would be slave. That's why I preferred this ordering. Do you think that alphabetical order would be better ? > >> + >> + reg: >> + description: I2C slave address or SPI chip select number. >> + maxItems: 1 >> + >> + ti,use-crc: >> + type: boolean >> + description: If true, use CRC for I2C and SPI interface protocols. > Hm, why different boards would like to enable or disable it? Why this > suits DT? You're right. Reading your comment, it appears to me that CRC feature is not fully related to HW description and should not be set in DT. CRC is not 'fully' related to HW, but... For CRC feature as well, PMICs are synchronized (for boards with multi-PMIC config). To use CRC mode, this feature must be requested explicitly on the master PMIC through I2C or SPI driver, then it is enabled for the slave PMICs through SPMI bus: that sync is performed 'automatically', without intervention from the I2C or SPI driver to enable CRC on slave PMICs. As a consequence, CRC feature is enabled for all PMICs at I2C/SPI driver probe, or it is let disabled for all PMICs. But it can't be enabled for one PMIC and disabled for another one. This will probably rediscussed for I2C/SPI drivers, but do you think that a 'use_crc' driver parameter would be an acceptable solution ? If so, the master PMIC would have to be identified, so that the driver can explicitly enable CRC mode for this one if 'use_crc' is true. With this solution, some 'ti,is-master;' bool property would be necessary. > >> + >> + 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] >> + >> +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. >> + >> + regulators: > This should go to properties, not patternProperties. > >> + 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 >> + > You could add here - on this level - of indentation allOf:if for > excluding setups > > if: > required: > - buck12 > then: > properties: > buck123: false > buck1234: false > > Or, if you want to require regulator then: > oneOf: > - required: > - buck12 > - required: > - buck123 > - required: > - buck1234 > > and anyway exclude buck34 with two above. I am not sure that we have the same understanding of the multi-phase setup. Maybe the description I wrote is not clear enough (?) Or I just don't understand what you mean exactly. How would you combine outputs of bucks 3 and 4 ? We use 'buck34' property to mean that: - buck1 output is mono-phase, - buck2 output is mono-phase, - buck3 and buck4 outputs are combined (i.e. multi-phases). This weird configuration is supported by these PMICs. Using a PMIC without using the provided regulators does not seem very interesting indeed. But strictly speaking, these regulators are not required. One could use some others resources provided by the PMIC (the Error Signal Monitor device for instance). Besides, multi-phase mode depends on the chosen design and is not required for all situations. > >> + additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + >> +additionalProperties: false > > > Best regards, > Krzysztof >
On 17/02/2023 13:10, Julien Panis wrote: > > On 2/17/23 10:06, Krzysztof Kozlowski wrote: >> On 16/02/2023 12:44, Julien Panis wrote: >>> TPS6594 is a Power Management IC which provides regulators and others >> Subject: drop second/last, redundant "DT bindings for". The >> "dt-bindings" prefix is already stating that these are bindings. >> >> >>> 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 | 164 ++++++++++++++++++ >>> 1 file changed, 164 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..37968d6c0420 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >>> @@ -0,0 +1,164 @@ >>> +# 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,tps6594 >>> + - ti,tps6593 >>> + - ti,lp8764x >> Any particular choice of ordering (different than alphabetical)? > > Thank you for the review. > > I chose this ordering because it emphasizes the fact that tps6593 and > lp8764x > are derivatives of tps6594 : tps6593 is nearly the same (a minor feature > is not > supported), and lp8764x has less resources (less bucks/LDO, and no RTC). > > Besides, a multi-PMIC synchronization scheme is implemented in the PMIC > device > to synchronize the power state changes with other PMIC devices. This is done > through a SPMI bus : the master PMIC is the controller device on the > SPMI bus, > and the slave PMICs are the target devices on the SPMI bus. For the 5 boards > we work on (for which device trees will be sent in another patch series): > - tps6594 is used on 3 boards and is always master (multi-PMIC config) > - tps6593 is used on 1 board and is master (single-PMIC config) > - lp8764x is used on 2 boards and is always slave (multi-PMIC config) > There might not be situations in which lp8764x would be master and tps6594 > or tps6593 would be slave. > > That's why I preferred this ordering. > > Do you think that alphabetical order would be better ? It's simpler (requires no knowledge about chips) and reduces the future conflicts. It's fine to keep it also ordered like you said, although I wonder how other people adding new compatibles here will figure it out... > >> >>> + >>> + reg: >>> + description: I2C slave address or SPI chip select number. >>> + maxItems: 1 >>> + >>> + ti,use-crc: >>> + type: boolean >>> + description: If true, use CRC for I2C and SPI interface protocols. >> Hm, why different boards would like to enable or disable it? Why this >> suits DT? > > You're right. Reading your comment, it appears to me that CRC feature is > not fully > related to HW description and should not be set in DT. > > CRC is not 'fully' related to HW, but... > For CRC feature as well, PMICs are synchronized (for boards with > multi-PMIC config). > To use CRC mode, this feature must be requested explicitly on the master > PMIC > through I2C or SPI driver, then it is enabled for the slave PMICs > through SPMI bus: that > sync is performed 'automatically', without intervention from the I2C or > SPI driver to > enable CRC on slave PMICs. > As a consequence, CRC feature is enabled for all PMICs at I2C/SPI driver > probe, > or it is let disabled for all PMICs. But it can't be enabled for one > PMIC and disabled > for another one. > > This will probably rediscussed for I2C/SPI drivers, but do you think > that a 'use_crc' > driver parameter would be an acceptable solution ? If so, the master > PMIC would have > to be identified, so that the driver can explicitly enable CRC mode for > this one if > 'use_crc' is true. With this solution, some 'ti,is-master;' bool > property would be necessary. It looks the property should be only in the drivers, not in the DT. > >> >>> + >>> + 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] >>> + >>> +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. >>> + >>> + regulators: >> This should go to properties, not patternProperties. >> >>> + 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 >>> + >> You could add here - on this level - of indentation allOf:if for >> excluding setups >> >> if: >> required: >> - buck12 >> then: >> properties: >> buck123: false >> buck1234: false >> >> Or, if you want to require regulator then: >> oneOf: >> - required: >> - buck12 >> - required: >> - buck123 >> - required: >> - buck1234 >> >> and anyway exclude buck34 with two above. > > I am not sure that we have the same understanding of the multi-phase setup. > Maybe the description I wrote is not clear enough (?) Or I just don't > understand > what you mean exactly. > > How would you combine outputs of bucks 3 and 4 ? No one discusses here changing this... > We use 'buck34' property to mean that: > - buck1 output is mono-phase, > - buck2 output is mono-phase, > - buck3 and buck4 outputs are combined (i.e. multi-phases). > This weird configuration is supported by these PMICs. > > Using a PMIC without using the provided regulators does not seem very > interesting > indeed. > But strictly speaking, these regulators are not required. One could use > some others > resources provided by the PMIC (the Error Signal Monitor device for > instance). Then the first method. > Besides, multi-phase mode depends on the chosen design and is not > required for > all situations. Sorry, I don't think it is related to the topic I proposed. Best regards, Krzysztof
On 2/21/23 12:17, Krzysztof Kozlowski wrote: > On 17/02/2023 13:10, Julien Panis wrote: >> On 2/17/23 10:06, Krzysztof Kozlowski wrote: >>> On 16/02/2023 12:44, Julien Panis wrote: >>>> TPS6594 is a Power Management IC which provides regulators and others >>> Subject: drop second/last, redundant "DT bindings for". The >>> "dt-bindings" prefix is already stating that these are bindings. >>> >>> >>>> 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 | 164 ++++++++++++++++++ >>>> 1 file changed, 164 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..37968d6c0420 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >>>> @@ -0,0 +1,164 @@ >>>> +# 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,tps6594 >>>> + - ti,tps6593 >>>> + - ti,lp8764x >>> Any particular choice of ordering (different than alphabetical)? >> Thank you for the review. >> >> I chose this ordering because it emphasizes the fact that tps6593 and >> lp8764x >> are derivatives of tps6594 : tps6593 is nearly the same (a minor feature >> is not >> supported), and lp8764x has less resources (less bucks/LDO, and no RTC). >> >> Besides, a multi-PMIC synchronization scheme is implemented in the PMIC >> device >> to synchronize the power state changes with other PMIC devices. This is done >> through a SPMI bus : the master PMIC is the controller device on the >> SPMI bus, >> and the slave PMICs are the target devices on the SPMI bus. For the 5 boards >> we work on (for which device trees will be sent in another patch series): >> - tps6594 is used on 3 boards and is always master (multi-PMIC config) >> - tps6593 is used on 1 board and is master (single-PMIC config) >> - lp8764x is used on 2 boards and is always slave (multi-PMIC config) >> There might not be situations in which lp8764x would be master and tps6594 >> or tps6593 would be slave. >> >> That's why I preferred this ordering. >> >> Do you think that alphabetical order would be better ? > It's simpler (requires no knowledge about chips) and reduces the future > conflicts. It's fine to keep it also ordered like you said, although I > wonder how other people adding new compatibles here will figure it out... I will reorder it alphabetically in v2. > >>>> + >>>> + reg: >>>> + description: I2C slave address or SPI chip select number. >>>> + maxItems: 1 >>>> + >>>> + ti,use-crc: >>>> + type: boolean >>>> + description: If true, use CRC for I2C and SPI interface protocols. >>> Hm, why different boards would like to enable or disable it? Why this >>> suits DT? >> You're right. Reading your comment, it appears to me that CRC feature is >> not fully >> related to HW description and should not be set in DT. >> >> CRC is not 'fully' related to HW, but... >> For CRC feature as well, PMICs are synchronized (for boards with >> multi-PMIC config). >> To use CRC mode, this feature must be requested explicitly on the master >> PMIC >> through I2C or SPI driver, then it is enabled for the slave PMICs >> through SPMI bus: that >> sync is performed 'automatically', without intervention from the I2C or >> SPI driver to >> enable CRC on slave PMICs. >> As a consequence, CRC feature is enabled for all PMICs at I2C/SPI driver >> probe, >> or it is let disabled for all PMICs. But it can't be enabled for one >> PMIC and disabled >> for another one. >> >> This will probably rediscussed for I2C/SPI drivers, but do you think >> that a 'use_crc' >> driver parameter would be an acceptable solution ? If so, the master >> PMIC would have >> to be identified, so that the driver can explicitly enable CRC mode for >> this one if >> 'use_crc' is true. With this solution, some 'ti,is-master;' bool >> property would be necessary. > It looks the property should be only in the drivers, not in the DT. I will remove 'ti,use-crc;' property from the DT. This will be only in the driver. Do you also consider that a property such as 'ti,is-secondary-pmic;' would not be acceptable either ? From driver point of view, this primary/secondary role on SPMI bus is a 'built-in' property of the PMIC (CRC must be enabled only via primary PMIC but using the primary PMIC does not imply that CRC is necessarily used). >>>> + >>>> + 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] >>>> + >>>> +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. >>>> + >>>> + regulators: >>> This should go to properties, not patternProperties. >>> >>>> + 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 >>>> + >>> You could add here - on this level - of indentation allOf:if for >>> excluding setups >>> >>> if: >>> required: >>> - buck12 >>> then: >>> properties: >>> buck123: false >>> buck1234: false >>> >>> Or, if you want to require regulator then: >>> oneOf: >>> - required: >>> - buck12 >>> - required: >>> - buck123 >>> - required: >>> - buck1234 >>> >>> and anyway exclude buck34 with two above. >> I am not sure that we have the same understanding of the multi-phase setup. >> Maybe the description I wrote is not clear enough (?) Or I just don't >> understand >> what you mean exactly. >> >> How would you combine outputs of bucks 3 and 4 ? > No one discusses here changing this... > >> We use 'buck34' property to mean that: >> - buck1 output is mono-phase, >> - buck2 output is mono-phase, >> - buck3 and buck4 outputs are combined (i.e. multi-phases). >> This weird configuration is supported by these PMICs. >> >> Using a PMIC without using the provided regulators does not seem very >> interesting >> indeed. >> But strictly speaking, these regulators are not required. One could use >> some others >> resources provided by the PMIC (the Error Signal Monitor device for >> instance). > Then the first method. OK. Regarding buck34, it might be unnecessary and could finally be removed in v2. If we keep it, my understanding of your suggestion is: allOf: - if: required: - buck12 then: properties: buck123: false buck1234: false - if: required: - buck123 then: properties: buck34: false - if: required: - buck1234 then: properties: buck34: false > >> Besides, multi-phase mode depends on the chosen design and is not >> required for >> all situations. > Sorry, I don't think it is related to the topic I proposed. > > > Best regards, > Krzysztof > Julien
On 2/21/23 16:01, Krzysztof Kozlowski wrote: > On 21/02/2023 15:49, Julien Panis wrote: >> >> On 2/21/23 12:17, Krzysztof Kozlowski wrote: >>> On 17/02/2023 13:10, Julien Panis wrote: >>>> On 2/17/23 10:06, Krzysztof Kozlowski wrote: >>>>> On 16/02/2023 12:44, Julien Panis wrote: >>>>>> TPS6594 is a Power Management IC which provides regulators and others >>>>> Subject: drop second/last, redundant "DT bindings for". The >>>>> "dt-bindings" prefix is already stating that these are bindings. >>>>> >>>>> >>>>>> 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 | 164 ++++++++++++++++++ >>>>>> 1 file changed, 164 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..37968d6c0420 >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >>>>>> @@ -0,0 +1,164 @@ >>>>>> +# 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,tps6594 >>>>>> + - ti,tps6593 >>>>>> + - ti,lp8764x >>>>> Any particular choice of ordering (different than alphabetical)? >>>> Thank you for the review. >>>> >>>> I chose this ordering because it emphasizes the fact that tps6593 and >>>> lp8764x >>>> are derivatives of tps6594 : tps6593 is nearly the same (a minor feature >>>> is not >>>> supported), and lp8764x has less resources (less bucks/LDO, and no RTC). >>>> >>>> Besides, a multi-PMIC synchronization scheme is implemented in the PMIC >>>> device >>>> to synchronize the power state changes with other PMIC devices. This is done >>>> through a SPMI bus : the master PMIC is the controller device on the >>>> SPMI bus, >>>> and the slave PMICs are the target devices on the SPMI bus. For the 5 boards >>>> we work on (for which device trees will be sent in another patch series): >>>> - tps6594 is used on 3 boards and is always master (multi-PMIC config) >>>> - tps6593 is used on 1 board and is master (single-PMIC config) >>>> - lp8764x is used on 2 boards and is always slave (multi-PMIC config) >>>> There might not be situations in which lp8764x would be master and tps6594 >>>> or tps6593 would be slave. >>>> >>>> That's why I preferred this ordering. >>>> >>>> Do you think that alphabetical order would be better ? >>> It's simpler (requires no knowledge about chips) and reduces the future >>> conflicts. It's fine to keep it also ordered like you said, although I >>> wonder how other people adding new compatibles here will figure it out... >> I will reorder it alphabetically in v2. >> >>>>>> + >>>>>> + reg: >>>>>> + description: I2C slave address or SPI chip select number. >>>>>> + maxItems: 1 >>>>>> + >>>>>> + ti,use-crc: >>>>>> + type: boolean >>>>>> + description: If true, use CRC for I2C and SPI interface protocols. >>>>> Hm, why different boards would like to enable or disable it? Why this >>>>> suits DT? >>>> You're right. Reading your comment, it appears to me that CRC feature is >>>> not fully >>>> related to HW description and should not be set in DT. >>>> >>>> CRC is not 'fully' related to HW, but... >>>> For CRC feature as well, PMICs are synchronized (for boards with >>>> multi-PMIC config). >>>> To use CRC mode, this feature must be requested explicitly on the master >>>> PMIC >>>> through I2C or SPI driver, then it is enabled for the slave PMICs >>>> through SPMI bus: that >>>> sync is performed 'automatically', without intervention from the I2C or >>>> SPI driver to >>>> enable CRC on slave PMICs. >>>> As a consequence, CRC feature is enabled for all PMICs at I2C/SPI driver >>>> probe, >>>> or it is let disabled for all PMICs. But it can't be enabled for one >>>> PMIC and disabled >>>> for another one. >>>> >>>> This will probably rediscussed for I2C/SPI drivers, but do you think >>>> that a 'use_crc' >>>> driver parameter would be an acceptable solution ? If so, the master >>>> PMIC would have >>>> to be identified, so that the driver can explicitly enable CRC mode for >>>> this one if >>>> 'use_crc' is true. With this solution, some 'ti,is-master;' bool >>>> property would be necessary. >>> It looks the property should be only in the drivers, not in the DT. >> I will remove 'ti,use-crc;' property from the DT. This will be only in >> the driver. >> Do you also consider that a property such as 'ti,is-secondary-pmic;' >> would not be acceptable either ? From driver point of view, this >> primary/secondary role on SPMI bus is a 'built-in' property of the >> PMIC (CRC must be enabled only via primary PMIC but using the >> primary PMIC does not imply that CRC is necessarily used). > Depends, I am not sure. Are the PMICs in some kind of hierarchical > topology? Like one is parent of another? If not (so both are > parallel/equal children of SPMI bus), then some property to indicate > which one is the main PMIC makes sense. There is no hierarchical topology. So, I will consider identifying in DT which one is the main PMIC. (...) Julien
diff --git a/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml new file mode 100644 index 000000000000..37968d6c0420 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml @@ -0,0 +1,164 @@ +# 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,tps6594 + - ti,tps6593 + - ti,lp8764x + + reg: + description: I2C slave address or SPI chip select number. + maxItems: 1 + + ti,use-crc: + type: boolean + description: If true, use CRC for I2C and SPI interface protocols. + + 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] + +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. + + 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 + + additionalProperties: false + +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,use-crc; + 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 | 164 ++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/ti,tps6594.yaml