[v5,01/16] dt-bindings: regulator: Document ROHM BD71282 regulator bindings

Document ROHM BD71828 PMIC regulator device tree bindings.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---

No changes from v4

 .../regulator/rohm,bd71828-regulator.yaml     | 122 ++++++++++++++++++
 1 file changed, 122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml

On Mon, Nov 18, 2019 at 08:53:57AM +0200, Matti Vaittinen wrote:

> +#Supported default DVS states:
> +#buck		| run		| idle		| suspend	| lpsr
> +#----------------------------------------------------------------------------
> +#1, 2, 6, and 7	| supported	| supported	| 	supported (*)
> +#----------------------------------------------------------------------------
> +#3, 4, and 5	| 			supported (**)
> +#----------------------------------------------------------------------------
> +#(*)  LPSR and SUSPEND states use same voltage but both states have own enable /
> +#     disable settings. Voltage 0 can be specified for a state to make regulator
> +#     disabled on that state.
> +#(**) All states use same voltage but have own enable / disable settings.
> +#     Voltage 0 can be specified for a state to make regulator disabled on that
> +#     state.
> +
> +      rohm,dvs-runlvl-ctrl:
> +        description: |
> +          buck control is done based on run-level. Regulator is not
> +          individually controllable. See ../mfd/rohm,bd71828-pmic.yaml for
> +          how to specify run-level control mechanism. Only bucks 1, 2, 6
> +          and 7 support this.
> +        type: boolean

I don't think I saw this having the effect on set_voltage() that I'd
have expected in the driver?  

> +      rohm,dvs-runlevel-microvolts:
> +        minimum: 0
> +        maximum: 2000000
> +        maxItems: 4
> +        description:
> +          Array of voltages for run-levels. First value is for run-level 0,
> +          second for run-level 1 etc. Microvolts.

What's the mapping from array indexes to the names used elsewhere to
support runlevels?

Hello Mark,

It's nice to hear from you again :) I hope you're enjoying all the new
things :)

On Mon, 2019-11-18 at 16:25 +0000, Mark Brown wrote:
> On Mon, Nov 18, 2019 at 08:53:57AM +0200, Matti Vaittinen wrote:
> 
> > +#Supported default DVS states:
> > +#buck		| run		| idle		| suspend	
> > | lpsr
> > +#-----------------------------------------------------------------
> > -----------
> > +#1, 2, 6, and 7	| supported	| supported	| 	supported
> > (*)
> > +#-----------------------------------------------------------------
> > -----------
> > +#3, 4, and 5	| 			supported (**)
> > +#-----------------------------------------------------------------
> > -----------
> > +#(*)  LPSR and SUSPEND states use same voltage but both states
> > have own enable /
> > +#     disable settings. Voltage 0 can be specified for a state to
> > make regulator
> > +#     disabled on that state.
> > +#(**) All states use same voltage but have own enable / disable
> > settings.
> > +#     Voltage 0 can be specified for a state to make regulator
> > disabled on that
> > +#     state.
> > +
> > +      rohm,dvs-runlvl-ctrl:
> > +        description: |
> > +          buck control is done based on run-level. Regulator is
> > not
> > +          individually controllable. See ../mfd/rohm,bd71828-
> > pmic.yaml for
> > +          how to specify run-level control mechanism. Only bucks
> > 1, 2, 6
> > +          and 7 support this.
> > +        type: boolean
> 
> I don't think I saw this having the effect on set_voltage() that I'd
> have expected in the driver?  

The support for this is added in patch 12. I should've ordered the
patch series so that all regulator patches were one after another.
Sorry for that.
The patch 12 adds the run-level support. Please see the functions
get_runcontrolled_bucks_dt(),
mark_regulator_runlvl_controlled() (sets the g->runlvl)
and set_buck_runlvl_controlled() (called based on g->runlvl)
which changes the ops to disallow setters and to get voltage based on
current runlevel - and different ops depending on if runlevels are
controlled by GPIO or I2C. Additionally set_buck_runlvl_controlled()
adds DT parsing call-back for setting the initial voltages.

Complex, I know. I'm open to hints,tips and suggestions :)

> > +      rohm,dvs-runlevel-microvolts:
> > +        minimum: 0
> > +        maximum: 2000000
> > +        maxItems: 4
> > +        description:
> > +          Array of voltages for run-levels. First value is for
> > run-level 0,
> > +          second for run-level 1 etc. Microvolts.
> 
> What's the mapping from array indexes to the names used elsewhere to
> support runlevels?

Hmm. Sorry Mark, I don't think I follow your question. Do you mean
names like LPSR, SUSPEND, IDLE, RUN? If so, then I might need to
rephrase this. The runlevels referred here are different from LPSR,
SUSPEND, IDLE etc. They are actually 'sub-levels' for PMIC's RUN state.
Eg, kind of a 'fast way' to change voltages for multiple power rails
when SoC is at RUN state. The names I have seen are RUN0, RUN1, RUN2
and RUN3. That mapping is described in description above.

Br,
	Matti Vaittinen

On Mon, Nov 18, 2019 at 06:03:42PM +0000, Vaittinen, Matti wrote:

> It's nice to hear from you again :) I hope you're enjoying all the new
> things :)

:)

> On Mon, 2019-11-18 at 16:25 +0000, Mark Brown wrote:
> > On Mon, Nov 18, 2019 at 08:53:57AM +0200, Matti Vaittinen wrote:

> > I don't think I saw this having the effect on set_voltage() that I'd
> > have expected in the driver?  

> The support for this is added in patch 12. I should've ordered the
> patch series so that all regulator patches were one after another.
> Sorry for that.
> The patch 12 adds the run-level support. Please see the functions
> get_runcontrolled_bucks_dt(),
> mark_regulator_runlvl_controlled() (sets the g->runlvl)
> and set_buck_runlvl_controlled() (called based on g->runlvl)
> which changes the ops to disallow setters and to get voltage based on
> current runlevel - and different ops depending on if runlevels are
> controlled by GPIO or I2C. Additionally set_buck_runlvl_controlled()
> adds DT parsing call-back for setting the initial voltages.

