Re: [PATCH v2 02/17] regulator: dt-bindings: add s2mpg10-pmic regulators

["André Draszik" <andre.draszik@linaro.org>]

Hi Krzysztof,

On Wed, 2025-06-11 at 10:55 +0200, Krzysztof Kozlowski wrote:
> On Fri, Jun 06, 2025 at 04:02:58PM GMT, André Draszik wrote:
> > The S2MPG10 PMIC is a Power Management IC for mobile applications with
> > buck converters, various LDOs, power meters, RTC, clock outputs, and
> > additional GPIO interfaces.
> > 
> > It has 10 buck and 31 LDO rails. Several of these can either be
> > controlled via software or via external signals, e.g. input pins
> > connected to a main processor's GPIO pins.
> > 
> > Add documentation related to the regulator (buck & ldo) parts like
> > devicetree definitions, regulator naming patterns, and additional
> > properties.
> > 
> > S2MPG10 is typically used as the main-PMIC together with an S2MPG11
> > PMIC in a main/sub configuration, hence the datasheet and the binding
> > both suffix the rails with an 'm'.
> > 
> > Signed-off-by: André Draszik <andre.draszik@linaro.org>
> > 
> > ---
> > v2:
> > - drop | (literal style mark) from samsung,ext-control-gpios
> >   description
> > ---
> >  .../regulator/samsung,s2mpg10-regulator.yaml       | 147 +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  .../regulator/samsung,s2mpg10-regulator.h          |  48 +++++++
> >  3 files changed, 196 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/regulator/samsung,s2mpg10-regulator.yaml
> > b/Documentation/devicetree/bindings/regulator/samsung,s2mpg10-regulator.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..82f2b06205e9bdb15cf90b1e896fe52c335c52c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/samsung,s2mpg10-regulator.yaml
> > @@ -0,0 +1,147 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/regulator/samsung,s2mpg10-regulator.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Samsung S2MPG10 Power Management IC regulators
> > +
> > +maintainers:
> > +  - André Draszik <andre.draszik@linaro.org>
> > +
> > +description: |
> > +  This is part of the device tree bindings for the S2MG10 Power Management IC
> > +  (PMIC).
> > +
> > +  The S2MPG10 PMIC provides 10 buck and 31 LDO regulators.
> > +
> > +  See also Documentation/devicetree/bindings/mfd/samsung,s2mps11.yaml for
> > +  additional information and example.
> > +
> > +definitions:
> > +  s2mpg10-ext-control:
> > +    properties:
> > +      samsung,ext-control:
> > +        description: |
> > +          These rails can be controlled via one of several possible external
> > +          (hardware) signals. If so, this property configures the signal the PMIC
> > +          should monitor. For S2MPG10 rails where external control is possible other
> > +          than ldo20m, the following values generally corresponding to the
> > +          respective on-chip pin are valid:
> > +            - 0 # S2MPG10_PCTRLSEL_ON - always on
> > +            - 1 # S2MPG10_PCTRLSEL_PWREN - PWREN pin
> > +            - 2 # S2MPG10_PCTRLSEL_PWREN_TRG - PWREN_TRG bit in MIMICKING_CTRL
> > +            - 3 # S2MPG10_PCTRLSEL_PWREN_MIF - PWREN_MIF pin
> > +            - 4 # S2MPG10_PCTRLSEL_PWREN_MIF_TRG - PWREN_MIF_TRG bit in MIMICKING_CTRL
> > +            - 5 # S2MPG10_PCTRLSEL_AP_ACTIVE_N - ~AP_ACTIVE_N pin
> > +            - 6 # S2MPG10_PCTRLSEL_AP_ACTIVE_N_TRG - ~AP_ACTIVE_N_TRG bit in MIMICKING_CTRL
> > +            - 7 # S2MPG10_PCTRLSEL_CPUCL1_EN - CPUCL1_EN pin
> > +            - 8 # S2MPG10_PCTRLSEL_CPUCL1_EN2 - CPUCL1_EN & PWREN pins
> > +            - 9 # S2MPG10_PCTRLSEL_CPUCL2_EN - CPUCL2_EN pin
> > +            - 10 # S2MPG10_PCTRLSEL_CPUCL2_EN2 - CPUCL2_E2 & PWREN pins
> > +            - 11 # S2MPG10_PCTRLSEL_TPU_EN - TPU_EN pin
> > +            - 12 # S2MPG10_PCTRLSEL_TPU_EN2 - TPU_EN & ~AP_ACTIVE_N pins
> > +            - 13 # S2MPG10_PCTRLSEL_TCXO_ON - TCXO_ON pin
> > +            - 14 # S2MPG10_PCTRLSEL_TCXO_ON2 - TCXO_ON & ~AP_ACTIVE_N pins
> > +
> > +          For S2MPG10 ldo20m, the following values are valid
> > +            - 0 # S2MPG10_PCTRLSEL_LDO20M_ON - always on
> 
> No, use standard regulator properties - regulator-always-on
> 
> 
> > +            - 1 # S2MPG10_PCTRLSEL_LDO20M_EN_SFR - VLDO20M_EN & LDO20M_SFR
> > +            - 2 # S2MPG10_PCTRLSEL_LDO20M_EN - VLDO20M_EN pin
> > +            - 3 # S2MPG10_PCTRLSEL_LDO20M_SFR - LDO20M_SFR in LDO_CTRL1 register
> > +            - 4 # S2MPG10_PCTRLSEL_LDO20M_OFF - disable
> 
> I don't think we allowed such property in the past.

I've done some more investigation now - the reason we need to configure
control of rails via signals (i.e. input pin on S2MPG1x) is that the PMU
and power domains in particular control at least some of them.

As an example, power domain g3d disable toggles an output pin on GS101,
which is connected to the G3D_EN pin on S2MPG1x on Pixel. The regulator
driver needs to configure all the G3D-related-PMIC rails to react to this
signal. There a) is a large amount of flexibility as to which rail should
react to which signal, and b) the bootloader doesn't configure (all of)
them.

Therefore, we need to be able to specify which rail should be controlled
by which signal, both in DT and in the driver.

The alternative would be do add explicit (driver-based) regulator control
for each power domain, rather than having the PMU handle this. Such an
approach appears suboptimal, because after all that's what the PMU is for.

Additionally, there are sequencing requirements on enabling/disabling rails
and when using the signals, the PMIC will ensure they're followed, whereas
a driver would have to duplicate that information and could get it a) wrong,
b) would use more CPU cycles due to additional code, and c) leave the rail
on for longer than necessarily due to timer resolution.

Also, it might not work in all cases, e.g. if the PMU disables the rail for
the CPU, the Linux driver can not afterwards disable the PMIC rail anymore,
leaving it unnecessarily enabled. Equally, the Linux driver can not disable
the rail before turning off the power domain, as once the rail is off, the
CPU/Linux can not execute any further code.


Hope the above justifies the introduction of this property :-)


Cheers,
Andre'