Ah, OK.  I didn't even notice that patch when I scanned the series.
I'll look out for this next time around but that sounds like it's
generally going in the right direction, especially if it's integrated
with the suspend mode regulator bindings that Chunyan did.

> > > +        minimum: 0
> > > +        maximum: 2000000
> > > +        maxItems: 4
> > > +        description:
> > > +          Array of voltages for run-levels. First value is for
> > > run-level 0,
> > > +          second for run-level 1 etc. Microvolts.

> > What's the mapping from array indexes to the names used elsewhere to
> > support runlevels?

> Hmm. Sorry Mark, I don't think I follow your question. Do you mean
> names like LPSR, SUSPEND, IDLE, RUN? If so, then I might need to
> rephrase this. The runlevels referred here are different from LPSR,
> SUSPEND, IDLE etc. They are actually 'sub-levels' for PMIC's RUN state.
> Eg, kind of a 'fast way' to change voltages for multiple power rails
> when SoC is at RUN state. The names I have seen are RUN0, RUN1, RUN2
> and RUN3. That mapping is described in description above.

Yes, I think this needs clarification as I completely failed to pick up
on this and did indeed read this as referring to the modes.  "Voltages
that can be set in RUN mode" or something?  I take it these voltages are
fixed and the OS can't change them?

On Tue, 2019-11-19 at 18:13 +0000, Mark Brown wrote:
> On Mon, Nov 18, 2019 at 06:03:42PM +0000, Vaittinen, Matti wrote:
> > On Mon, 2019-11-18 at 16:25 +0000, Mark Brown wrote:
> > > On Mon, Nov 18, 2019 at 08:53:57AM +0200, Matti Vaittinen wrote:
> > > I don't think I saw this having the effect on set_voltage() that
> > > I'd
> > > have expected in the driver?  
> > The support for this is added in patch 12. I should've ordered the
> > patch series so that all regulator patches were one after another.
> > Sorry for that.
> > The patch 12 adds the run-level support. Please see the functions
> > get_runcontrolled_bucks_dt(),
> > mark_regulator_runlvl_controlled() (sets the g->runlvl)
> > and set_buck_runlvl_controlled() (called based on g->runlvl)
> > which changes the ops to disallow setters and to get voltage based
> > on
> > current runlevel - and different ops depending on if runlevels are
> > controlled by GPIO or I2C. Additionally
> > set_buck_runlvl_controlled()
> > adds DT parsing call-back for setting the initial voltages.
> 
> Ah, OK.  I didn't even notice that patch when I scanned the series.
> I'll look out for this next time around but that sounds like it's
> generally going in the right direction, especially if it's integrated
> with the suspend mode regulator bindings that Chunyan did.

Probably it is not as I am not familiar with Chunyan's work. I'll try
looking what has been done on that front :) And I am pretty sure you
might not be happy with that patch - but perhaps you can give me a
nudge to better direction...

> > > > +        minimum: 0
> > > > +        maximum: 2000000
> > > > +        maxItems: 4
> > > > +        description:
> > > > +          Array of voltages for run-levels. First value is for
> > > > run-level 0,
> > > > +          second for run-level 1 etc. Microvolts.
> > > What's the mapping from array indexes to the names used elsewhere
> > > to
> > > support runlevels?
> > Hmm. Sorry Mark, I don't think I follow your question. Do you mean
> > names like LPSR, SUSPEND, IDLE, RUN? If so, then I might need to
> > rephrase this. The runlevels referred here are different from LPSR,
> > SUSPEND, IDLE etc. They are actually 'sub-levels' for PMIC's RUN
> > state.
> > Eg, kind of a 'fast way' to change voltages for multiple power
> > rails
> > when SoC is at RUN state. The names I have seen are RUN0, RUN1,
> > RUN2
> > and RUN3. That mapping is described in description above.
> 
> Yes, I think this needs clarification as I completely failed to pick
> up
> on this and did indeed read this as referring to the
> modes.  "Voltages
> that can be set in RUN mode" or something?  I take it these voltages
> are
> fixed and the OS can't change them?

Unfortunately they are not. Voltages and enable/disable statuses for
each run-level (and individually for each run-level capable buck) can
be changed at runtime via I2C. And a customer requested me also to
support this - hence the in-kernel API - but I am sure you have some
nice words when you check the patch 12. :]

Br,
	Matti Vaittinen

On Tue, Nov 19, 2019 at 06:51:37PM +0000, Vaittinen, Matti wrote:
> On Tue, 2019-11-19 at 18:13 +0000, Mark Brown wrote:

> > Ah, OK.  I didn't even notice that patch when I scanned the series.
> > I'll look out for this next time around but that sounds like it's
> > generally going in the right direction, especially if it's integrated
> > with the suspend mode regulator bindings that Chunyan did.

> Probably it is not as I am not familiar with Chunyan's work. I'll try
> looking what has been done on that front :) And I am pretty sure you
> might not be happy with that patch - but perhaps you can give me a
> nudge to better direction...

The driver interface was added in "regulator: add PM suspend and resume
hooks".

> > Yes, I think this needs clarification as I completely failed to pick
> > up
> > on this and did indeed read this as referring to the
> > modes.  "Voltages
> > that can be set in RUN mode" or something?  I take it these voltages
> > are
> > fixed and the OS can't change them?

> Unfortunately they are not. Voltages and enable/disable statuses for
> each run-level (and individually for each run-level capable buck) can
> be changed at runtime via I2C. And a customer requested me also to
> support this - hence the in-kernel API - but I am sure you have some
> nice words when you check the patch 12. :]

Ah, that's actually better.  It opens up possiblities for making use of
the feature without encoding voltages in DT.  For example, you can cache
the last however many voltages that were set and jump quickly to them or
do something like put the top of the constraints in to help with
governors like ondemand.  I'd recommend trying for something like that
rather than encoding in DT, it'll probably be more robust with things
like cpufreq changing.

On Mon, Nov 18, 2019 at 08:53:57AM +0200, Matti Vaittinen wrote:
> Document ROHM BD71828 PMIC regulator device tree bindings.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> 
> No changes from v4
> 
>  .../regulator/rohm,bd71828-regulator.yaml     | 122 ++++++++++++++++++
>  1 file changed, 122 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml b/Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml
> new file mode 100644
> index 000000000000..c23ec4d8584b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml
> @@ -0,0 +1,122 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Please dual license new bindings:

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/rohm,bd71828-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM BD71828 Power Management Integrated Circuit regulators
> +
> +maintainers:
> +  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> +
> +description: |
> +  This module is part of the ROHM BD71828 MFD device. For more details
> +  see Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml.
> +
> +  The regulator controller is represented as a sub-node of the PMIC node
> +  on the device tree.
> +
> +  Regulator nodes should be named to BUCK_<number> and LDO_<number>.
> +  The valid names for BD71828 regulator nodes are
> +  BUCK1, BUCK2, BUCK3, BUCK4, BUCK5, BUCK6, BUCK7
> +  LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7
> +
> +patternProperties:
> +  "^LDO[1-7]$":
> +    type: object
> +    allOf:
> +      - $ref: regulator.yaml#
> +    description:
> +      Properties for single LDO regulator.
> +
> +    properties:
> +      #Is there a nice way to check the name is same as node name but lower case

Nope.

Why not make the node names lower case? That's the preference though 
the regulator binding is special.

> +      regulator-name:
> +        pattern: "^ldo[1-7]$"
> +        description:
> +          should be "ldo1", ..., "ldo7"
> +
> +  "^BUCK[1-7]$":
> +    type: object
> +    allOf:
> +      - $ref: regulator.yaml#
> +    description:
> +      Properties for single BUCK regulator.
> +
> +    properties:
> +      #Is there a nice way to check the name is same as node name but lower case
> +      regulator-name:
> +        pattern: "^buck[1-7]$"
> +        description:
> +          should be "buck1", ..., "buck7"
> +
> +      rohm,dvs-run-voltage:
> +        allOf:
> +          - $ref: "/schemas/types.yaml#/definitions/uint32"
> +          - minimum: 0
> +            maximum: 3300000
> +        description:
> +          PMIC default "RUN" state voltage in uV. See below table for
> +          bucks which support this. 0 means disabled.
> +
> +      rohm,dvs-idle-voltage:
> +        allOf:
> +          - $ref: "/schemas/types.yaml#/definitions/uint32"
> +          - minimum: 0
> +            maximum: 3300000
> +        description:
> +          PMIC default "IDLE" state voltage in uV. See below table for
> +          bucks which support this. 0 means disabled.
> +
> +      rohm,dvs-suspend-voltage:
> +        allOf:
> +          - $ref: "/schemas/types.yaml#/definitions/uint32"
> +          - minimum: 0
> +            maximum: 3300000
> +        description:
> +          PMIC default "SUSPEND" state voltage in uV. See below table for
> +          bucks which support this. 0 means disabled.
> +
> +      rohm,dvs-lpsr-voltage:
> +        allOf:
> +          - $ref: "/schemas/types.yaml#/definitions/uint32"
> +          - minimum: 0
> +            maximum: 3300000
> +        description:
> +          PMIC default "LPSR" state voltage in uV. See below table for
> +          bucks which support this. 0 means disabled.
> +
> +#Supported default DVS states:
> +#buck		| run		| idle		| suspend	| lpsr
> +#----------------------------------------------------------------------------
> +#1, 2, 6, and 7	| supported	| supported	| 	supported (*)
> +#----------------------------------------------------------------------------
> +#3, 4, and 5	| 			supported (**)
> +#----------------------------------------------------------------------------
> +#(*)  LPSR and SUSPEND states use same voltage but both states have own enable /
> +#     disable settings. Voltage 0 can be specified for a state to make regulator
> +#     disabled on that state.
> +#(**) All states use same voltage but have own enable / disable settings.
> +#     Voltage 0 can be specified for a state to make regulator disabled on that
> +#     state.

Would be nicer if indented to the same level.

> +
> +      rohm,dvs-runlvl-ctrl:
> +        description: |
> +          buck control is done based on run-level. Regulator is not
> +          individually controllable. See ../mfd/rohm,bd71828-pmic.yaml for
> +          how to specify run-level control mechanism. Only bucks 1, 2, 6
> +          and 7 support this.
> +        type: boolean
> +
> +      rohm,dvs-runlevel-microvolts:
> +        minimum: 0
> +        maximum: 2000000
> +        maxItems: 4

Mixing array and scalar constraints.

maxItems: 4
items:
  minimum: 0
  maximum: 2000000


> +        description:
> +          Array of voltages for run-levels. First value is for run-level 0,
> +          second for run-level 1 etc. Microvolts.
> +
> +    required:
> +      - regulator-name
> +  additionalProperties: false
> +additionalProperties: false
> -- 
> 2.21.0
> 
> 
> -- 
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
> 
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]

Hello Again Mark,

Sorry for long delay - I am really having too many things on my table
right now :/

On Tue, 2019-11-19 at 19:36 +0000, Mark Brown wrote:
> On Tue, Nov 19, 2019 at 06:51:37PM +0000, Vaittinen, Matti wrote:
> > On Tue, 2019-11-19 at 18:13 +0000, Mark Brown wrote:
> > > Ah, OK.  I didn't even notice that patch when I scanned the
> > > series.
> > > I'll look out for this next time around but that sounds like it's
> > > generally going in the right direction, especially if it's
> > > integrated
> > > with the suspend mode regulator bindings that Chunyan did.
> > Probably it is not as I am not familiar with Chunyan's work. I'll
> > try
> > looking what has been done on that front :) And I am pretty sure
> > you
> > might not be happy with that patch - but perhaps you can give me a
> > nudge to better direction...
> 
> The driver interface was added in "regulator: add PM suspend and
> resume
> hooks".

I looked through the set but didn't spot any new interface towards the
regulator driver (which accesses the HW). I saw interface towards
regulator consumer driver which can be used to set the constrains
though.

Specifically, I don't see voltage setting callback for different run-
modes. Nor do I see voltage setting (or differentiation) of more than
one suspend state.

To explain it further - my assumption is that the BD71828 'run-levels'
(RUN0, ... RUN3) could be mapped to regulator modes
REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL, REGULATOR_MODE_IDLE and 
REGULATOR_MODE_STANDBY. But regulators which are controlled by these
run-levels, can't be individually controlled. If state for one is
changed, the state is changed for all of them. The DVS bucks 1,2,6 and
7 support this. Rest of the LDOs and BUCKs (and also those DVS bucks
which are not configured to be controlled by run-levels) have modes RUN
and IDLE, where the processor stays powered.

In addition to these (active) modes/states, there is few states where
processor is not powered. I guess these could be mapped to 'different'
suspend states. At least LPSR, HBNT and SHIP states are such. These are
again global PMIC states - not per regulator states. They can be either
controlled by driving a pin on PMIC - or by I2C register setting. I
don't see how I could differentiate these states when using standard
APIs - nor do I know if these should be changed via regulator
interfaces at all.

All in all - I am also a bit unsure how I should do the mapping of the
PMIC low-power modes to the modes used by Linux - the curse of working
for component vendor is that I have limited visibility to actual end-
products - if they are not in-tree. :( And I don't think we have any
in-tree boards which use these low-power states (at least for now) - So
if you or Rob do not object - I will leave these bindings in this doc -
but I need to consider the value of adding stuff presented in patch 12
in-tree kernel... Guess I'll drop it out unless I get some better
understanding how run-levels and low-power modes are handled in some of
the actual devices. We can always add this support later :)

> > > Yes, I think this needs clarification as I completely failed to
> > > pick
> > > up
> > > on this and did indeed read this as referring to the
> > > modes.  "Voltages
> > > that can be set in RUN mode" or something?  I take it these
> > > voltages
> > > are
> > > fixed and the OS can't change them?
> > Unfortunately they are not. Voltages and enable/disable statuses
> > for
> > each run-level (and individually for each run-level capable buck)
> > can
> > be changed at runtime via I2C. And a customer requested me also to
> > support this - hence the in-kernel API - but I am sure you have
> > some
> > nice words when you check the patch 12. :]
> 
> Ah, that's actually better.  It opens up possiblities for making use
> of
> the feature without encoding voltages in DT.  For example, you can
> cache
> the last however many voltages that were set and jump quickly to them
> or
> do something like put the top of the constraints in to help with
> governors like ondemand.  I'd recommend trying for something like
> that
> rather than encoding in DT, it'll probably be more robust with things
> like cpufreq changing.

I wish I was working with the full product so that I could see and
learn a proper example on how the cpufreq actually uses these
interfaces :) I'd really like to understand this much better. Maybe
this could be a topic for you to present in some Linux conference ;)
Just please ping me when you are doing that and I'll be listening there
for sure ;)

Anyways, my idea was to set the inital voltage values for these states
via DT - but allow the voltages to be changed at run-time too (I guess
this idea is visible in the patch 12).

Br,
	Matti Vaittinen

On Fri, Nov 29, 2019 at 07:48:13AM +0000, Vaittinen, Matti wrote:
> On Tue, 2019-11-19 at 19:36 +0000, Mark Brown wrote:

> > The driver interface was added in "regulator: add PM suspend and
> > resume
> > hooks".

> I looked through the set but didn't spot any new interface towards the
> regulator driver (which accesses the HW). I saw interface towards
> regulator consumer driver which can be used to set the constrains
> though.

The regulator driver has a bunch fo set_suspend_ operations.

> Specifically, I don't see voltage setting callback for different run-
> modes. Nor do I see voltage setting (or differentiation) of more than
> one suspend state.

set_suspend_voltage.

> To explain it further - my assumption is that the BD71828 'run-levels'
> (RUN0, ... RUN3) could be mapped to regulator modes
> REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL, REGULATOR_MODE_IDLE and 
> REGULATOR_MODE_STANDBY. But regulators which are controlled by these

That doesn't make sense at all, the modes affect the quality of
regulation not the voltage that is set.

> run-levels, can't be individually controlled. If state for one is
> changed, the state is changed for all of them. The DVS bucks 1,2,6 and

We don't really have anything that'd only work for group configuration
except for the suspend modes.

> > Ah, that's actually better.  It opens up possiblities for making use
> > of
> > the feature without encoding voltages in DT.  For example, you can
> > cache
> > the last however many voltages that were set and jump quickly to them
> > or
> > do something like put the top of the constraints in to help with
> > governors like ondemand.  I'd recommend trying for something like
> > that
> > rather than encoding in DT, it'll probably be more robust with things
> > like cpufreq changing.

> I wish I was working with the full product so that I could see and
> learn a proper example on how the cpufreq actually uses these
> interfaces :) I'd really like to understand this much better. Maybe
> this could be a topic for you to present in some Linux conference ;)
> Just please ping me when you are doing that and I'll be listening there
> for sure ;)

The cpufreq code is all there in kernel - drivers/cpufreq.  I can't
remember if Android still has a custom governor in their trees but it
doesn't really make much difference in terms of how it interacts with
the regulator drivers.

> Anyways, my idea was to set the inital voltage values for these states
> via DT - but allow the voltages to be changed at run-time too (I guess
> this idea is visible in the patch 12).

It'd be much better if you could avoid putting the voltages in the
binding if they're not strictly required.

Hello Mark!

On Fri, 2019-11-29 at 12:09 +0000, Mark Brown wrote:
> On Fri, Nov 29, 2019 at 07:48:13AM +0000, Vaittinen, Matti wrote:
> > On Tue, 2019-11-19 at 19:36 +0000, Mark Brown wrote:
> > > The driver interface was added in "regulator: add PM suspend and
> > > resume
> > > hooks".
> > I looked through the set but didn't spot any new interface towards
> > the
> > regulator driver (which accesses the HW). I saw interface towards
> > regulator consumer driver which can be used to set the constrains
> > though.
> 
> The regulator driver has a bunch fo set_suspend_ operations.

Hmm. I saw these. But unless I am mistaken linux only knows one
'suspend' state whereas the PMIC has a few separate states I can see as
'suspend' states. As far as I understood the set_suspend_voltage does
not allow setting separate suspend voltages depending on the "type of
suspend" (as there is only one 'suspend' state).

For example, from CPU point of view the BD71828 PMIC states HIBERNATE
and LPSR are probably both just "suspend" - but the PMIC could set
different voltages or ON/OFF states for some regulators depending on
the 'suspend' target (LPSR or HIBERNATE or STANDBY).

Yet, as I said, I haven't seen how these states are used by real
devices - we don't currently have any in-tree SoCs which use BD71828
and utilize these states (as far as I know). Hence I can't really
figure out how to add support for these PMIC features if there is no
'de-facto' mechanism in place :(
> 
> > Specifically, I don't see voltage setting callback for different
> > run-
> > modes. Nor do I see voltage setting (or differentiation) of more
> > than
> > one suspend state.
> 
> set_suspend_voltage.

Yes. But this does only allow setting one suspend voltage for
regulator, not own voltage for HIBERNATE, LPSR, STANDBY etc - or am I
mistaken?

> > To explain it further - my assumption is that the BD71828 'run-
> > levels'
> > (RUN0, ... RUN3) could be mapped to regulator modes
> > REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL, REGULATOR_MODE_IDLE
> > and 
> > REGULATOR_MODE_STANDBY. But regulators which are controlled by
> > these
> 
> That doesn't make sense at all, the modes affect the quality of
> regulation not the voltage that is set.

Thanks. I misunderstood this. I thought these states could be used for
some adaptive voltages. My understanding is that the RUN0,...RUN3 are
designed for that - but I didn't know if regulator framework is
designed for this.

> > run-levels, can't be individually controlled. If state for one is
> > changed, the state is changed for all of them. The DVS bucks 1,2,6
> > and
> 
> We don't really have anything that'd only work for group
> configuration
> except for the suspend modes.

Thanks. As I said, I thought the 'quality of regulation' states could
have been supporting also changing the voltages.

> 
> > > Ah, that's actually better.  It opens up possiblities for making
> > > use
> > > of
> > > the feature without encoding voltages in DT.  For example, you
> > > can
> > > cache
> > > the last however many voltages that were set and jump quickly to
> > > them
> > > or
> > > do something like put the top of the constraints in to help with
> > > governors like ondemand.  I'd recommend trying for something like
> > > that
> > > rather than encoding in DT, it'll probably be more robust with
> > > things
> > > like cpufreq changing.
> > I wish I was working with the full product so that I could see and
> > learn a proper example on how the cpufreq actually uses these
> > interfaces :) I'd really like to understand this much better. Maybe
> > this could be a topic for you to present in some Linux conference
> > ;)
> > Just please ping me when you are doing that and I'll be listening
> > there
> > for sure ;)
> 
> The cpufreq code is all there in kernel - drivers/cpufreq.  I can't
> remember if Android still has a custom governor in their trees but it
> doesn't really make much difference in terms of how it interacts with
> the regulator drivers.

Right. I guess your answers mean that there is no "regulator group
control" for "adaptive voltage changes" supported by regulator
framework / cpufreq - as of now. Furthermore, I have understood that it
is different story depending on PMIC and CPU/SoC capabilities. (Like
PMIC inbuilt states and state change mechanisms, SoC voltage scaling
support etc.)

>  Anyways, my idea was to set the inital voltage values for these
> states
> > via DT - but allow the voltages to be changed at run-time too (I
> > guess
> > this idea is visible in the patch 12).
> 
> It'd be much better if you could avoid putting the voltages in the
> binding if they're not strictly required.

Hmm.. I guess I can omit that from in-tree driver for now. I can try
maintaining own set of patches for existing customers for now. I think
adding these to bindings later is not impossible :) Removing them would
probably be harder.

Thanks again Mark. I truly appreciate your help and guidance :)

Br,
	Matti

On Mon, Dec 02, 2019 at 07:57:13AM +0000, Vaittinen, Matti wrote:
> On Fri, 2019-11-29 at 12:09 +0000, Mark Brown wrote:

> > The regulator driver has a bunch fo set_suspend_ operations.

> Hmm. I saw these. But unless I am mistaken linux only knows one
> 'suspend' state whereas the PMIC has a few separate states I can see as
> 'suspend' states. As far as I understood the set_suspend_voltage does
> not allow setting separate suspend voltages depending on the "type of
> suspend" (as there is only one 'suspend' state).

No, look at the bindings - we support a bunch of different
suspend states matching the different suspend states that the
kernel as a whole supports.  We don't assume that the device will
know this but you can always use the current suspend we're going
for to decide where to update.

> > > (RUN0, ... RUN3) could be mapped to regulator modes
> > > REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL, REGULATOR_MODE_IDLE
> > > and 
> > > REGULATOR_MODE_STANDBY. But regulators which are controlled by
> > > these

> > That doesn't make sense at all, the modes affect the quality of
> > regulation not the voltage that is set.

> Thanks. I misunderstood this. I thought these states could be used for
> some adaptive voltages. My understanding is that the RUN0,...RUN3 are
> designed for that - but I didn't know if regulator framework is
> designed for this.

The framework doesn't care how a device is controlled, that's up
to the device.  Like I said I recommend figuring out what
voltages are useful to have quick access to at runtime, for
example it's likely that it's good to have quick access to the
highest voltage that's been set (and/or the top of the
constraints).

> > The cpufreq code is all there in kernel - drivers/cpufreq.  I can't
> > remember if Android still has a custom governor in their trees but it
> > doesn't really make much difference in terms of how it interacts with
> > the regulator drivers.

> Right. I guess your answers mean that there is no "regulator group
> control" for "adaptive voltage changes" supported by regulator

I can't parse the above, sorry.  What is "regulator group
control"?

On Mon, 2019-12-02 at 13:11 +0000, Mark Brown wrote:
> On Mon, Dec 02, 2019 at 07:57:13AM +0000, Vaittinen, Matti wrote:
> > On Fri, 2019-11-29 at 12:09 +0000, Mark Brown wrote:
> > > The regulator driver has a bunch fo set_suspend_ operations.
> > Hmm. I saw these. But unless I am mistaken linux only knows one
> > 'suspend' state whereas the PMIC has a few separate states I can
> > see as
> > 'suspend' states. As far as I understood the set_suspend_voltage
> > does
> > not allow setting separate suspend voltages depending on the "type
> > of
> > suspend" (as there is only one 'suspend' state).
> 
> No, look at the bindings - we support a bunch of different
> suspend states matching the different suspend states that the
> kernel as a whole supports.  We don't assume that the device will
> know this but you can always use the current suspend we're going
> for to decide where to update.

Hm. So if I understand this correctly, you mean user should set the
suspend 'target' - and then call the set_suspend_voltage for this
state. To set voltages for all states one should do loop

get_current_mode()

for_all_modes() {
	set_mode()
	set_voltage()
}

restore_original_mode()

am I on a right track? I'll try to see if I can find some examples of
this - thanks.

> 
> > > > (RUN0, ... RUN3) could be mapped to regulator modes
> > > > REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL, REGULATOR_MODE_IDLE
> > > > and 
> > > > REGULATOR_MODE_STANDBY. But regulators which are controlled by
> > > > these
> > > That doesn't make sense at all, the modes affect the quality of
> > > regulation not the voltage that is set.
> > Thanks. I misunderstood this. I thought these states could be used
> > for
> > some adaptive voltages. My understanding is that the RUN0,...RUN3
> > are
> > designed for that - but I didn't know if regulator framework is
> > designed for this.
> 
> The framework doesn't care how a device is controlled, that's up
> to the device.  Like I said I recommend figuring out what
> voltages are useful to have quick access to at runtime, for
> example it's likely that it's good to have quick access to the
> highest voltage that's been set (and/or the top of the
> constraints).

Problem is that the run-level controlled regulator can't be
individually controlled (unless it is only regulator in the group). I
misunderstood these REGULATOR_MODE_FAST, ...,REGULATOR_MODE_STANDBY to
be global 'states' rather than states of individual regulators. And I
thought these were also designed for voltage scaling. But as I said, I
misunderstood them - so thanks for correcting me on this.

> 
> > > The cpufreq code is all there in kernel - drivers/cpufreq.  I
> > > can't
> > > remember if Android still has a custom governor in their trees
> > > but it
> > > doesn't really make much difference in terms of how it interacts
> > > with
> > > the regulator drivers.
> > Right. I guess your answers mean that there is no "regulator group
> > control" for "adaptive voltage changes" supported by regulator
> 
> I can't parse the above, sorry.  What is "regulator group
> control"?

I mean bundling the regulators in a group - and changing state for all
of the bundled regulators in one go. The thing I mentioned earlier -
and I guess you did already confirm it's not doable. I think you said
that only 'mass operation' or 'group operation' is the suspend.

But just to confirm, I meant for example assigning bucks 1,2,6 and 7
into a group which 'state' is changed via GPIO line. Say 'states' are
RUN0, RUN1. For each of these bucks we can define a voltage and
enable/disable status which is to be used on RUN0, and another
voltage/state tuple for RUN1.

When certain 'trigger' is detected (I assume CPU load here and adaptive
voltage scaling - but this is just my assumption of the use-case for
now) the PMIC state can be quickly changed via this GPIO toggle.

In realty, we have two GPIOs and 4 states - but that does not change
the princible. I don't think there is any 'de-facto' mechanism to
control such groups.


Br,
	Matti Vaittinen

On Mon, Dec 02, 2019 at 02:02:41PM +0000, Vaittinen, Matti wrote:
> On Mon, 2019-12-02 at 13:11 +0000, Mark Brown wrote:

> > No, look at the bindings - we support a bunch of different
> > suspend states matching the different suspend states that the
> > kernel as a whole supports.  We don't assume that the device will
> > know this but you can always use the current suspend we're going
> > for to decide where to update.

> Hm. So if I understand this correctly, you mean user should set the
> suspend 'target' - and then call the set_suspend_voltage for this
> state. To set voltages for all states one should do loop

The general idea is that we set the suspend state during the
process of suspending rather than in advance - that way when the
hardware doesn't understand different types of suspsend things
work fine.

> get_current_mode()

> for_all_modes() {
> 	set_mode()
> 	set_voltage()
> }

> restore_original_mode()

> am I on a right track? I'll try to see if I can find some examples of
> this - thanks.

I don't understand the save and restore of mode?  If setting the
suspend configuration affects the runtime state then the hardware
doesn't support suspend configuration.

> > The framework doesn't care how a device is controlled, that's up
> > to the device.  Like I said I recommend figuring out what
> > voltages are useful to have quick access to at runtime, for
> > example it's likely that it's good to have quick access to the
> > highest voltage that's been set (and/or the top of the
> > constraints).

> Problem is that the run-level controlled regulator can't be
> individually controlled (unless it is only regulator in the group). I

Regulators that have to be controlled en masse aren't really
supported by the API, it only understands regulators that are
individually controllable.

> But just to confirm, I meant for example assigning bucks 1,2,6 and 7
> into a group which 'state' is changed via GPIO line. Say 'states' are
> RUN0, RUN1. For each of these bucks we can define a voltage and
> enable/disable status which is to be used on RUN0, and another
> voltage/state tuple for RUN1.

So you could also just create a group consisting of a single
regulator?  That would be fine for the API.

On Wed, 2019-12-04 at 12:47 +0000, Mark Brown wrote:
> On Mon, Dec 02, 2019 at 02:02:41PM +0000, Vaittinen, Matti wrote:
> > On Mon, 2019-12-02 at 13:11 +0000, Mark Brown wrote:
> > > No, look at the bindings - we support a bunch of different
> > > suspend states matching the different suspend states that the
> > > kernel as a whole supports.  We don't assume that the device will
> > > know this but you can always use the current suspend we're going
> > > for to decide where to update.
> > Hm. So if I understand this correctly, you mean user should set the
> > suspend 'target' - and then call the set_suspend_voltage for this
> > state. To set voltages for all states one should do loop
> 
> The general idea is that we set the suspend state during the
> process of suspending rather than in advance - that way when the
> hardware doesn't understand different types of suspsend things
> work fine.

Ok. So voltage for the specific suspend state is set just before going
to suspend - when the target suspend state is already selected. Makes
sense. Thanks.

> > get_current_mode()
> > for_all_modes() {
> > 	set_mode()
> > 	set_voltage()
> > }
> > restore_original_mode()
> > am I on a right track? I'll try to see if I can find some examples
> > of
> > this - thanks.
> 
> I don't understand the save and restore of mode?

I was thinking that there is some 'cahced suspend target state' in
framework. This get mode is pseudo-code for getting the suspend mode
from framework - "restore mode" is returning back the "original mode"
after voltages for all modes are set. But please ignore this - I think
I captured your message already :)

>   If setting the
> suspend configuration affects the runtime state then the hardware
> doesn't support suspend configuration.

No, it does not. I probably explained this badly.

> 
> > > The framework doesn't care how a device is controlled, that's up
> > > to the device.  Like I said I recommend figuring out what
> > > voltages are useful to have quick access to at runtime, for
> > > example it's likely that it's good to have quick access to the
> > > highest voltage that's been set (and/or the top of the
> > > constraints).
> > Problem is that the run-level controlled regulator can't be
> > individually controlled (unless it is only regulator in the group).
> > I
> 
> Regulators that have to be controlled en masse aren't really
> supported by the API, it only understands regulators that are
> individually controllable.

Thanks. This was the piece of information I wanted and assumed. Thus
the patch 12 did introduce new in-kernel APIs - but as I said, we don't
have any in-tree socs using BD71828 RUN0, ..., RUN3 states for now so
I'll drop the patch 12 and leave only the basic support for
individually controllable regulators.

> > But just to confirm, I meant for example assigning bucks 1,2,6 and
> > 7
> > into a group which 'state' is changed via GPIO line. Say 'states'
> > are
> > RUN0, RUN1. For each of these bucks we can define a voltage and
> > enable/disable status which is to be used on RUN0, and another
> > voltage/state tuple for RUN1.
> 
> So you could also just create a group consisting of a single
> regulator?  That would be fine for the API.

I think I once again explained myself badly. There can be only one
group with 4 RUN states selected by combination of 2 GPIO lines. bucks
1,2,6 and 7 can each either be assigned into this one group or
controlled individually via I2C. But I doubt assigning only one of the
bucks in this group is the typical use-case. What we would need would
really be the 'en-masse' control - either via GPIO or I2C - but I don't
want to suggest any framework expansion as I don't have proper in-tree
use-case for this as of now :) So I'll forget this for now (and thanks
for all the help!) and drop the patch 12 from series. I'll see if I can
send updated series tomorrow - or latest early next week :)

Br,
	Matti Vaittinen

On Wed, Dec 04, 2019 at 01:13:08PM +0000, Vaittinen, Matti wrote:

> I think I once again explained myself badly. There can be only one
> group with 4 RUN states selected by combination of 2 GPIO lines. bucks
> 1,2,6 and 7 can each either be assigned into this one group or
> controlled individually via I2C. But I doubt assigning only one of the
> bucks in this group is the typical use-case. What we would need would

I don't think this is as unusual as you're thinking - the
regulators people want to control quickly are usually the main
CPU supply regulators and these often vary independently of
anything else.

On Wed, 2019-12-04 at 14:14 +0000, Mark Brown wrote:
> On Wed, Dec 04, 2019 at 01:13:08PM +0000, Vaittinen, Matti wrote:
> 
> > I think I once again explained myself badly. There can be only one
> > group with 4 RUN states selected by combination of 2 GPIO lines.
> > bucks
> > 1,2,6 and 7 can each either be assigned into this one group or
> > controlled individually via I2C. But I doubt assigning only one of
> > the
> > bucks in this group is the typical use-case. What we would need
> > would
> 
> I don't think this is as unusual as you're thinking - the
> regulators people want to control quickly are usually the main
> CPU supply regulators and these often vary independently of
> anything else.

Hmm. I see your point. You might be correct. Allowing only one buck to
be assigned in 'run-level group' (to be controlled by GPIOs) would be
totally possible with current regulator API - and it might be useful
for scaling the CPU voltage. I appreciate your help and experience here
:) Implementing it would be also pretty simple, caching and controlling
the run-level voltages is already there in patch 12, I just should
restrict the group size to one buck. I will see how it works and also
ask if my colleagues know whether this is valuable with our current
customer's SOCs. Thanks a bunch!

Br,
	Matti Vaittinen

Hello Mark,

On Fri, 2019-11-29 at 12:09 +0000, Mark Brown wrote:
> On Fri, Nov 29, 2019 at 07:48:13AM +0000, Vaittinen, Matti wrote:
> > On Tue, 2019-11-19 at 19:36 +0000, Mark Brown wrote:
> The cpufreq code is all there in kernel - drivers/cpufreq.  I can't
> remember if Android still has a custom governor in their trees but it
> doesn't really make much difference in terms of how it interacts with
> the regulator drivers.
> 
> > Anyways, my idea was to set the inital voltage values for these
> > states
> > via DT - but allow the voltages to be changed at run-time too (I
> > guess
> > this idea is visible in the patch 12).
> 
> It'd be much better if you could avoid putting the voltages in the
> binding if they're not strictly required.

You suggested in the other mail that it might be worth making a run-
level 'group' consisting only one buck for fast voltage changes via
GPIOs. So I am back to adding the run-level support to the BD71828
driver. Which lead me back to this.

The PMIC supports wide range of voltages for these DVS bucks - but only
4 of these voltages can be selected to be switched fast via GPIO line
states. Eg, in HW level we can set RUN0 voltage (selected when both
GPIO lines are LOW) to one register. RUN1 voltage (selected when one
GPIO is high, other low) to second register. Same for RUN2 and RUN3
voltages.

I could make this so that initially there is the HW default voltages
are read up by driver and cached to be used for each level. When new
voltage is requested by the consumer, correct RUN level is selected or
if matching voltage is not set to any RUN level, then it is written to
one of the run level registers (and cache).

Problem is that if no default voltages are given from DT, the the first
voltage changes are likely to be slow (require register access - I
guess the HW defaults are not working for many use-cases) - which may
be undesirable.

So I still think the DT bindings for setting these initial values might
be the most convenient way if we are not adding custom API for this.
Hence I plan to keep this binding. Please let me know if you have
better ideas or if this is absolutely a no go.

Br,
	Matti Vaittinen

On Tue, Dec 10, 2019 at 11:14:48AM +0000, Vaittinen, Matti wrote:

> Problem is that if no default voltages are given from DT, the the first
> voltage changes are likely to be slow (require register access - I
> guess the HW defaults are not working for many use-cases) - which may
> be undesirable.

I don't think that's likely to be a practical problem, and it's not
likely it'd be worse than always doing writes.  A lot of things are
slower the first time you do them and you're still going to have to
do the writes no matter what.

Hello Mark,

On Tue, 2019-12-10 at 12:11 +0000, Mark Brown wrote:
> On Tue, Dec 10, 2019 at 11:14:48AM +0000, Vaittinen, Matti wrote:
> 
> > Problem is that if no default voltages are given from DT, the the
> > first
> > voltage changes are likely to be slow (require register access - I
> > guess the HW defaults are not working for many use-cases) - which
> > may
> > be undesirable.
> 
> I don't think that's likely to be a practical problem, and it's not
> likely it'd be worse than always doing writes.  A lot of things are
> slower the first time you do them and you're still going to have to
> do the writes no matter what.

The thing is that if we do initial setting of voltages (based on
binding data) we can set the voltages to registers before we switch to
that run-level. If we don't do initial setting then we will only do
setting when voltage change is actually requested - which may be too
late. (I actually heard somewhere that there is 40 uS time limit - but
I don't see how this is counted. Starting from what? - and I don't see
how this is guaranteed even with GPIO if interrupts are to be served).

OTOH, I also heard that the SoC probably require at least two of the
bucks to be controlled as a group (bucks connected to SRAM and CORE) -
probably all four.

So, I am again wondering if I should just upstream the basic control
with I2C for SoCs which do not require fast DVS voltage changes and
perhaps maintain/provide own set of patches with additional interface
for run-level control for those customers who require it... Sorry for
being such a difficult guy. Decision making seems to not be my strong
point :/

Br,
	Matti Vaittinen

On Tue, Dec 10, 2019 at 12:41:47PM +0000, Vaittinen, Matti wrote:

> The thing is that if we do initial setting of voltages (based on
> binding data) we can set the voltages to registers before we switch to
> that run-level. If we don't do initial setting then we will only do
> setting when voltage change is actually requested - which may be too
> late. (I actually heard somewhere that there is 40 uS time limit - but
> I don't see how this is counted. Starting from what? - and I don't see
> how this is guaranteed even with GPIO if interrupts are to be served).

I suspect that if that limit is a real thing it's from some runtime
performance metrics where people are doing benchmarking to verify that
everything is working fine rather than an absolute thing that is a basic
requirement for operation.

> So, I am again wondering if I should just upstream the basic control
> with I2C for SoCs which do not require fast DVS voltage changes and
> perhaps maintain/provide own set of patches with additional interface
> for run-level control for those customers who require it... Sorry for
> being such a difficult guy. Decision making seems to not be my strong
> point :/

Yes, definitely submit the basic stuff separately - the GPIO changes can
be reviewed as a separate, incremental patch.

On Tue, 2019-12-10 at 12:45 +0000, Mark Brown wrote:
> On Tue, Dec 10, 2019 at 12:41:47PM +0000, Vaittinen, Matti wrote:
> 
> > The thing is that if we do initial setting of voltages (based on
> > binding data) we can set the voltages to registers before we switch
> > to
> > that run-level. If we don't do initial setting then we will only do
> > setting when voltage change is actually requested - which may be
> > too
> > late. (I actually heard somewhere that there is 40 uS time limit -
> > but
> > I don't see how this is counted. Starting from what? - and I don't
> > see
> > how this is guaranteed even with GPIO if interrupts are to be
> > served).
> 
> I suspect that if that limit is a real thing it's from some runtime
> performance metrics where people are doing benchmarking to verify
> that
> everything is working fine rather than an absolute thing that is a
> basic
> requirement for operation.
> 
> > So, I am again wondering if I should just upstream the basic
> > control
> > with I2C for SoCs which do not require fast DVS voltage changes and
> > perhaps maintain/provide own set of patches with additional
> > interface
> > for run-level control for those customers who require it... Sorry
> > for
> > being such a difficult guy. Decision making seems to not be my
> > strong
> > point :/
> 
> Yes, definitely submit the basic stuff separately - the GPIO changes
> can
> be reviewed as a separate, incremental patch.

Right. That was rationale behind splitting the regulator support in two
patches. I was just unsure if I should add all the DT bindings already
here. Well, I guess I will drop the run-level ones for now. Adding new
bindings later might not be as hard as removing them. Thanks for
support! Discussing this with someone is definitely helpful :)

Br,
	Matti