[PATCH v3 0/2] regulator: add QCOM RPMh regulator driver

[PATCH v3 0/2] regulator: add QCOM RPMh regulator driver

[David Collins]

This patch series adds a driver and device tree binding documentation for
PMIC regulator control via Resource Power Manager-hardened (RPMh) on some
Qualcomm Technologies, Inc. SoCs such as SDM845.  RPMh is a hardware block
which contains several accelerators which are used to manage various
hardware resources that are shared between the processors of the SoC.  The
final hardware state of a regulator is determined within RPMh by performing
max aggregation of the requests made by all of the processors.

The RPMh regulator driver depends upon the RPMh driver [1] and command DB
driver [2] which are both still undergoing review.  It also depends upon
two recent of_regulator changes: [3] and [4].

Changes since v2 [5]:
 - Replaced '_' with '-' in device tree supply property names
 - Renamed qcom_rpmh-regulator.c to be qcom-rpmh-regulator.c
 - Updated various DT property names to use "microvolt" and "microamp"
 - Moved allowed modes constraint specification out of the driver [4]
 - Replaced rpmh_client with device pointer to match new RPMh API [1]
 - Corrected drms mode threshold checking
 - Initialized voltage_selector to -EINVAL when not specified in DT
 - Added constants for PMIC regulator hardware modes
 - Corrected type sign of mode mapping tables
 - Made variable names for mode arrays plural
 - Simplified Kconfig depends on
 - Removed unnecessary constants and struct fields
 - Added some descriptive comments

Changes since v1 [6]:
 - Addressed review feedback from Doug, Mark, and Stephen
 - Replaced set_voltage()/get_voltage() callbacks with set_voltage_sel()/
   get_voltage_sel()
 - Added set_bypass()/get_bypass() callbacks for BOB pass-through mode
   control
 - Removed top-level PMIC data structures
 - Removed initialization variables from structs and passed them as
   function parameters
 - Removed various comments and error messages
 - Simplified mode handling
 - Refactored per-PMIC rpmh-regulator data specification
 - Simplified probe function
 - Moved header into DT patch
 - Removed redundant property listings from DT binding documentation

[1]: https://lkml.org/lkml/2018/5/9/729
[2]: https://lkml.org/lkml/2018/4/10/714
[3]: https://patchwork.kernel.org/patch/10348629
[4]: https://lkml.org/lkml/2018/5/11/696
[5]: https://lkml.org/lkml/2018/4/13/687
[6]: https://lkml.org/lkml/2018/3/16/1431

David Collins (2):
  regulator: dt-bindings: add QCOM RPMh regulator bindings
  regulator: add QCOM RPMh regulator driver

 .../bindings/regulator/qcom,rpmh-regulator.txt     | 208 +++++
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/qcom-rpmh-regulator.c            | 925 +++++++++++++++++++++
 .../dt-bindings/regulator/qcom,rpmh-regulator.h    |  36 +
 5 files changed, 1179 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
 create mode 100644 drivers/regulator/qcom-rpmh-regulator.c
 create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[David Collins]

Introduce bindings for RPMh regulator devices found on some
Qualcomm Technlogies, Inc. SoCs.  These devices allow a given
processor within the SoC to make PMIC regulator requests which
are aggregated within the RPMh hardware block along with requests
from other processors in the SoC to determine the final PMIC
regulator hardware state.

Signed-off-by: David Collins <collinsd@codeaurora.org>
---
 .../bindings/regulator/qcom,rpmh-regulator.txt     | 208 +++++++++++++++++++++
 .../dt-bindings/regulator/qcom,rpmh-regulator.h    |  36 ++++
 2 files changed, 244 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
 create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h

diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
new file mode 100644
index 0000000..ad2185e
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
@@ -0,0 +1,208 @@
+Qualcomm Technologies, Inc. RPMh Regulators
+
+rpmh-regulator devices support PMIC regulator management via the Voltage
+Regulator Manager (VRM) and Oscillator Buffer (XOB) RPMh accelerators.  The APPS
+processor communicates with these hardware blocks via a Resource State
+Coordinator (RSC) using command packets.  The VRM allows changing four
+parameters for a given regulator: enable state, output voltage, operating mode,
+and minimum headroom voltage.  The XOB allows changing only a single parameter
+for a given regulator: its enable state.  Despite its name, the XOB is capable
+of controlling the enable state of any PMIC peripheral.  It is used for clock
+buffers, low-voltage switches, and LDO/SMPS regulators which have a fixed
+voltage and mode.
+
+=======================
+Required Node Structure
+=======================
+
+RPMh regulators must be described in two levels of device nodes.  The first
+level describes the PMIC containing the regulators and must reside within an
+RPMh device node.  The second level describes each regulator within the PMIC
+which is to be used on the board.  Each of these regulators maps to a single
+RPMh resource.
+
+The names used for regulator nodes must match those supported by a given PMIC.
+Supported regulator node names:
+	PM8998:		smps1 - smps13, ldo1 - ldo28, lvs1 - lvs2
+	PMI8998:	bob
+	PM8005:		smps1 - smps4
+
+========================
+First Level Nodes - PMIC
+========================
+
+- compatible
+	Usage:      required
+	Value type: <string>
+	Definition: Must be one of: "qcom,pm8998-rpmh-regulators",
+		    "qcom,pmi8998-rpmh-regulators" or
+		    "qcom,pm8005-rpmh-regulators".
+
+- qcom,pmic-id
+	Usage:      required
+	Value type: <string>
+	Definition: RPMh resource name suffix used for the regulators found on
+		    this PMIC.  Typical values: "a", "b", "c", "d", "e", "f".
+
+- vdd-s1-supply
+- vdd-s2-supply
+- vdd-s3-supply
+- vdd-s4-supply
+	Usage:      optional (PM8998 and PM8005 only)
+	Value type: <phandle>
+	Definition: phandle of the parent supply regulator of one or more of the
+		    regulators for this PMIC.
+
+- vdd-s5-supply
+- vdd-s6-supply
+- vdd-s7-supply
+- vdd-s8-supply
+- vdd-s9-supply
+- vdd-s10-supply
+- vdd-s11-supply
+- vdd-s12-supply
+- vdd-s13-supply
+- vdd-l1-l27-supply
+- vdd-l2-l8-l17-supply
+- vdd-l3-l11-supply
+- vdd-l4-l5-supply
+- vdd-l6-supply
+- vdd-l7-l12-l14-l15-supply
+- vdd-l9-supply
+- vdd-l10-l23-l25-supply
+- vdd-l13-l19-l21-supply
+- vdd-l16-l28-supply
+- vdd-l18-l22-supply
+- vdd-l20-l24-supply
+- vdd-l26-supply
+- vin-lvs-1-2-supply
+	Usage:      optional (PM8998 only)
+	Value type: <phandle>
+	Definition: phandle of the parent supply regulator of one or more of the
+		    regulators for this PMIC.
+
+- vdd-bob-supply
+	Usage:      optional (PMI8998 only)
+	Value type: <phandle>
+	Definition: BOB regulator parent supply phandle
+
+===============================
+Second Level Nodes - Regulators
+===============================
+
+- qcom,regulator-initial-microvolt
+	Usage:      optional; VRM regulators only
+	Value type: <u32>
+	Definition: Specifies the initial voltage in microvolts to request for a
+		    VRM regulator.
+
+- regulator-initial-mode
+	Usage:      optional; VRM regulators only
+	Value type: <u32>
+	Definition: Specifies the initial mode to request for a VRM regulator.
+		    Supported values are RPMH_REGULATOR_MODE_* which are defined
+		    in [1] (i.e. 0 to 3).  This property may be specified even
+		    if the regulator-allow-set-load property is not specified.
+
+- qcom,allowed-drms-modes
+	Usage:      required if regulator-allow-set-load is specified;
+		    VRM regulators only
+	Value type: <prop-encoded-array>
+	Definition: A list of integers specifying the PMIC regulator modes which
+		    can be configured at runtime based upon consumer load needs.
+		    Supported values are RPMH_REGULATOR_MODE_* which are defined
+		    in [1] (i.e. 0 to 3).
+
+- qcom,drms-mode-max-microamps
+	Usage:      required if regulator-allow-set-load is specified;
+		    VRM regulators only
+	Value type: <prop-encoded-array>
+	Definition: A list of integers specifying the maximum allowed load
+		    current in microamps for each of the modes listed in
+		    qcom,allowed-drms-modes (matched 1-to-1 in order).  Elements
+		    must be specified in order from lowest to highest value.
+
+- qcom,headroom-microvolt
+	Usage:      optional; VRM regulators only
+	Value type: <u32>
+	Definition: Specifies the headroom voltage in microvolts to request for
+		    a VRM regulator.  RPMh hardware automatically ensures that
+		    the parent of this regulator outputs a voltage high enough
+		    to satisfy the requested headroom.  Supported values are
+		    0 to 511000.
+
+- qcom,always-wait-for-ack
+	Usage:      optional
+	Value type: <empty>
+	Definition: Boolean flag which indicates that the application processor
+		    must wait for an ACK or a NACK from RPMh for every request
+		    sent for this regulator including those which are for a
+		    strictly lower power state.
+
+Other properties defined in Documentation/devicetree/bindings/regulator.txt
+may also be used.
+
+[1] include/dt-bindings/regulator/qcom,rpmh-regulator.h
+
+========
+Examples
+========
+
+#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+
+&apps_rsc {
+	pm8998-rpmh-regulators {
+		compatible = "qcom,pm8998-rpmh-regulators";
+		qcom,pmic-id = "a";
+
+		vdd-l7-l12-l14-l15-supply = <&pm8998_s5>;
+
+		smps2 {
+			regulator-min-microvolt = <1100000>;
+			regulator-max-microvolt = <1100000>;
+			qcom,regulator-initial-microvolt = <1100000>;
+		};
+
+		pm8998_s5: smps5 {
+			regulator-min-microvolt = <1904000>;
+			regulator-max-microvolt = <2040000>;
+			qcom,regulator-initial-microvolt = <1904000>;
+		};
+
+		ldo7 {
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			qcom,regulator-initial-microvolt = <1800000>;
+			qcom,headroom-microvolt = <56000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_LPM>;
+			regulator-allowed-modes =
+				<RPMH_REGULATOR_MODE_LPM
+				 RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+			qcom,allowed-drms-modes =
+				<RPMH_REGULATOR_MODE_LPM
+				 RPMH_REGULATOR_MODE_HPM>;
+			qcom,drms-mode-max-microamps = <10000 1000000>;
+		};
+
+		lvs1 {
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+		};
+	};
+
+	pmi8998-rpmh-regulators {
+		compatible = "qcom,pmi8998-rpmh-regulators";
+		qcom,pmic-id = "b";
+
+		bob {
+			regulator-min-microvolt = <3312000>;
+			regulator-max-microvolt = <3600000>;
+			qcom,regulator-initial-microvolt = <3312000>;
+			regulator-allowed-modes =
+				<RPMH_REGULATOR_MODE_AUTO
+				 RPMH_REGULATOR_MODE_HPM>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_AUTO>;
+		};
+	};
+};
diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
new file mode 100644
index 0000000..4378c4b
--- /dev/null
+++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#ifndef __QCOM_RPMH_REGULATOR_H
+#define __QCOM_RPMH_REGULATOR_H
+
+/*
+ * These mode constants may be used for regulator-initial-mode and
+ * qcom,allowed-drms-modes properties of an RPMh regulator device tree node.
+ * Each type of regulator supports a subset of the possible modes.
+ *
+ * %RPMH_REGULATOR_MODE_RET:	Retention mode in which only an extremely small
+ *				load current is allowed.  This mode is supported
+ *				by LDO and SMPS type regulators.
+ * %RPMH_REGULATOR_MODE_LPM:	Low power mode in which a small load current is
+ *				allowed.  This mode corresponds to PFM for SMPS
+ *				and BOB type regulators.  This mode is supported
+ *				by LDO, HFSMPS, BOB, and PMIC4 FTSMPS type
+ *				regulators.
+ * %RPMH_REGULATOR_MODE_AUTO:	Auto mode in which the regulator hardware
+ *				automatically switches between LPM and HPM based
+ *				upon the real-time load current.  This mode is
+ *				supported by HFSMPS, BOB, and PMIC4 FTSMPS type
+ *				regulators.
+ * %RPMH_REGULATOR_MODE_HPM:	High power mode in which the full rated current
+ *				of the regulator is allowed.  This mode
+ *				corresponds to PWM for SMPS and BOB type
+ *				regulators.  This mode is supported by all types
+ *				of regulators.
+ */
+#define RPMH_REGULATOR_MODE_RET		0
+#define RPMH_REGULATOR_MODE_LPM		1
+#define RPMH_REGULATOR_MODE_AUTO	2
+#define RPMH_REGULATOR_MODE_HPM		3
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
> +- qcom,regulator-initial-microvolt
> +       Usage:      optional; VRM regulators only
> +       Value type: <u32>
> +       Definition: Specifies the initial voltage in microvolts to request for a
> +                   VRM regulator.

Now that Mark has landed the patch adding support for the
-ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
we still need the qcom,regulator-initial-microvolt property?  If this
is really still needed, can it be moved to the regulator core?


> +- regulator-initial-mode
> +       Usage:      optional; VRM regulators only
> +       Value type: <u32>
> +       Definition: Specifies the initial mode to request for a VRM regulator.
> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
> +                   in [1] (i.e. 0 to 3).  This property may be specified even
> +                   if the regulator-allow-set-load property is not specified.

Every time I read the above I wonder why you're documenting a standard
regulator regulator property in your bindings.  ...then I realize it's
because you're doing it because you want to explicitly document what
the valid modes are.  I wonder if it makes sense to just put a
reference somewhere else in this document to go look at the header
file where these are all nicely documented.

Speaking of documenting things like that, it might be worth finding
somewhere in this doc to mention that the "bob" regulator on PMI8998
can support "regulator-allow-bypass".  That tidbit got lost when we
moved to the standard regulator bindings for bypass.


> +- qcom,allowed-drms-modes
> +       Usage:      required if regulator-allow-set-load is specified;
> +                   VRM regulators only
> +       Value type: <prop-encoded-array>
> +       Definition: A list of integers specifying the PMIC regulator modes which
> +                   can be configured at runtime based upon consumer load needs.
> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
> +                   in [1] (i.e. 0 to 3).

Why is this still here?  You moved it to the core regulator framework,
right?  It's still in your examples too.  Shouldn't this be removed?
It looks like the driver still needs this and it needs to be an exact
duplicate of the common binding.  That doesn't seem right...


> +- qcom,drms-mode-max-microamps
> +       Usage:      required if regulator-allow-set-load is specified;
> +                   VRM regulators only
> +       Value type: <prop-encoded-array>
> +       Definition: A list of integers specifying the maximum allowed load
> +                   current in microamps for each of the modes listed in
> +                   qcom,allowed-drms-modes (matched 1-to-1 in order).  Elements
> +                   must be specified in order from lowest to highest value.

Any reason this can't go into the regulator core?  You'd basically
just take the existing concept of rpmh_regulator_vrm_set_load() and
put it in the core.


> +- qcom,headroom-microvolt
> +       Usage:      optional; VRM regulators only
> +       Value type: <u32>
> +       Definition: Specifies the headroom voltage in microvolts to request for
> +                   a VRM regulator.  RPMh hardware automatically ensures that
> +                   the parent of this regulator outputs a voltage high enough
> +                   to satisfy the requested headroom.  Supported values are
> +                   0 to 511000.

I'm curious: is this a voted-for value, or a global value?

Said another way: the whole point of RPMh is that there may be more
than one processor that needs the same rails, right?  So the AP might
request 1.1 V for a rail and the modem might request 1.3 V.  RPMh
would decide to pick the higher of those two (1.3 V), but if the modem
said it no longer needs the rail it will drop down to 1.1 V.

...and as an example of why the headroom needs to be in hardware, if
the source voltage was normally 1.4 V and the headroom was 200 mV then
the hardware would need to know to bump up the source voltage to 1.5V
during the period of of time that the modem wants the rail at 1.3V.

So my question is: do the AP and modem in the above situation
separately vote for headroom?  How is it aggregated?  ...or is it a
global value and this sets the headroom for all clients of RPMh?  It
would be interesting to document this as it might help with figuring
out how this value should be set.


> diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
> new file mode 100644
> index 0000000..4378c4b
> --- /dev/null
> +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
> +/*
> + * These mode constants may be used for regulator-initial-mode and
> + * qcom,allowed-drms-modes properties of an RPMh regulator device tree node.

Technically also for your new "regulator-allowed-modes".  Maybe just
say that they're used anywhere a regulator mode is needed in this
driver and give regulator-initial-mode as an example?


-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[David Collins]

On 05/17/2018 02:22 PM, Doug Anderson wrote:
> On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
>> +- qcom,regulator-initial-microvolt
>> +       Usage:      optional; VRM regulators only
>> +       Value type: <u32>
>> +       Definition: Specifies the initial voltage in microvolts to request for a
>> +                   VRM regulator.
> 
> Now that Mark has landed the patch adding support for the
> -ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
> we still need the qcom,regulator-initial-microvolt property?

Yes, this is still needed.  The -ENOTRECOVERABLE patch ensures that
qcom-rpmh-regulator devices can be registered even if
qcom,regulator-initial-microvolt is not specified.  However, that will
result in the regulators being configured for the minimum voltage
supported in the DT specified min/max range.  The
qcom,regulator-initial-microvolt property allows us to set a specific
voltage that is larger than the min constraint.

> If this is really still needed, can it be moved to the regulator core?

I'm not opposed to the idea, but I think that Mark is [1]:

>> Do you have a preference for qcom,regulator-initial-microvolt vs a generic
>> framework supported regulator-initial-microvolt property for configuring a
>> specific voltage at registration time?  We'll need to have support for one
>> or the other in order for the qcom_rpmh-regulator driver to be functional.
> 
> This is basically specific to Qualcomm, I can't off hand think of any
> other devices with similar issues.


>> +- regulator-initial-mode
>> +       Usage:      optional; VRM regulators only
>> +       Value type: <u32>
>> +       Definition: Specifies the initial mode to request for a VRM regulator.
>> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
>> +                   in [1] (i.e. 0 to 3).  This property may be specified even
>> +                   if the regulator-allow-set-load property is not specified.
> 
> Every time I read the above I wonder why you're documenting a standard
> regulator regulator property in your bindings.  ...then I realize it's
> because you're doing it because you want to explicitly document what
> the valid modes are.  I wonder if it makes sense to just put a
> reference somewhere else in this document to go look at the header
> file where these are all nicely documented.

Isn't that what the [1] in the above snippet is currently doing.  Further
down in qcom,rpmh-regulator.txt is this line:

+[1] include/dt-bindings/regulator/qcom,rpmh-regulator.h


> Speaking of documenting things like that, it might be worth finding
> somewhere in this doc to mention that the "bob" regulator on PMI8998
> can support "regulator-allow-bypass".  That tidbit got lost when we
> moved to the standard regulator bindings for bypass.

I suppose that I could add something like this:

+- regulator-allow-bypass
+	Usage:      optional; BOB type VRM regulators only
+	Value type: <empty>
+	Definition: See [2] for details.
...
+[2]: Documentation/devicetree/bindings/regulator.txt

However, I don't want the patch to get NACKed because it is defining a
property that is already defined in the common regulator.txt file.


>> +- qcom,allowed-drms-modes
>> +       Usage:      required if regulator-allow-set-load is specified;
>> +                   VRM regulators only
>> +       Value type: <prop-encoded-array>
>> +       Definition: A list of integers specifying the PMIC regulator modes which
>> +                   can be configured at runtime based upon consumer load needs.
>> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
>> +                   in [1] (i.e. 0 to 3).
> 
> Why is this still here?  You moved it to the core regulator framework,
> right?  It's still in your examples too.  Shouldn't this be removed?
> It looks like the driver still needs this and it needs to be an exact
> duplicate of the common binding.  That doesn't seem right...

The qcom,allowed-drms-modes property supports a different feature than the
regulator-allowed-modes property accepted in [2].  The latter specifies
the modes that may be used at all (e.g. in regulator_set_mode() calls) and
it lists the mode values in an unordered fashion.

qcom,allowed-drms-modes defines a specific subset of the possible allowed
modes that should be set based on DRMS (e.g. in regulator_set_load()
calls).  Its values are listed in a specific order and must match 1-to-1
with qcom,drms-mode-max-microamps entries.

It would probably be good to change the name of the property from
qcom,allowed-drms-modes to qcom,regulator-drms-modes.


>> +- qcom,drms-mode-max-microamps
>> +       Usage:      required if regulator-allow-set-load is specified;
>> +                   VRM regulators only
>> +       Value type: <prop-encoded-array>
>> +       Definition: A list of integers specifying the maximum allowed load
>> +                   current in microamps for each of the modes listed in
>> +                   qcom,allowed-drms-modes (matched 1-to-1 in order).  Elements
>> +                   must be specified in order from lowest to highest value.
> 
> Any reason this can't go into the regulator core?  You'd basically
> just take the existing concept of rpmh_regulator_vrm_set_load() and
> put it in the core.

This could be implemented in the core via new constraint elements parsed
in of_regulator and a helper function to specify in regulator_ops.
However, I'm not sure about the wide-spread applicability of this feature.
 I'd prefer to leave it in the driver unless Mark would like me to add it
into the core.


>> +- qcom,headroom-microvolt
>> +       Usage:      optional; VRM regulators only
>> +       Value type: <u32>
>> +       Definition: Specifies the headroom voltage in microvolts to request for
>> +                   a VRM regulator.  RPMh hardware automatically ensures that
>> +                   the parent of this regulator outputs a voltage high enough
>> +                   to satisfy the requested headroom.  Supported values are
>> +                   0 to 511000.
> 
> I'm curious: is this a voted-for value, or a global value?
> 
> Said another way: the whole point of RPMh is that there may be more
> than one processor that needs the same rails, right?  So the AP might
> request 1.1 V for a rail and the modem might request 1.3 V.  RPMh
> would decide to pick the higher of those two (1.3 V), but if the modem
> said it no longer needs the rail it will drop down to 1.1 V.
> 
> ...and as an example of why the headroom needs to be in hardware, if
> the source voltage was normally 1.4 V and the headroom was 200 mV then
> the hardware would need to know to bump up the source voltage to 1.5V
> during the period of of time that the modem wants the rail at 1.3V.
> 
> So my question is: do the AP and modem in the above situation
> separately vote for headroom?  How is it aggregated?  ...or is it a
> global value and this sets the headroom for all clients of RPMh?  It
> would be interesting to document this as it might help with figuring
> out how this value should be set.

The headroom voltage voting is supported in hardware per-regulator and
per-master (AP, modem, etc).  The headroom voltage and output voltage are
each aggregated (using max) per-regulator across masters.  If the
aggregated enable state for a regulator is on, then the aggregated output
voltage and headroom voltage are added together and applied as a min
constraint on the parent's output voltage (if there is a parent).


>> diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
>> new file mode 100644
>> index 0000000..4378c4b
>> --- /dev/null
>> +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
>> +/*
>> + * These mode constants may be used for regulator-initial-mode and
>> + * qcom,allowed-drms-modes properties of an RPMh regulator device tree node.
> 
> Technically also for your new "regulator-allowed-modes".  Maybe just
> say that they're used anywhere a regulator mode is needed in this
> driver and give regulator-initial-mode as an example?

Sure, I'll update this description.

Take care,
David

[1]: https://lkml.org/lkml/2018/4/24/960
[2]: https://lkml.org/lkml/2018/5/11/696

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Thu, May 17, 2018 at 5:16 PM, David Collins <collinsd@codeaurora.org> wrote:
> On 05/17/2018 02:22 PM, Doug Anderson wrote:
>> On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
>>> +- qcom,regulator-initial-microvolt
>>> +       Usage:      optional; VRM regulators only
>>> +       Value type: <u32>
>>> +       Definition: Specifies the initial voltage in microvolts to request for a
>>> +                   VRM regulator.
>>
>> Now that Mark has landed the patch adding support for the
>> -ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
>> we still need the qcom,regulator-initial-microvolt property?
>
> Yes, this is still needed.  The -ENOTRECOVERABLE patch ensures that
> qcom-rpmh-regulator devices can be registered even if
> qcom,regulator-initial-microvolt is not specified.  However, that will
> result in the regulators being configured for the minimum voltage
> supported in the DT specified min/max range.  The
> qcom,regulator-initial-microvolt property allows us to set a specific
> voltage that is larger than the min constraint.

Ah, OK.  In the device tree fragment I saw the initial was always
equal to the min, so I wasn't sure if this was really needed in
practice.  I presume it would only be important if a voltage was left
high by the bootloader for some peripheral that needs to continue to
function (and use the existing higher voltage) until a real device
claims it.  For all other voltages, it should be fine if it's set to
the min until a real device claims it.  Do you have real examples of
devices like this in boards using sdm845?


>> If this is really still needed, can it be moved to the regulator core?
>
> I'm not opposed to the idea, but I think that Mark is [1]:

Oh right.  The downside of weeks between spins I guess.  If Mark is
fine with the private property I won't fight it.


>>> +- regulator-initial-mode
>>> +       Usage:      optional; VRM regulators only
>>> +       Value type: <u32>
>>> +       Definition: Specifies the initial mode to request for a VRM regulator.
>>> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
>>> +                   in [1] (i.e. 0 to 3).  This property may be specified even
>>> +                   if the regulator-allow-set-load property is not specified.
>>
>> Every time I read the above I wonder why you're documenting a standard
>> regulator regulator property in your bindings.  ...then I realize it's
>> because you're doing it because you want to explicitly document what
>> the valid modes are.  I wonder if it makes sense to just put a
>> reference somewhere else in this document to go look at the header
>> file where these are all nicely documented.
>
> Isn't that what the [1] in the above snippet is currently doing.  Further
> down in qcom,rpmh-regulator.txt is this line:
>
> +[1] include/dt-bindings/regulator/qcom,rpmh-regulator.h

Right, but I want to move it so it doesn't look like you're defining a
property that's already defined in the common bindings.  AKA get rid
of the "regulator-initial-mode" property description.  Then add above
Examples:

========================
Regulator Modes
========================

RPMh regulators are designed to work with the standard regulator mode
bindings, using properties like "regulator-initial-mode".  See
include/dt-bindings/regulator/qcom,rpmh-regulator.h for information on
the modes relevant to RPMh regulators.

Some RPMh regulators (BOB regulators only) also support bypass using
the standard "regulator-allow-bypass" binding.


...feel fee to reword, but basically the idea is to document it but
not make it look like you're defining a novel property.


>> Speaking of documenting things like that, it might be worth finding
>> somewhere in this doc to mention that the "bob" regulator on PMI8998
>> can support "regulator-allow-bypass".  That tidbit got lost when we
>> moved to the standard regulator bindings for bypass.
>
> I suppose that I could add something like this:
>
> +- regulator-allow-bypass
> +       Usage:      optional; BOB type VRM regulators only
> +       Value type: <empty>
> +       Definition: See [2] for details.
> ...
> +[2]: Documentation/devicetree/bindings/regulator.txt
>
> However, I don't want the patch to get NACKed because it is defining a
> property that is already defined in the common regulator.txt file.

See above for my suggestion.


>>> +- qcom,allowed-drms-modes
>>> +       Usage:      required if regulator-allow-set-load is specified;
>>> +                   VRM regulators only
>>> +       Value type: <prop-encoded-array>
>>> +       Definition: A list of integers specifying the PMIC regulator modes which
>>> +                   can be configured at runtime based upon consumer load needs.
>>> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
>>> +                   in [1] (i.e. 0 to 3).
>>
>> Why is this still here?  You moved it to the core regulator framework,
>> right?  It's still in your examples too.  Shouldn't this be removed?
>> It looks like the driver still needs this and it needs to be an exact
>> duplicate of the common binding.  That doesn't seem right...
>
> The qcom,allowed-drms-modes property supports a different feature than the
> regulator-allowed-modes property accepted in [2].  The latter specifies
> the modes that may be used at all (e.g. in regulator_set_mode() calls) and
> it lists the mode values in an unordered fashion.
>
> qcom,allowed-drms-modes defines a specific subset of the possible allowed
> modes that should be set based on DRMS (e.g. in regulator_set_load()
> calls).  Its values are listed in a specific order and must match 1-to-1
> with qcom,drms-mode-max-microamps entries.
>
> It would probably be good to change the name of the property from
> qcom,allowed-drms-modes to qcom,regulator-drms-modes.

Ah, I see.  It's unfortunate that now we need to effectively list all
modes twice.  Have you seen real-life examples where these sets of
modes need to be different, or is this just theoretical?  If not can
we start with one property (that controls both things) and if we
really see that we need to specify different sets of modes for the two
cases we can add a separate property?  ...actually, even if you do
have real-life examples of where these need to be different, if 90% of
the time they are the same it would still be nice to just have one
property apply to both cases.


>>> +- qcom,drms-mode-max-microamps
>>> +       Usage:      required if regulator-allow-set-load is specified;
>>> +                   VRM regulators only
>>> +       Value type: <prop-encoded-array>
>>> +       Definition: A list of integers specifying the maximum allowed load
>>> +                   current in microamps for each of the modes listed in
>>> +                   qcom,allowed-drms-modes (matched 1-to-1 in order).  Elements
>>> +                   must be specified in order from lowest to highest value.
>>
>> Any reason this can't go into the regulator core?  You'd basically
>> just take the existing concept of rpmh_regulator_vrm_set_load() and
>> put it in the core.
>
> This could be implemented in the core via new constraint elements parsed
> in of_regulator and a helper function to specify in regulator_ops.
> However, I'm not sure about the wide-spread applicability of this feature.
>  I'd prefer to leave it in the driver unless Mark would like me to add it
> into the core.

You're already using pre-existing APIs around specifying the current
and having the regulator core call you to map the total current into a
mode.  That implies that this is applicable to others.  Adding this
tiny amount of code to the core makes the pre-existing APIs generally
useful.


>>> +- qcom,headroom-microvolt
>>> +       Usage:      optional; VRM regulators only
>>> +       Value type: <u32>
>>> +       Definition: Specifies the headroom voltage in microvolts to request for
>>> +                   a VRM regulator.  RPMh hardware automatically ensures that
>>> +                   the parent of this regulator outputs a voltage high enough
>>> +                   to satisfy the requested headroom.  Supported values are
>>> +                   0 to 511000.
>>
>> I'm curious: is this a voted-for value, or a global value?
>>
>> Said another way: the whole point of RPMh is that there may be more
>> than one processor that needs the same rails, right?  So the AP might
>> request 1.1 V for a rail and the modem might request 1.3 V.  RPMh
>> would decide to pick the higher of those two (1.3 V), but if the modem
>> said it no longer needs the rail it will drop down to 1.1 V.
>>
>> ...and as an example of why the headroom needs to be in hardware, if
>> the source voltage was normally 1.4 V and the headroom was 200 mV then
>> the hardware would need to know to bump up the source voltage to 1.5V
>> during the period of of time that the modem wants the rail at 1.3V.
>>
>> So my question is: do the AP and modem in the above situation
>> separately vote for headroom?  How is it aggregated?  ...or is it a
>> global value and this sets the headroom for all clients of RPMh?  It
>> would be interesting to document this as it might help with figuring
>> out how this value should be set.
>
> The headroom voltage voting is supported in hardware per-regulator and
> per-master (AP, modem, etc).  The headroom voltage and output voltage are
> each aggregated (using max) per-regulator across masters.  If the
> aggregated enable state for a regulator is on, then the aggregated output
> voltage and headroom voltage are added together and applied as a min
> constraint on the parent's output voltage (if there is a parent).

Ah, interesting.  I'm not 100% convinced that the RPMh API is at the
right abstraction level here.  I guess you increase the headroom
voltage if you expect a lot of current and need the regulator to still
give a clean signal?  If you truly wanted to aggregate then if both
the modem and AP wanted to draw a lot of current they would both need
to increase the headroom and then the headroom should maybe not be the
max but something slightly more (you wouldn't want to add, but ...)

Since it's just a max, in theory it seems like you get 99% of the way
there by just using the Linux APIs to deal with dropout voltage.  If
Linux was managing it in software then if it needed to account for
extra headroom it would just increase the supply voltage.  That should
play just fine with the modem (which might be using the hardware
headroom feature) since it will be making its own completely separate
requests and they should be aggregated OK.

In another thread you said you'd be OK dropping the headroom voltage
since it wasn't needed on SDM845.  Maybe we should do that?  ...and if
someone later needs to account for a larger dropout they can figure
out how to hookup the standard linux min_dropout_uV?

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[David Collins]

On 05/17/2018 06:01 PM, Doug Anderson wrote:
> On Thu, May 17, 2018 at 5:16 PM, David Collins <collinsd@codeaurora.org> wrote:
>> On 05/17/2018 02:22 PM, Doug Anderson wrote:
>>> On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
>>>> +- qcom,regulator-initial-microvolt
>>>> +       Usage:      optional; VRM regulators only
>>>> +       Value type: <u32>
>>>> +       Definition: Specifies the initial voltage in microvolts to request for a
>>>> +                   VRM regulator.
>>>
>>> Now that Mark has landed the patch adding support for the
>>> -ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
>>> we still need the qcom,regulator-initial-microvolt property?
>>
>> Yes, this is still needed.  The -ENOTRECOVERABLE patch ensures that
>> qcom-rpmh-regulator devices can be registered even if
>> qcom,regulator-initial-microvolt is not specified.  However, that will
>> result in the regulators being configured for the minimum voltage
>> supported in the DT specified min/max range.  The
>> qcom,regulator-initial-microvolt property allows us to set a specific
>> voltage that is larger than the min constraint.
> 
> Ah, OK.  In the device tree fragment I saw the initial was always
> equal to the min, so I wasn't sure if this was really needed in
> practice.  I presume it would only be important if a voltage was left
> high by the bootloader for some peripheral that needs to continue to
> function (and use the existing higher voltage) until a real device
> claims it.  For all other voltages, it should be fine if it's set to
> the min until a real device claims it.  Do you have real examples of
> devices like this in boards using sdm845?

Something to keep in mind about the downstream rpmh-regulator driver is
that it caches the initial voltages specified in device tree and only
sends them after a consumer driver makes a regulator framework call.  This
saves time during boot and ensures that requests are not made for
regulators that no Linux consumer cares about.

It is generally not safe to request all regulators to be set to the
minimum allowed voltage.  Special care will be needed with the upstream
qcom-rpmh-regulator driver to avoid disrupting the boot up state of
regulators that are needed by other subsystems.  Therefore, I would like
to keep the initial voltage feature supported.


>>>> +- regulator-initial-mode
>>>> +       Usage:      optional; VRM regulators only
>>>> +       Value type: <u32>
>>>> +       Definition: Specifies the initial mode to request for a VRM regulator.
>>>> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
>>>> +                   in [1] (i.e. 0 to 3).  This property may be specified even
>>>> +                   if the regulator-allow-set-load property is not specified.
>>>
>>> Every time I read the above I wonder why you're documenting a standard
>>> regulator regulator property in your bindings.  ...then I realize it's
>>> because you're doing it because you want to explicitly document what
>>> the valid modes are.  I wonder if it makes sense to just put a
>>> reference somewhere else in this document to go look at the header
>>> file where these are all nicely documented.
>>
>> Isn't that what the [1] in the above snippet is currently doing.  Further
>> down in qcom,rpmh-regulator.txt is this line:
>>
>> +[1] include/dt-bindings/regulator/qcom,rpmh-regulator.h
> 
> Right, but I want to move it so it doesn't look like you're defining a
> property that's already defined in the common bindings.  AKA get rid
> of the "regulator-initial-mode" property description.  Then add above
> Examples:
> 
> ========================
> Regulator Modes
> ========================
> 
> RPMh regulators are designed to work with the standard regulator mode
> bindings, using properties like "regulator-initial-mode".  See
> include/dt-bindings/regulator/qcom,rpmh-regulator.h for information on
> the modes relevant to RPMh regulators.
> 
> Some RPMh regulators (BOB regulators only) also support bypass using
> the standard "regulator-allow-bypass" binding.
> 
> 
> ...feel fee to reword, but basically the idea is to document it but
> not make it look like you're defining a novel property.

Ok, I'll try rewording the mode explanation and move it into another
section of the binding doc.


>>> Speaking of documenting things like that, it might be worth finding
>>> somewhere in this doc to mention that the "bob" regulator on PMI8998
>>> can support "regulator-allow-bypass".  That tidbit got lost when we
>>> moved to the standard regulator bindings for bypass.
>>
>> I suppose that I could add something like this:
>>
>> +- regulator-allow-bypass
>> +       Usage:      optional; BOB type VRM regulators only
>> +       Value type: <empty>
>> +       Definition: See [2] for details.
>> ...
>> +[2]: Documentation/devicetree/bindings/regulator.txt
>>
>> However, I don't want the patch to get NACKed because it is defining a
>> property that is already defined in the common regulator.txt file.
> 
> See above for my suggestion.

Ok.


>>>> +- qcom,allowed-drms-modes
>>>> +       Usage:      required if regulator-allow-set-load is specified;
>>>> +                   VRM regulators only
>>>> +       Value type: <prop-encoded-array>
>>>> +       Definition: A list of integers specifying the PMIC regulator modes which
>>>> +                   can be configured at runtime based upon consumer load needs.
>>>> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
>>>> +                   in [1] (i.e. 0 to 3).
>>>
>>> Why is this still here?  You moved it to the core regulator framework,
>>> right?  It's still in your examples too.  Shouldn't this be removed?
>>> It looks like the driver still needs this and it needs to be an exact
>>> duplicate of the common binding.  That doesn't seem right...
>>
>> The qcom,allowed-drms-modes property supports a different feature than the
>> regulator-allowed-modes property accepted in [2].  The latter specifies
>> the modes that may be used at all (e.g. in regulator_set_mode() calls) and
>> it lists the mode values in an unordered fashion.
>>
>> qcom,allowed-drms-modes defines a specific subset of the possible allowed
>> modes that should be set based on DRMS (e.g. in regulator_set_load()
>> calls).  Its values are listed in a specific order and must match 1-to-1
>> with qcom,drms-mode-max-microamps entries.
>>
>> It would probably be good to change the name of the property from
>> qcom,allowed-drms-modes to qcom,regulator-drms-modes.
> 
> Ah, I see.  It's unfortunate that now we need to effectively list all
> modes twice.  Have you seen real-life examples where these sets of
> modes need to be different, or is this just theoretical?  If not can
> we start with one property (that controls both things) and if we
> really see that we need to specify different sets of modes for the two
> cases we can add a separate property?  ...actually, even if you do
> have real-life examples of where these need to be different, if 90% of
> the time they are the same it would still be nice to just have one
> property apply to both cases.

I plan to keep qcom,regulator-drms-modes (and
qcom,drms-mode-max-microamps) around as a property specifically handled
for qcom-rpmh-regulator.  It serves a purpose that is distinct from that
of the generic regulator-allowed-modes.  Without it, there will not be a
way to utilize regulator_set_load() to configure the regulator modes.


>>>> +- qcom,drms-mode-max-microamps
>>>> +       Usage:      required if regulator-allow-set-load is specified;
>>>> +                   VRM regulators only
>>>> +       Value type: <prop-encoded-array>
>>>> +       Definition: A list of integers specifying the maximum allowed load
>>>> +                   current in microamps for each of the modes listed in
>>>> +                   qcom,allowed-drms-modes (matched 1-to-1 in order).  Elements
>>>> +                   must be specified in order from lowest to highest value.
>>>
>>> Any reason this can't go into the regulator core?  You'd basically
>>> just take the existing concept of rpmh_regulator_vrm_set_load() and
>>> put it in the core.
>>
>> This could be implemented in the core via new constraint elements parsed
>> in of_regulator and a helper function to specify in regulator_ops.
>> However, I'm not sure about the wide-spread applicability of this feature.
>>  I'd prefer to leave it in the driver unless Mark would like me to add it
>> into the core.
> 
> You're already using pre-existing APIs around specifying the current
> and having the regulator core call you to map the total current into a
> mode.  That implies that this is applicable to others.  Adding this
> tiny amount of code to the core makes the pre-existing APIs generally
> useful.

I don't see the benefit of making struct regulation_constraints more
complicated with DRMS mode and current arrays that would only every be
used by the qcom-rpmh-regulator driver.  Other regulator drivers are able
to hard code this information in the driver code using get_optimum_mode()
callbacks.

As a side note, changing qcom-rpmh-regulator to use a get_optimum_mode()
callback instead of a set_load() callback would probably be a good idea too.


>>>> +- qcom,headroom-microvolt
>>>> +       Usage:      optional; VRM regulators only
>>>> +       Value type: <u32>
>>>> +       Definition: Specifies the headroom voltage in microvolts to request for
>>>> +                   a VRM regulator.  RPMh hardware automatically ensures that
>>>> +                   the parent of this regulator outputs a voltage high enough
>>>> +                   to satisfy the requested headroom.  Supported values are
>>>> +                   0 to 511000.
>>>
>>> I'm curious: is this a voted-for value, or a global value?
>>>
>>> Said another way: the whole point of RPMh is that there may be more
>>> than one processor that needs the same rails, right?  So the AP might
>>> request 1.1 V for a rail and the modem might request 1.3 V.  RPMh
>>> would decide to pick the higher of those two (1.3 V), but if the modem
>>> said it no longer needs the rail it will drop down to 1.1 V.
>>>
>>> ...and as an example of why the headroom needs to be in hardware, if
>>> the source voltage was normally 1.4 V and the headroom was 200 mV then
>>> the hardware would need to know to bump up the source voltage to 1.5V
>>> during the period of of time that the modem wants the rail at 1.3V.
>>>
>>> So my question is: do the AP and modem in the above situation
>>> separately vote for headroom?  How is it aggregated?  ...or is it a
>>> global value and this sets the headroom for all clients of RPMh?  It
>>> would be interesting to document this as it might help with figuring
>>> out how this value should be set.
>>
>> The headroom voltage voting is supported in hardware per-regulator and
>> per-master (AP, modem, etc).  The headroom voltage and output voltage are
>> each aggregated (using max) per-regulator across masters.  If the
>> aggregated enable state for a regulator is on, then the aggregated output
>> voltage and headroom voltage are added together and applied as a min
>> constraint on the parent's output voltage (if there is a parent).
> 
> Ah, interesting.  I'm not 100% convinced that the RPMh API is at the
> right abstraction level here.  I guess you increase the headroom
> voltage if you expect a lot of current and need the regulator to still
> give a clean signal?  If you truly wanted to aggregate then if both
> the modem and AP wanted to draw a lot of current they would both need
> to increase the headroom and then the headroom should maybe not be the
> max but something slightly more (you wouldn't want to add, but ...)
> 
> Since it's just a max, in theory it seems like you get 99% of the way
> there by just using the Linux APIs to deal with dropout voltage.  If
> Linux was managing it in software then if it needed to account for
> extra headroom it would just increase the supply voltage.  That should
> play just fine with the modem (which might be using the hardware
> headroom feature) since it will be making its own completely separate
> requests and they should be aggregated OK.
> 
> In another thread you said you'd be OK dropping the headroom voltage
> since it wasn't needed on SDM845.  Maybe we should do that?  ...and if
> someone later needs to account for a larger dropout they can figure
> out how to hookup the standard linux min_dropout_uV?

I will remove qcom,headroom-microvolt.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Fri, May 18, 2018 at 5:46 PM, David Collins <collinsd@codeaurora.org> wrote:
> On 05/17/2018 06:01 PM, Doug Anderson wrote:
>> On Thu, May 17, 2018 at 5:16 PM, David Collins <collinsd@codeaurora.org> wrote:
>>> On 05/17/2018 02:22 PM, Doug Anderson wrote:
>>>> On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
>>>>> +- qcom,regulator-initial-microvolt
>>>>> +       Usage:      optional; VRM regulators only
>>>>> +       Value type: <u32>
>>>>> +       Definition: Specifies the initial voltage in microvolts to request for a
>>>>> +                   VRM regulator.
>>>>
>>>> Now that Mark has landed the patch adding support for the
>>>> -ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
>>>> we still need the qcom,regulator-initial-microvolt property?
>>>
>>> Yes, this is still needed.  The -ENOTRECOVERABLE patch ensures that
>>> qcom-rpmh-regulator devices can be registered even if
>>> qcom,regulator-initial-microvolt is not specified.  However, that will
>>> result in the regulators being configured for the minimum voltage
>>> supported in the DT specified min/max range.  The
>>> qcom,regulator-initial-microvolt property allows us to set a specific
>>> voltage that is larger than the min constraint.
>>
>> Ah, OK.  In the device tree fragment I saw the initial was always
>> equal to the min, so I wasn't sure if this was really needed in
>> practice.  I presume it would only be important if a voltage was left
>> high by the bootloader for some peripheral that needs to continue to
>> function (and use the existing higher voltage) until a real device
>> claims it.  For all other voltages, it should be fine if it's set to
>> the min until a real device claims it.  Do you have real examples of
>> devices like this in boards using sdm845?
>
> Something to keep in mind about the downstream rpmh-regulator driver is
> that it caches the initial voltages specified in device tree and only
> sends them after a consumer driver makes a regulator framework call. This
> saves time during boot and ensures that requests are not made for
> regulators that no Linux consumer cares about.

You're saying that in the downstream driver you'd specify
"initial-voltage" in the device tree and:

* This voltage would be reported by any subsequent get_voltage() calls

* This voltage would _not_ be communicated to RPMh.

That seems really strange because you're essentially going to be
returning something from get_voltage() that could be a lie.  You don't
know if the BIOS actually set the value or not but you'll claim that
it did.  It also doesn't seem to match what I see in the downstream
driver.  There I see it read "qcom,init-voltage" and then do a
"rpmh_regulator_set_reg()".  Thus my reading of the downstream driver
is that it should do the same requests that you're doing.


> It is generally not safe to request all regulators to be set to the
> minimum allowed voltage.  Special care will be needed with the upstream
> qcom-rpmh-regulator driver to avoid disrupting the boot up state of
> regulators that are needed by other subsystems.  Therefore, I would like
> to keep the initial voltage feature supported.

I was asking for specific examples.  Do you have any?


BTW: have I already said how terrible of a design it is that you can't
read back the voltages that the BIOS set?  If we could just read back
what the BIOS set then everything would work great and we could stop
talking about this.


>>>>> +- qcom,allowed-drms-modes
>>>>> +       Usage:      required if regulator-allow-set-load is specified;
>>>>> +                   VRM regulators only
>>>>> +       Value type: <prop-encoded-array>
>>>>> +       Definition: A list of integers specifying the PMIC regulator modes which
>>>>> +                   can be configured at runtime based upon consumer load needs.
>>>>> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
>>>>> +                   in [1] (i.e. 0 to 3).
>>>>
>>>> Why is this still here?  You moved it to the core regulator framework,
>>>> right?  It's still in your examples too.  Shouldn't this be removed?
>>>> It looks like the driver still needs this and it needs to be an exact
>>>> duplicate of the common binding.  That doesn't seem right...
>>>
>>> The qcom,allowed-drms-modes property supports a different feature than the
>>> regulator-allowed-modes property accepted in [2].  The latter specifies
>>> the modes that may be used at all (e.g. in regulator_set_mode() calls) and
>>> it lists the mode values in an unordered fashion.
>>>
>>> qcom,allowed-drms-modes defines a specific subset of the possible allowed
>>> modes that should be set based on DRMS (e.g. in regulator_set_load()
>>> calls).  Its values are listed in a specific order and must match 1-to-1
>>> with qcom,drms-mode-max-microamps entries.
>>>
>>> It would probably be good to change the name of the property from
>>> qcom,allowed-drms-modes to qcom,regulator-drms-modes.
>>
>> Ah, I see.  It's unfortunate that now we need to effectively list all
>> modes twice.  Have you seen real-life examples where these sets of
>> modes need to be different, or is this just theoretical?  If not can
>> we start with one property (that controls both things) and if we
>> really see that we need to specify different sets of modes for the two
>> cases we can add a separate property?  ...actually, even if you do
>> have real-life examples of where these need to be different, if 90% of
>> the time they are the same it would still be nice to just have one
>> property apply to both cases.
>
> I plan to keep qcom,regulator-drms-modes (and
> qcom,drms-mode-max-microamps) around as a property specifically handled
> for qcom-rpmh-regulator.  It serves a purpose that is distinct from that
> of the generic regulator-allowed-modes.  Without it, there will not be a
> way to utilize regulator_set_load() to configure the regulator modes.

I guess we'll have to wait for Mark's opinion here.  If it were up to
me I wouldn't accept things with two properties, but if Mark is happy
with it then I won't yell.  To make it really clear what we're talking
about, currently the bindings want you to specify both:

regulator-allowed-modes =
  <RPMH_REGULATOR_MODE_LPM
   RPMH_REGULATOR_MODE_HPM>;
qcom,allowed-drms-modes =
  <RPMH_REGULATOR_MODE_LPM
   RPMH_REGULATOR_MODE_HPM>;
qcom,drms-mode-max-microamps = <1 500000>;

...with the argument that "regulator-allowed-modes" is unordered and
"qcom,allowed-drms-modes" is ordered and needs to match with
"qcom,drms-mode-max-microamps".  ...and also (in theory) you could
come up with an example where the set of allowed modes could be
different sometimes.


>>>>> +- qcom,drms-mode-max-microamps
>>>>> +       Usage:      required if regulator-allow-set-load is specified;
>>>>> +                   VRM regulators only
>>>>> +       Value type: <prop-encoded-array>
>>>>> +       Definition: A list of integers specifying the maximum allowed load
>>>>> +                   current in microamps for each of the modes listed in
>>>>> +                   qcom,allowed-drms-modes (matched 1-to-1 in order).  Elements
>>>>> +                   must be specified in order from lowest to highest value.
>>>>
>>>> Any reason this can't go into the regulator core?  You'd basically
>>>> just take the existing concept of rpmh_regulator_vrm_set_load() and
>>>> put it in the core.
>>>
>>> This could be implemented in the core via new constraint elements parsed
>>> in of_regulator and a helper function to specify in regulator_ops.
>>> However, I'm not sure about the wide-spread applicability of this feature.
>>>  I'd prefer to leave it in the driver unless Mark would like me to add it
>>> into the core.
>>
>> You're already using pre-existing APIs around specifying the current
>> and having the regulator core call you to map the total current into a
>> mode.  That implies that this is applicable to others.  Adding this
>> tiny amount of code to the core makes the pre-existing APIs generally
>> useful.
>
> I don't see the benefit of making struct regulation_constraints more
> complicated with DRMS mode and current arrays that would only every be
> used by the qcom-rpmh-regulator driver.  Other regulator drivers are able
> to hard code this information in the driver code using get_optimum_mode()
> callbacks.

IMO this belongs in the core since it's a generic mechanism and if
drivers want to implement their own custom thing they can, but again
whatever Mark says goes so I guess we'll leave it to him.


> As a side note, changing qcom-rpmh-regulator to use a get_optimum_mode()
> callback instead of a set_load() callback would probably be a good idea too.

Yeah, I remember wondering this earlier and it seemed like it was 6 of
one half dozen of the other.  ...the downside of using
get_optimum_mode() is that it requires a valid input voltage to be
supplied.  It also makes a handful of other calls that you probably
don't need in your case.


-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[David Collins]

On 05/21/2018 11:01 AM, Doug Anderson wrote:
> On Fri, May 18, 2018 at 5:46 PM, David Collins <collinsd@codeaurora.org> wrote:
...
>> Something to keep in mind about the downstream rpmh-regulator driver is
>> that it caches the initial voltages specified in device tree and only
>> sends them after a consumer driver makes a regulator framework call. This
>> saves time during boot and ensures that requests are not made for
>> regulators that no Linux consumer cares about.
> 
> You're saying that in the downstream driver you'd specify
> "initial-voltage" in the device tree and:
> 
> * This voltage would be reported by any subsequent get_voltage() calls
> 
> * This voltage would _not_ be communicated to RPMh.
> 
> That seems really strange because you're essentially going to be
> returning something from get_voltage() that could be a lie.  You don't
> know if the BIOS actually set the value or not but you'll claim that
> it did.  It also doesn't seem to match what I see in the downstream
> driver.  There I see it read "qcom,init-voltage" and then do a
> "rpmh_regulator_set_reg()".  Thus my reading of the downstream driver
> is that it should do the same requests that you're doing.

In the downstream rpmh-regulator driver [1], the value specified via the
"qcom,init-voltage" DT property is only sent to RPMh at probe time if the
"qcom,send-defaults" property is also specified.  "qcom,send-defaults" is
currently specified only for PMI8998 BOB.  The rpmh_regulator_set_reg()
function only updates the cached RPMh request value to be sent in the next
request.  The rpmh_regulator_send_aggregate_requests() function must be
called to actually issue the request.

Returning the cached (but not sent) initial voltage equal to the min
constraint voltage in get_voltage() calls should not cause any problems.
This represents the highest voltage that is guaranteed to be output by the
regulator.  Consumer's should call regulator_set_voltage() to specify
their voltage needs.  If they simply call regulator_enable(), then the
cached voltage will be sent along with the enable request.


>> It is generally not safe to request all regulators to be set to the
>> minimum allowed voltage.  Special care will be needed with the upstream
>> qcom-rpmh-regulator driver to avoid disrupting the boot up state of
>> regulators that are needed by other subsystems.  Therefore, I would like
>> to keep the initial voltage feature supported.
> 
> I was asking for specific examples.  Do you have any?

I was able to track down an example that requires initialization at a
non-minimum voltage: PM8998 LDO 13 on SDM845 MTP.  This regulator supplies
the microSD card with a voltage in the range 1.8 V to 2.96 V.  The boot
loader leaves this regulator enabled at 2.96 V.  It is only guaranteed to
be safe to reduce the voltage to 1.8 V on UHS type cards and only after
following the proper SD identification and command sequence.


> BTW: have I already said how terrible of a design it is that you can't
> read back the voltages that the BIOS set?  If we could just read back
> what the BIOS set then everything would work great and we could stop
> talking about this.

Even if such reading were feasible, it would not help the situation
substantially.  Very few requests are made via the AP RSC before Linux
kernel boot, so 0 V values would still be read back for most regulators.
Additionally, reading the true hardware state (as opposed to what AP RSC
voted before Linux boot) would also not be helpful.  For example, if a
regulator was physically enabled due to a vote via modem RSC and
qcom-rpmh-regulator returned is_enabled() == true inside of a
regulator_enable() call, then no enable request would be made via the AP
RSC which would result in the regulator turning off unexpectedly later
when the modem requests to disable the regulator.

Take care,
David

[1]:
https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree/drivers/regulator/rpmh-regulator.c?h=msm-4.9

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:
> On 05/21/2018 11:01 AM, Doug Anderson wrote:
>> On Fri, May 18, 2018 at 5:46 PM, David Collins <collinsd@codeaurora.org> wrote:
> ...
>>> Something to keep in mind about the downstream rpmh-regulator driver is
>>> that it caches the initial voltages specified in device tree and only
>>> sends them after a consumer driver makes a regulator framework call. This
>>> saves time during boot and ensures that requests are not made for
>>> regulators that no Linux consumer cares about.
>>
>> You're saying that in the downstream driver you'd specify
>> "initial-voltage" in the device tree and:
>>
>> * This voltage would be reported by any subsequent get_voltage() calls
>>
>> * This voltage would _not_ be communicated to RPMh.
>>
>> That seems really strange because you're essentially going to be
>> returning something from get_voltage() that could be a lie.  You don't
>> know if the BIOS actually set the value or not but you'll claim that
>> it did.  It also doesn't seem to match what I see in the downstream
>> driver.  There I see it read "qcom,init-voltage" and then do a
>> "rpmh_regulator_set_reg()".  Thus my reading of the downstream driver
>> is that it should do the same requests that you're doing.
>
> In the downstream rpmh-regulator driver [1], the value specified via the
> "qcom,init-voltage" DT property is only sent to RPMh at probe time if the
> "qcom,send-defaults" property is also specified.  "qcom,send-defaults" is
> currently specified only for PMI8998 BOB.  The rpmh_regulator_set_reg()
> function only updates the cached RPMh request value to be sent in the next
> request.  The rpmh_regulator_send_aggregate_requests() function must be
> called to actually issue the request.

Got it.  This wasn't in the "snapshot" of the downstream driver that I saw.


> Returning the cached (but not sent) initial voltage equal to the min
> constraint voltage in get_voltage() calls should not cause any problems.
> This represents the highest voltage that is guaranteed to be output by the
> regulator.  Consumer's should call regulator_set_voltage() to specify
> their voltage needs.  If they simply call regulator_enable(), then the
> cached voltage will be sent along with the enable request.

I'm still not seeing the argument for initial-voltage here.  If we
added a feature like you describe where we don't send the voltage
until the first enable couldn't we use the minimum voltage here?  If a
consumer calls regulator_enable() without setting a voltage (which
seems like a terrible idea for something where the voltage could vary)
then it would end up at the minimum voltage.


>>> It is generally not safe to request all regulators to be set to the
>>> minimum allowed voltage.  Special care will be needed with the upstream
>>> qcom-rpmh-regulator driver to avoid disrupting the boot up state of
>>> regulators that are needed by other subsystems.  Therefore, I would like
>>> to keep the initial voltage feature supported.
>>
>> I was asking for specific examples.  Do you have any?
>
> I was able to track down an example that requires initialization at a
> non-minimum voltage: PM8998 LDO 13 on SDM845 MTP.  This regulator supplies
> the microSD card with a voltage in the range 1.8 V to 2.96 V.  The boot
> loader leaves this regulator enabled at 2.96 V.  It is only guaranteed to
> be safe to reduce the voltage to 1.8 V on UHS type cards and only after
> following the proper SD identification and command sequence.

Ironically, this is also a perfect example of why we _shouldn't_ have
an "initial-voltage" specified in the device tree.  It is certainly
plausible to imagine a bootloader that might enable UHS speeds on an
SD card and leave the rail on at 1.8V.  Having an initial-voltage
specified in the device tree would be a bad idea here because it's
even worse to transition to 2.96V when the card was expecting 1.8V.

I suppose this is a theoretical example since (as far as I know) no
bootloaders run the micro SD card at UHS speeds, but it is still
worrying that the kernel needs to have a hardcoded initial voltage in
it.  ...basically whenever we're playing with the voltage at bootup
before a regulator has been claimed it's not amazingly safe.
Generally Linux doesn't need to worry about this because it will only
play with voltages if they are out of the constrained range, but in
Qualcomm's case where we can't read the existing voltages then we get
badness.


>> BTW: have I already said how terrible of a design it is that you can't
>> read back the voltages that the BIOS set?  If we could just read back
>> what the BIOS set then everything would work great and we could stop
>> talking about this.
>
> Even if such reading were feasible, it would not help the situation
> substantially.  Very few requests are made via the AP RSC before Linux
> kernel boot, so 0 V values would still be read back for most regulators.

Sure, but all the regulators we're talking about are ones where this
would help.  Said another way: are there any rails that are not
touched by the bootloader where it's bad to set the minimum voltage?


OK, so how's this for a proposal then:

1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
specified that the regulator is enabled then we don't send the
voltage, we just cache it.

2. When we see the first enable then we first send the cached voltage
and then do the enable.

3. We don't need an "initial voltage" because any rails that are
expected to be variable voltage the client should be choosing a
voltage.


...taking the SD card case as an example: if the regulator framework
says "set to the minimum: 1.8V" we'll cache this but not apply it yet
because the rail is off as far as Linux is concerned.  Then when the
SD card framework starts up it will set the voltage to 3.3V which will
overwrite the cache.  Then the SD card framework will enable the
regulator and RPMh will set the voltage to 3.3V and enable.


This whole thing makes me worry about another problem, though.  You
say that the bootloader left the SD card rail on, right?  ...but as
far as Linux is concerned the SD card rail is off (argh, it can't read
the state that the bootloader left the rail in!)

...now imagine any of the following:

* The user boots up a kernel where the SD card driver is disabled.

* The user ejects the SD card right after the bootloader used it but
before the kernel driver started.

When the kernel comes up it will believe that the SD card rail is
disabled so it won't try to disable it.  So the voltage will be left
on.


You can even come up with a less contrived use case here.  One could
argue that the SD card framework really ought to be ensuring VMMC and
VQMMC are off before it starts tweaking with them just in case the
bootloader left them on.  Thus, it should do:

A) Turn off VMMC and VQMMC
B) Program VMMC and VQMMC to defaults
C) Turn on VMMC and VQMMC

...right now we bootup and pretend to Linux that VMMC and VQMMC start
off, so step A) will be no-op.  Sigh.


Do we need to have ".is_enabled" return -ENOTRECOVERABLE to help the
regulator framework understand that we don't know the state?  I think
that might require a pile of patches to the regulator framework, but
it can't be helped unless we can somehow get RPMh to give us back the
status of how the bootloader left us (if we had that, we could return
0 for anything the bootloader didn't touch and that would be correct
from the point of view of the AP).


-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4283 bytes --]

On Tue, May 22, 2018 at 09:43:02AM -0700, Doug Anderson wrote:
> On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:

> > Returning the cached (but not sent) initial voltage equal to the min
> > constraint voltage in get_voltage() calls should not cause any problems.
> > This represents the highest voltage that is guaranteed to be output by the
> > regulator.  Consumer's should call regulator_set_voltage() to specify
> > their voltage needs.  If they simply call regulator_enable(), then the
> > cached voltage will be sent along with the enable request.

> I'm still not seeing the argument for initial-voltage here.  If we
> added a feature like you describe where we don't send the voltage
> until the first enable couldn't we use the minimum voltage here?  If a
> consumer calls regulator_enable() without setting a voltage (which
> seems like a terrible idea for something where the voltage could vary)
> then it would end up at the minimum voltage.

Or if something else has already set a voltage...  though shared voltage
varying regulators aren't a superb idea they do occasionally happen.

> >> I was asking for specific examples.  Do you have any?

> > I was able to track down an example that requires initialization at a
> > non-minimum voltage: PM8998 LDO 13 on SDM845 MTP.  This regulator supplies
> > the microSD card with a voltage in the range 1.8 V to 2.96 V.  The boot
> > loader leaves this regulator enabled at 2.96 V.  It is only guaranteed to
> > be safe to reduce the voltage to 1.8 V on UHS type cards and only after
> > following the proper SD identification and command sequence.

> Ironically, this is also a perfect example of why we _shouldn't_ have
> an "initial-voltage" specified in the device tree.  It is certainly
> plausible to imagine a bootloader that might enable UHS speeds on an
> SD card and leave the rail on at 1.8V.  Having an initial-voltage
> specified in the device tree would be a bad idea here because it's
> even worse to transition to 2.96V when the card was expecting 1.8V.

Right, this sort of situation is why the regulator API has a policy of
leaving things untouched unless it was specifically told to do
something.

> I suppose this is a theoretical example since (as far as I know) no
> bootloaders run the micro SD card at UHS speeds, but it is still

kexec is the most obvious example I can think of here.  You could
probably arrange for something to patch the device tree that's provided
to the kexeced kernel to tell it about the current state but we don't
currently do anything there.

> OK, so how's this for a proposal then:

> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
> specified that the regulator is enabled then we don't send the
> voltage, we just cache it.

> 2. When we see the first enable then we first send the cached voltage
> and then do the enable.

> 3. We don't need an "initial voltage" because any rails that are
> expected to be variable voltage the client should be choosing a
> voltage.

That seems relatively safe.

> You can even come up with a less contrived use case here.  One could
> argue that the SD card framework really ought to be ensuring VMMC and
> VQMMC are off before it starts tweaking with them just in case the
> bootloader left them on.  Thus, it should do:

> A) Turn off VMMC and VQMMC
> B) Program VMMC and VQMMC to defaults
> C) Turn on VMMC and VQMMC

> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
> off, so step A) will be no-op.  Sigh.

> Do we need to have ".is_enabled" return -ENOTRECOVERABLE to help the
> regulator framework understand that we don't know the state?  I think

Yes, we should be doing that.  Then subsystems where it's an issue can
explicitly turn off the supply.

> that might require a pile of patches to the regulator framework, but
> it can't be helped unless we can somehow get RPMh to give us back the
> status of how the bootloader left us (if we had that, we could return
> 0 for anything the bootloader didn't touch and that would be correct
> from the point of view of the AP).

I think it's fine from a framework point of view, just provide an
is_enabled() operation which returns the error.  The required fiddling
in the core should be fairly minor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[David Collins]

On 05/22/2018 09:43 AM, Doug Anderson wrote:
> On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:
...
>> Returning the cached (but not sent) initial voltage equal to the min
>> constraint voltage in get_voltage() calls should not cause any problems.
>> This represents the highest voltage that is guaranteed to be output by the
>> regulator.  Consumer's should call regulator_set_voltage() to specify
>> their voltage needs.  If they simply call regulator_enable(), then the
>> cached voltage will be sent along with the enable request.
> 
> I'm still not seeing the argument for initial-voltage here.  If we
> added a feature like you describe where we don't send the voltage
> until the first enable couldn't we use the minimum voltage here?  If a
> consumer calls regulator_enable() without setting a voltage (which
> seems like a terrible idea for something where the voltage could vary)
> then it would end up at the minimum voltage.

I wasn't proposing the voltage caching feature to be used in the upstream
qcom-rpmh-regulator.  I was explaining exactly how the downstream
rpmh-regulator driver works.

However, if the voltage caching feature is acceptable for upstream usage,
then I could add it.  With that in place, I see less of a need for the
qcom,regulator-initial-microvolt property and would be ok with removing it
for now.


>>> BTW: have I already said how terrible of a design it is that you can't
>>> read back the voltages that the BIOS set?  If we could just read back
>>> what the BIOS set then everything would work great and we could stop
>>> talking about this.
>>
>> Even if such reading were feasible, it would not help the situation
>> substantially.  Very few requests are made via the AP RSC before Linux
>> kernel boot, so 0 V values would still be read back for most regulators.
> 
> Sure, but all the regulators we're talking about are ones where this
> would help.  Said another way: are there any rails that are not
> touched by the bootloader where it's bad to set the minimum voltage?

I'm not sure about this.


> OK, so how's this for a proposal then:
> 
> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
> specified that the regulator is enabled then we don't send the
> voltage, we just cache it.
> 
> 2. When we see the first enable then we first send the cached voltage
> and then do the enable.
> 
> 3. We don't need an "initial voltage" because any rails that are
> expected to be variable voltage the client should be choosing a
> voltage.
> 
> 
> ...taking the SD card case as an example: if the regulator framework
> says "set to the minimum: 1.8V" we'll cache this but not apply it yet
> because the rail is off as far as Linux is concerned.  Then when the
> SD card framework starts up it will set the voltage to 3.3V which will
> overwrite the cache.  Then the SD card framework will enable the
> regulator and RPMh will set the voltage to 3.3V and enable.

I am ok with implementing this feature.

However, should the voltage be cached instead of sent immediately any time
that Linux has not explicitly requested the regulator to be enabled, or
only before the first time that an enable request is sent?

1. regulator_set_voltage(reg, 2960000, 2960000)
   --> cache voltage=2960 mV
2. regulator_enable(reg)
   --> Send voltage=2960 then enable=1
3. regulator_disable(reg)
   --> Send enable=0
4. regulator_set_voltage(reg, 1800000, 2960000)
   --> A. Send voltage=1800 OR B. cache voltage=1800?

Option A is used on the downstream rpmh-regulator driver in order to avoid
cases where AP votes to disable a regulator that is kept enabled by
another subsystem but then does not lower the voltage requested thanks to
regulator_set_voltage() being called after regulator_disable().  I plan to
go with option A for qcom-rpmh-regulator unless there are objections.


> This whole thing makes me worry about another problem, though.  You
> say that the bootloader left the SD card rail on, right?  ...but as
> far as Linux is concerned the SD card rail is off (argh, it can't read
> the state that the bootloader left the rail in!)
> 
> ...now imagine any of the following:
> 
> * The user boots up a kernel where the SD card driver is disabled.
> 
> * The user ejects the SD card right after the bootloader used it but
> before the kernel driver started.
> 
> When the kernel comes up it will believe that the SD card rail is
> disabled so it won't try to disable it.  So the voltage will be left
> on.

We have not encountered issues with regulators getting left on
indefinitely because Linux devices failed to take control of them during
boot.  I don't think that this hypothetical issue needs to be addressed in
the first qcom-rpmh-regulator driver patch if at all.


> You can even come up with a less contrived use case here.  One could
> argue that the SD card framework really ought to be ensuring VMMC and
> VQMMC are off before it starts tweaking with them just in case the
> bootloader left them on.  Thus, it should do:
> 
> A) Turn off VMMC and VQMMC
> B) Program VMMC and VQMMC to defaults
> C) Turn on VMMC and VQMMC
> 
> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
> off, so step A) will be no-op.  Sigh.

Step A) would not work because the regulator's use_count would be 0 and
regulator_disable() can only be called successfully if use_count > 0.  The
call would have no impact and it would return an error.

I don't think that this is an avenue that we want to pursue.  Consumers
should not be expected to call regulator_disable() before regulator_enable().


> Do we need to have ".is_enabled" return -ENOTRECOVERABLE to help the
> regulator framework understand that we don't know the state?  I think
> that might require a pile of patches to the regulator framework, but
> it can't be helped unless we can somehow get RPMh to give us back the
> status of how the bootloader left us (if we had that, we could return
> 0 for anything the bootloader didn't touch and that would be correct
> from the point of view of the AP).

I'm not following what the regulator framework would do as a result of
is_enabled() returning -ENOTRECOVERABLE.  If it saw this while processing
a regulator_enable() call then it would continue to enable the regulator.
This value could not be seen while handling a regulator_disable() call
since the is_enabled() callback is not invoked in the disable call-path.
This also seems like an optimization for a problem that we are not
encountering now (or likely to ever encounter).  Therefore, I would
suggest that we not try to work this into the initial qcom-rpmh-regulator
patch.

Thanks,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Tue, May 22, 2018 at 3:46 PM, David Collins <collinsd@codeaurora.org> wrote:
> On 05/22/2018 09:43 AM, Doug Anderson wrote:
>> On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:
> ...
>>> Returning the cached (but not sent) initial voltage equal to the min
>>> constraint voltage in get_voltage() calls should not cause any problems.
>>> This represents the highest voltage that is guaranteed to be output by the
>>> regulator.  Consumer's should call regulator_set_voltage() to specify
>>> their voltage needs.  If they simply call regulator_enable(), then the
>>> cached voltage will be sent along with the enable request.
>>
>> I'm still not seeing the argument for initial-voltage here.  If we
>> added a feature like you describe where we don't send the voltage
>> until the first enable couldn't we use the minimum voltage here?  If a
>> consumer calls regulator_enable() without setting a voltage (which
>> seems like a terrible idea for something where the voltage could vary)
>> then it would end up at the minimum voltage.
>
> I wasn't proposing the voltage caching feature to be used in the upstream
> qcom-rpmh-regulator.  I was explaining exactly how the downstream
> rpmh-regulator driver works.
>
> However, if the voltage caching feature is acceptable for upstream usage,
> then I could add it.  With that in place, I see less of a need for the
> qcom,regulator-initial-microvolt property and would be ok with removing it
> for now.

I think it's the right thing to do an Mark didn't seem to yell, so I'd
say go for it.


>> OK, so how's this for a proposal then:
>>
>> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
>> specified that the regulator is enabled then we don't send the
>> voltage, we just cache it.
>>
>> 2. When we see the first enable then we first send the cached voltage
>> and then do the enable.
>>
>> 3. We don't need an "initial voltage" because any rails that are
>> expected to be variable voltage the client should be choosing a
>> voltage.
>>
>>
>> ...taking the SD card case as an example: if the regulator framework
>> says "set to the minimum: 1.8V" we'll cache this but not apply it yet
>> because the rail is off as far as Linux is concerned.  Then when the
>> SD card framework starts up it will set the voltage to 3.3V which will
>> overwrite the cache.  Then the SD card framework will enable the
>> regulator and RPMh will set the voltage to 3.3V and enable.
>
> I am ok with implementing this feature.
>
> However, should the voltage be cached instead of sent immediately any time
> that Linux has not explicitly requested the regulator to be enabled, or
> only before the first time that an enable request is sent?
>
> 1. regulator_set_voltage(reg, 2960000, 2960000)
>    --> cache voltage=2960 mV
> 2. regulator_enable(reg)
>    --> Send voltage=2960 then enable=1
> 3. regulator_disable(reg)
>    --> Send enable=0
> 4. regulator_set_voltage(reg, 1800000, 2960000)
>    --> A. Send voltage=1800 OR B. cache voltage=1800?
>
> Option A is used on the downstream rpmh-regulator driver in order to avoid
> cases where AP votes to disable a regulator that is kept enabled by
> another subsystem but then does not lower the voltage requested thanks to
> regulator_set_voltage() being called after regulator_disable().  I plan to
> go with option A for qcom-rpmh-regulator unless there are objections.

So one client's vote for a voltage continues to be in effect even if
that client votes to have the regulator disabled?  That seems
fundamentally broken in RPMh.  I guess my take would be to work around
this RPMh misfeature by saying that whenever Linux votes to disable a
regulator it also votes for a voltage of 0.  Then another client of
RPMh won't be affected.

That seems like it would be beneficial in any case.  If the AP always
asks for 1.3 V and the modem always asks for 1.1 V for the same rail
then the rail should go down to 1.1 V when the AP says to disable the
rail.


>> This whole thing makes me worry about another problem, though.  You
>> say that the bootloader left the SD card rail on, right?  ...but as
>> far as Linux is concerned the SD card rail is off (argh, it can't read
>> the state that the bootloader left the rail in!)
>>
>> ...now imagine any of the following:
>>
>> * The user boots up a kernel where the SD card driver is disabled.
>>
>> * The user ejects the SD card right after the bootloader used it but
>> before the kernel driver started.
>>
>> When the kernel comes up it will believe that the SD card rail is
>> disabled so it won't try to disable it.  So the voltage will be left
>> on.
>
> We have not encountered issues with regulators getting left on
> indefinitely because Linux devices failed to take control of them during
> boot.  I don't think that this hypothetical issue needs to be addressed in
> the first qcom-rpmh-regulator driver patch if at all.

I don't think it hypothetical at all.  Linux takes control of a
regulator and then at bootup it disables all regulators that aren't
currently used/enabled.  In your case you will report that regulators
are already disabled and thus you'll neuter the kernel's attempt to
disable regulators that nobody is using but that might have been left
on by the bootloader.  It seemed like Mark agreed here.


>> You can even come up with a less contrived use case here.  One could
>> argue that the SD card framework really ought to be ensuring VMMC and
>> VQMMC are off before it starts tweaking with them just in case the
>> bootloader left them on.  Thus, it should do:
>>
>> A) Turn off VMMC and VQMMC
>> B) Program VMMC and VQMMC to defaults
>> C) Turn on VMMC and VQMMC
>>
>> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
>> off, so step A) will be no-op.  Sigh.
>
> Step A) would not work because the regulator's use_count would be 0 and
> regulator_disable() can only be called successfully if use_count > 0.  The
> call would have no impact and it would return an error.

Are you sure regulator_force_disable() won't do the trick on most
boards (which will report the regulator being enabled at bootup)?  I
haven't tried it, but it seems like it might.

...I think you're right that this is a theoretical case at the moment
because I don't think the MMC framework attempts to get this right,
but I don't have time to dig right now.  I think it just sets the
voltage to 3.3V and turns the rail on and if the card thinks it should
be at 1.8V because the bootloader left the card in that state then:
whoops.  I'd have to walk through the regulator framework more closely
to confirm that though.  Certainly on all other boards besides ones
using RPMh the bootloader can leave regulators on and the kernel can
query that state.  It seems sane that there would be some way to turn
a regulator in this state off.  I know for a fact that if you just
leave the regulator alone (don't claim it or try to enable it) that
Linux will turn it off.


> I don't think that this is an avenue that we want to pursue.  Consumers
> should not be expected to call regulator_disable() before regulator_enable().
>
>
>> Do we need to have ".is_enabled" return -ENOTRECOVERABLE to help the
>> regulator framework understand that we don't know the state?  I think
>> that might require a pile of patches to the regulator framework, but
>> it can't be helped unless we can somehow get RPMh to give us back the
>> status of how the bootloader left us (if we had that, we could return
>> 0 for anything the bootloader didn't touch and that would be correct
>> from the point of view of the AP).
>
> I'm not following what the regulator framework would do as a result of
> is_enabled() returning -ENOTRECOVERABLE.  If it saw this while processing
> a regulator_enable() call then it would continue to enable the regulator.

The important use case here is at bootup when we try to disable all
unused regulators.  We need the framework to know that these
regulators might not be disabled so it should go ahead and try to
disable them.


> This value could not be seen while handling a regulator_disable() call
> since the is_enabled() callback is not invoked in the disable call-path.
> This also seems like an optimization for a problem that we are not
> encountering now (or likely to ever encounter).  Therefore, I would
> suggest that we not try to work this into the initial qcom-rpmh-regulator
> patch.

I think you haven't thought through the bootup case of disabling all
unused regulators.  When you look at that path I think you'll find
it's important to make sure that the regulator framework knows that
you don't know if you're disabled or enabled yet.  I think you want to
look at regulator_late_cleanup().  Note that right now
regulator_late_cleanup() doesn't handle error codes from is_enabled
(actually, several places in the regulator core don't), but actually
since the error case and the "enabled" case are probably the same this
might be OK.


-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[David Collins]

On 05/22/2018 05:08 PM, Doug Anderson wrote:
> On Tue, May 22, 2018 at 3:46 PM, David Collins <collinsd@codeaurora.org> wrote:
>> On 05/22/2018 09:43 AM, Doug Anderson wrote:
>>> On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:
...
>> However, if the voltage caching feature is acceptable for upstream usage,
>> then I could add it.  With that in place, I see less of a need for the
>> qcom,regulator-initial-microvolt property and would be ok with removing it
>> for now.
> 
> I think it's the right thing to do an Mark didn't seem to yell, so I'd
> say go for it.

I'll remove qcom,regulator-initial-microvolt support and add voltage caching.


>>> OK, so how's this for a proposal then:
>>>
>>> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
>>> specified that the regulator is enabled then we don't send the
>>> voltage, we just cache it.
>>>
>>> 2. When we see the first enable then we first send the cached voltage
>>> and then do the enable.
>>>
>>> 3. We don't need an "initial voltage" because any rails that are
>>> expected to be variable voltage the client should be choosing a
>>> voltage.
>>>
>>>
>>> ...taking the SD card case as an example: if the regulator framework
>>> says "set to the minimum: 1.8V" we'll cache this but not apply it yet
>>> because the rail is off as far as Linux is concerned.  Then when the
>>> SD card framework starts up it will set the voltage to 3.3V which will
>>> overwrite the cache.  Then the SD card framework will enable the
>>> regulator and RPMh will set the voltage to 3.3V and enable.
>>
>> I am ok with implementing this feature.
>>
>> However, should the voltage be cached instead of sent immediately any time
>> that Linux has not explicitly requested the regulator to be enabled, or
>> only before the first time that an enable request is sent?
>>
>> 1. regulator_set_voltage(reg, 2960000, 2960000)
>>    --> cache voltage=2960 mV
>> 2. regulator_enable(reg)
>>    --> Send voltage=2960 then enable=1
>> 3. regulator_disable(reg)
>>    --> Send enable=0
>> 4. regulator_set_voltage(reg, 1800000, 2960000)
>>    --> A. Send voltage=1800 OR B. cache voltage=1800?
>>
>> Option A is used on the downstream rpmh-regulator driver in order to avoid
>> cases where AP votes to disable a regulator that is kept enabled by
>> another subsystem but then does not lower the voltage requested thanks to
>> regulator_set_voltage() being called after regulator_disable().  I plan to
>> go with option A for qcom-rpmh-regulator unless there are objections.
> 
> So one client's vote for a voltage continues to be in effect even if
> that client votes to have the regulator disabled?  That seems
> fundamentally broken in RPMh.  I guess my take would be to work around
> this RPMh misfeature by saying that whenever Linux votes to disable a
> regulator it also votes for a voltage of 0.  Then another client of
> RPMh won't be affected.
> 
> That seems like it would be beneficial in any case.  If the AP always
> asks for 1.3 V and the modem always asks for 1.1 V for the same rail
> then the rail should go down to 1.1 V when the AP says to disable the
> rail.

The VRM in RPMh hardware aggregates enable state, output voltage, mode,
and headroom voltage requests independently for each regulator.  This
allows for maximum request flexibility and makes no assumptions about
consumer use cases.  There is nothing inherently wrong about this approach.

I'm concerned about a blanket policy of setting voltage=0 when issuing
disable requests.  That seems to violate the semantics of the
regulator_set_voltage() API which enforces the requested voltage range
until a new range is specified.  There may be workarounds that require a
regulator to operate at a specific voltage even when no Linux consumers
explicitly need the regulator enabled.

Given that not masking the voltage on disable is guaranteed to be safe and
does not silently break potential use cases, I plan to stick with this
approach.  Therefore, the question about option A or B for voltage caching
is still relevant.  I think that option A is safer/more API conforming so
I plan to implement that.


>>> This whole thing makes me worry about another problem, though.  You
>>> say that the bootloader left the SD card rail on, right?  ...but as
>>> far as Linux is concerned the SD card rail is off (argh, it can't read
>>> the state that the bootloader left the rail in!)
>>>
>>> ...now imagine any of the following:
>>>
>>> * The user boots up a kernel where the SD card driver is disabled.
>>>
>>> * The user ejects the SD card right after the bootloader used it but
>>> before the kernel driver started.
>>>
>>> When the kernel comes up it will believe that the SD card rail is
>>> disabled so it won't try to disable it.  So the voltage will be left
>>> on.
>>
>> We have not encountered issues with regulators getting left on
>> indefinitely because Linux devices failed to take control of them during
>> boot.  I don't think that this hypothetical issue needs to be addressed in
>> the first qcom-rpmh-regulator driver patch if at all.
> 
> I don't think it hypothetical at all.  Linux takes control of a
> regulator and then at bootup it disables all regulators that aren't
> currently used/enabled.  In your case you will report that regulators
> are already disabled and thus you'll neuter the kernel's attempt to
> disable regulators that nobody is using but that might have been left
> on by the bootloader.  It seemed like Mark agreed here.

I did not consider regulator_late_cleanup().  I'll initialize the enabled
value in qcom-rpmh-regulator to -EINVAL to handle this case and also so
that _regulator_enable() succeeds without further core modification.


>>> You can even come up with a less contrived use case here.  One could
>>> argue that the SD card framework really ought to be ensuring VMMC and
>>> VQMMC are off before it starts tweaking with them just in case the
>>> bootloader left them on.  Thus, it should do:
>>>
>>> A) Turn off VMMC and VQMMC
>>> B) Program VMMC and VQMMC to defaults
>>> C) Turn on VMMC and VQMMC
>>>
>>> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
>>> off, so step A) will be no-op.  Sigh.
>>
>> Step A) would not work because the regulator's use_count would be 0 and
>> regulator_disable() can only be called successfully if use_count > 0.  The
>> call would have no impact and it would return an error.
> 
> Are you sure regulator_force_disable() won't do the trick on most
> boards (which will report the regulator being enabled at bootup)?  I
> haven't tried it, but it seems like it might.

regulator_force_disable() would indeed call the disable() callback.
However, this function is designed for emergency uses only and will cause
the use_count to become out of sync with the requested regulator state.  I
don't think that we want to suggest to consumers that they call
regulator_force_disable().  It would completely break any shared regulator
uses cases between multiple Linux consumers.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Tue, May 22, 2018 at 6:19 PM, David Collins <collinsd@codeaurora.org> wrote:
>>>> OK, so how's this for a proposal then:
>>>>
>>>> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
>>>> specified that the regulator is enabled then we don't send the
>>>> voltage, we just cache it.
>>>>
>>>> 2. When we see the first enable then we first send the cached voltage
>>>> and then do the enable.
>>>>
>>>> 3. We don't need an "initial voltage" because any rails that are
>>>> expected to be variable voltage the client should be choosing a
>>>> voltage.
>>>>
>>>>
>>>> ...taking the SD card case as an example: if the regulator framework
>>>> says "set to the minimum: 1.8V" we'll cache this but not apply it yet
>>>> because the rail is off as far as Linux is concerned.  Then when the
>>>> SD card framework starts up it will set the voltage to 3.3V which will
>>>> overwrite the cache.  Then the SD card framework will enable the
>>>> regulator and RPMh will set the voltage to 3.3V and enable.
>>>
>>> I am ok with implementing this feature.
>>>
>>> However, should the voltage be cached instead of sent immediately any time
>>> that Linux has not explicitly requested the regulator to be enabled, or
>>> only before the first time that an enable request is sent?
>>>
>>> 1. regulator_set_voltage(reg, 2960000, 2960000)
>>>    --> cache voltage=2960 mV
>>> 2. regulator_enable(reg)
>>>    --> Send voltage=2960 then enable=1
>>> 3. regulator_disable(reg)
>>>    --> Send enable=0
>>> 4. regulator_set_voltage(reg, 1800000, 2960000)
>>>    --> A. Send voltage=1800 OR B. cache voltage=1800?
>>>
>>> Option A is used on the downstream rpmh-regulator driver in order to avoid
>>> cases where AP votes to disable a regulator that is kept enabled by
>>> another subsystem but then does not lower the voltage requested thanks to
>>> regulator_set_voltage() being called after regulator_disable().  I plan to
>>> go with option A for qcom-rpmh-regulator unless there are objections.
>>
>> So one client's vote for a voltage continues to be in effect even if
>> that client votes to have the regulator disabled?  That seems
>> fundamentally broken in RPMh.  I guess my take would be to work around
>> this RPMh misfeature by saying that whenever Linux votes to disable a
>> regulator it also votes for a voltage of 0.  Then another client of
>> RPMh won't be affected.
>>
>> That seems like it would be beneficial in any case.  If the AP always
>> asks for 1.3 V and the modem always asks for 1.1 V for the same rail
>> then the rail should go down to 1.1 V when the AP says to disable the
>> rail.
>
> The VRM in RPMh hardware aggregates enable state, output voltage, mode,
> and headroom voltage requests independently for each regulator.  This
> allows for maximum request flexibility and makes no assumptions about
> consumer use cases.  There is nothing inherently wrong about this approach.

Just to confirm I'm understanding correctly:

1. AP: request that regulator A be at 1.3 V and enabled
==> actual output: regulator A is 1.3 V and enabled

2. modem: requests that regulator A be at 1.1 V and enabled
==> actual output: regulator A is 1.3 V and enabled

3. AP: request that regulator A be disabled

You're saying that here the output of regulator A will be "1.3 V" and
"enabled".

I just can't see how that can be correct behavior.  A given client's
vote for the voltage should really only make sense if that client
wants the regulator enabled.  In the case above the kernel would have
no idea that someone else might have the regulator enabled.  Why would
it care if that other user gets it at 1.3 V or at 1.1 V?

You have some use case in mind where the kernel would need to have
control over the voltage of a regulator that it thinks is disabled?

Now obviously if the kernel re-enables the regulator then its old
voltage vote should be re-instated right away, but until then its vote
about the voltage shouldn't count.  If that means that the kernel has
to "vote" for 0V then I guess that's the way the API works.


> I'm concerned about a blanket policy of setting voltage=0 when issuing
> disable requests.  That seems to violate the semantics of the
> regulator_set_voltage() API which enforces the requested voltage range
> until a new range is specified.  There may be workarounds that require a
> regulator to operate at a specific voltage even when no Linux consumers
> explicitly need the regulator enabled.
>
> Given that not masking the voltage on disable is guaranteed to be safe and
> does not silently break potential use cases, I plan to stick with this
> approach.  Therefore, the question about option A or B for voltage caching
> is still relevant.  I think that option A is safer/more API conforming so
> I plan to implement that.

I still can't understand how it ever makes sense to vote for a voltage
for a regulator that you think is disabled.  ...but if there's some
reason it does then I guess I'm OK with A.


>>>> This whole thing makes me worry about another problem, though.  You
>>>> say that the bootloader left the SD card rail on, right?  ...but as
>>>> far as Linux is concerned the SD card rail is off (argh, it can't read
>>>> the state that the bootloader left the rail in!)
>>>>
>>>> ...now imagine any of the following:
>>>>
>>>> * The user boots up a kernel where the SD card driver is disabled.
>>>>
>>>> * The user ejects the SD card right after the bootloader used it but
>>>> before the kernel driver started.
>>>>
>>>> When the kernel comes up it will believe that the SD card rail is
>>>> disabled so it won't try to disable it.  So the voltage will be left
>>>> on.
>>>
>>> We have not encountered issues with regulators getting left on
>>> indefinitely because Linux devices failed to take control of them during
>>> boot.  I don't think that this hypothetical issue needs to be addressed in
>>> the first qcom-rpmh-regulator driver patch if at all.
>>
>> I don't think it hypothetical at all.  Linux takes control of a
>> regulator and then at bootup it disables all regulators that aren't
>> currently used/enabled.  In your case you will report that regulators
>> are already disabled and thus you'll neuter the kernel's attempt to
>> disable regulators that nobody is using but that might have been left
>> on by the bootloader.  It seemed like Mark agreed here.
>
> I did not consider regulator_late_cleanup().  I'll initialize the enabled
> value in qcom-rpmh-regulator to -EINVAL to handle this case and also so
> that _regulator_enable() succeeds without further core modification.

That's weird that the regulator code has a special case for -EINVAL in
_regulator_enable().  Given how most of the code in the
regulator/core.c doesn't seem to check for error codes I wonder if
anyone is using that...  I guess I'd leave it to Mark whether he's
happy with -EINVAL for this case even though it's asymmetric to using
-ENOTRECOVERABLE for getting the voltage.

Are we really sure there aren't any places that the regulator code
needs to handle an error from _regulator_enable()?  An an example, in
regulator_resolve_supply() are we sure we should be passing a
regulator_enable() on to our parent supply even if
_regulator_is_enabled() returns an error?  I haven't looked in depth
of all use cases, though...


I see you posted a new version.  Thanks!  I'll try to find some time
soon to review it, but I'll be a bit busy over the next few days.

-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Tue, May 22, 2018 at 05:08:45PM -0700, Doug Anderson wrote:

> So one client's vote for a voltage continues to be in effect even if
> that client votes to have the regulator disabled?  That seems
> fundamentally broken in RPMh.  I guess my take would be to work around

It's arguable either way - you could say that the client gets to specify
a safe range at all times or you could say that the machine constraints
should cover all cases where the hardware is idling.  Of course RPMh
is missing anything like the machine constraints (as we can see from all
the fixing up of undesirable hard coding we have to do) so it's kind of
pushed towards the first case.

> >> A) Turn off VMMC and VQMMC
> >> B) Program VMMC and VQMMC to defaults
> >> C) Turn on VMMC and VQMMC

> >> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
> >> off, so step A) will be no-op.  Sigh.

> > Step A) would not work because the regulator's use_count would be 0 and
> > regulator_disable() can only be called successfully if use_count > 0.  The
> > call would have no impact and it would return an error.

> Are you sure regulator_force_disable() won't do the trick on most
> boards (which will report the regulator being enabled at bootup)?  I
> haven't tried it, but it seems like it might.

It does mean that things will go wrong if the regulator is shared.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 23, 2018 at 1:29 AM, Mark Brown <broonie@kernel.org> wrote:

> It's arguable either way - you could say that the client gets to specify
> a safe range at all times or you could say that the machine constraints
> should cover all cases where the hardware is idling.  Of course RPMh
> is missing anything like the machine constraints (as we can see from all
> the fixing up of undesirable hard coding we have to do) so it's kind of
> pushed towards the first case.

OK, fair enough.  If others all agree that it's OK to make requests
about the voltage of a disabled regulator then I won't stand in the
way.  I guess it does call into question the whole idea of caching /
not sending the voltage until the first enable because it means
there's no way for the client to use this feature until they've
enabled / disabled the regulator once.  If you think of it as invalid
to adjust the voltage of a disabled regulator then the caching
argument is super clean, but once you say that you should normally be
able to do it it feels more like a hacky workaround.  ...but I suppose
that's what we've got to live with...

-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]

On Wed, May 23, 2018 at 08:23:22AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, May 23, 2018 at 1:29 AM, Mark Brown <broonie@kernel.org> wrote:
> 
> > It's arguable either way - you could say that the client gets to specify
> > a safe range at all times or you could say that the machine constraints
> > should cover all cases where the hardware is idling.  Of course RPMh
> > is missing anything like the machine constraints (as we can see from all
> > the fixing up of undesirable hard coding we have to do) so it's kind of
> > pushed towards the first case.

> OK, fair enough.  If others all agree that it's OK to make requests
> about the voltage of a disabled regulator then I won't stand in the
> way.  I guess it does call into question the whole idea of caching /
> not sending the voltage until the first enable because it means
> there's no way for the client to use this feature until they've
> enabled / disabled the regulator once.  If you think of it as invalid
> to adjust the voltage of a disabled regulator then the caching
> argument is super clean, but once you say that you should normally be

It's got to be valid to think about the voltage of a disabled regulator
since drivers want to be able make sure that the regulator gets enabled
with a sensible config.  With most hardware this is really easy since
you can just look at the status reported by the hardware but the RPM
makes this hard since there's so much write only stuff in there.

> able to do it it feels more like a hacky workaround.  ...but I suppose
> that's what we've got to live with...

These RPM systems are always going to have problems of some kind
unfortunately unless the interface changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 23, 2018 at 8:40 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 23, 2018 at 08:23:22AM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Wed, May 23, 2018 at 1:29 AM, Mark Brown <broonie@kernel.org> wrote:
>>
>> > It's arguable either way - you could say that the client gets to specify
>> > a safe range at all times or you could say that the machine constraints
>> > should cover all cases where the hardware is idling.  Of course RPMh
>> > is missing anything like the machine constraints (as we can see from all
>> > the fixing up of undesirable hard coding we have to do) so it's kind of
>> > pushed towards the first case.
>
>> OK, fair enough.  If others all agree that it's OK to make requests
>> about the voltage of a disabled regulator then I won't stand in the
>> way.  I guess it does call into question the whole idea of caching /
>> not sending the voltage until the first enable because it means
>> there's no way for the client to use this feature until they've
>> enabled / disabled the regulator once.  If you think of it as invalid
>> to adjust the voltage of a disabled regulator then the caching
>> argument is super clean, but once you say that you should normally be
>
> It's got to be valid to think about the voltage of a disabled regulator
> since drivers want to be able make sure that the regulator gets enabled
> with a sensible config.  With most hardware this is really easy since
> you can just look at the status reported by the hardware but the RPM
> makes this hard since there's so much write only stuff in there.

I should be more clear.  Certainly it should be valid to set the
voltage before enabling it so, as you said, the regulator turns on at
the right voltage.  I'm saying that it's weird (to me) to expect that
setting the voltage for a regulator that a client thinks is disabled
will affect any real voltages in the system until the regulator is
enabled.  In RPMh apparently setting a voltage of a regulator you
think is disabled can affect the regulator output if another client
(unbeknownst to you) happens to have it enabled.

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On Wed, May 23, 2018 at 08:50:22AM -0700, Doug Anderson wrote:
> On Wed, May 23, 2018 at 8:40 AM, Mark Brown <broonie@kernel.org> wrote:

> > It's got to be valid to think about the voltage of a disabled regulator
> > since drivers want to be able make sure that the regulator gets enabled
> > with a sensible config.  With most hardware this is really easy since
> > you can just look at the status reported by the hardware but the RPM
> > makes this hard since there's so much write only stuff in there.

> I should be more clear.  Certainly it should be valid to set the
> voltage before enabling it so, as you said, the regulator turns on at
> the right voltage.  I'm saying that it's weird (to me) to expect that
> setting the voltage for a regulator that a client thinks is disabled
> will affect any real voltages in the system until the regulator is
> enabled.  In RPMh apparently setting a voltage of a regulator you
> think is disabled can affect the regulator output if another client
> (unbeknownst to you) happens to have it enabled.

Yes, that's definitely not what's expected but it's unfortunately what
the firmware chose to implement so we may well be stuck with it
unfortunately.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 23, 2018 at 8:56 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 23, 2018 at 08:50:22AM -0700, Doug Anderson wrote:
>> On Wed, May 23, 2018 at 8:40 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > It's got to be valid to think about the voltage of a disabled regulator
>> > since drivers want to be able make sure that the regulator gets enabled
>> > with a sensible config.  With most hardware this is really easy since
>> > you can just look at the status reported by the hardware but the RPM
>> > makes this hard since there's so much write only stuff in there.
>
>> I should be more clear.  Certainly it should be valid to set the
>> voltage before enabling it so, as you said, the regulator turns on at
>> the right voltage.  I'm saying that it's weird (to me) to expect that
>> setting the voltage for a regulator that a client thinks is disabled
>> will affect any real voltages in the system until the regulator is
>> enabled.  In RPMh apparently setting a voltage of a regulator you
>> think is disabled can affect the regulator output if another client
>> (unbeknownst to you) happens to have it enabled.
>
> Yes, that's definitely not what's expected but it's unfortunately what
> the firmware chose to implement so we may well be stuck with it
> unfortunately.

We're not really stuck with it if we do what I was suggesting.  I was
suggesting that every time we disable the regulator in Linux we have
Linux vote for the lowest voltage it's comfortable with.  Linux keeps
track of the true voltage that the driver wants and will always change
its vote back to that before enabling.  Thus (assuming Linux is OK
with 1.2 V - 1.4 V for a rail):

Modem: want 1.3 V and enabled.
=> Modem votes for 1.3 V
=> Modem votes for enabled.
=> At least one vote for enabled? Yes
=> Highest voltage vote: 1.3V
=> True output: 1.3V

Linux: want 1.4 V and enabled.
=> Linux votes for 1.4 V
=> Linux votes for enabled
=> At least one vote for enabled? Yes
=> Highest voltage vote: 1.4V
=> True output: 1.4V

Linux: want disabled
=> Linux votes for disabled
=> Linux votes for 1.2 V (keeps 1.4 V in local var)
=> At least one vote for enabled? Yes
=> Highest voltage vote: 1.3V
=> True output: 1.3V

Linux: want enabled
=> Linux votes for 1.4 V (from local var)
=> Linux votes for enabled
=> At least one vote for enabled? Yes
=> Highest voltage vote: 1.4V
=> True output: 1.4V


...but I'll leave it to you if you think this is a big deal.  If
you're happy with how David's driver works without my suggestion then
I won't push it.

-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

On Tue, May 29, 2018 at 10:30:33PM -0700, Doug Anderson wrote:
> On Wed, May 23, 2018 at 8:56 AM, Mark Brown <broonie@kernel.org> wrote:

> > Yes, that's definitely not what's expected but it's unfortunately what
> > the firmware chose to implement so we may well be stuck with it
> > unfortunately.

> We're not really stuck with it if we do what I was suggesting.  I was
> suggesting that every time we disable the regulator in Linux we have
> Linux vote for the lowest voltage it's comfortable with.  Linux keeps
> track of the true voltage that the driver wants and will always change
> its vote back to that before enabling.  Thus (assuming Linux is OK
> with 1.2 V - 1.4 V for a rail):

That's pretty much what it should do anyway with normally designed
hardware.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 30, 2018 at 2:37 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, May 29, 2018 at 10:30:33PM -0700, Doug Anderson wrote:
>> On Wed, May 23, 2018 at 8:56 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > Yes, that's definitely not what's expected but it's unfortunately what
>> > the firmware chose to implement so we may well be stuck with it
>> > unfortunately.
>
>> We're not really stuck with it if we do what I was suggesting.  I was
>> suggesting that every time we disable the regulator in Linux we have
>> Linux vote for the lowest voltage it's comfortable with.  Linux keeps
>> track of the true voltage that the driver wants and will always change
>> its vote back to that before enabling.  Thus (assuming Linux is OK
>> with 1.2 V - 1.4 V for a rail):
>
> That's pretty much what it should do anyway with normally designed
> hardware.

I guess the question is: do we insist that the driver include this
workaround, or are we OK with letting the hardware behave as the
hardware does?

-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

On Wed, May 30, 2018 at 07:46:50AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 2:37 AM, Mark Brown <broonie@kernel.org> wrote:

> >> Linux vote for the lowest voltage it's comfortable with.  Linux keeps
> >> track of the true voltage that the driver wants and will always change
> >> its vote back to that before enabling.  Thus (assuming Linux is OK
> >> with 1.2 V - 1.4 V for a rail):

> > That's pretty much what it should do anyway with normally designed
> > hardware.

> I guess the question is: do we insist that the driver include this
> workaround, or are we OK with letting the hardware behave as the
> hardware does?

What you're describing sounds like what we should be doing normally, if
we're not doing that we should probably be fixing the core.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 30, 2018 at 8:02 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 07:46:50AM -0700, Doug Anderson wrote:
>> On Wed, May 30, 2018 at 2:37 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> >> Linux vote for the lowest voltage it's comfortable with.  Linux keeps
>> >> track of the true voltage that the driver wants and will always change
>> >> its vote back to that before enabling.  Thus (assuming Linux is OK
>> >> with 1.2 V - 1.4 V for a rail):
>
>> > That's pretty much what it should do anyway with normally designed
>> > hardware.
>
>> I guess the question is: do we insist that the driver include this
>> workaround, or are we OK with letting the hardware behave as the
>> hardware does?
>
> What you're describing sounds like what we should be doing normally, if
> we're not doing that we should probably be fixing the core.

I'm not convinced that this behavior makes sense to move to the core.
On most regulators I'd expect that when the regulator driver says to
turn them off that they will output no voltage.  The reason RPMh is
special is that there's an aggregation outside of Linux.  Specifically
I like to make it concrete use the example where both Linux and "the
modem" make requests for the same regulator and RPMh aggregates these
requests.  This aggregation happens separately for "enabled" and
"voltage".

Thus if you consider:
Modem: vote for 1.3V and enabled=true
Linux: vote for 1.4V and enabled=false

The aggregated voltage is 1.4V and the aggregated enabled is true.
Said another way: Linux's vote for the voltage affected the state of
the rail even though Linux wanted the rail disabled.

In any other system when Linux disabled the regulator it wouldn't
matter that you left it set at 1.4V.  Thus RPMh is special and IMO the
workaround belongs there.


-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

On Wed, May 30, 2018 at 08:34:50AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 8:02 AM, Mark Brown <broonie@kernel.org> wrote:

> > What you're describing sounds like what we should be doing normally, if
> > we're not doing that we should probably be fixing the core.

> I'm not convinced that this behavior makes sense to move to the core.
> On most regulators I'd expect that when the regulator driver says to
> turn them off that they will output no voltage.  The reason RPMh is

Oh, you mean while the regulator is off...  TBH I don't see a huge
problem doing that anyway and just reverting to the bottom of the
constraints when everything gets turned off since we have to see if we
need to adjust the voltage every time we enabled anyway.

> In any other system when Linux disabled the regulator it wouldn't
> matter that you left it set at 1.4V.  Thus RPMh is special and IMO the
> workaround belongs there.

Without the core doing something the regulator isn't going to get told
that anything updated voltages anyway...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 30, 2018 at 8:48 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 08:34:50AM -0700, Doug Anderson wrote:
>> On Wed, May 30, 2018 at 8:02 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > What you're describing sounds like what we should be doing normally, if
>> > we're not doing that we should probably be fixing the core.
>
>> I'm not convinced that this behavior makes sense to move to the core.
>> On most regulators I'd expect that when the regulator driver says to
>> turn them off that they will output no voltage.  The reason RPMh is
>
> Oh, you mean while the regulator is off...  TBH I don't see a huge
> problem doing that anyway and just reverting to the bottom of the
> constraints when everything gets turned off since we have to see if we
> need to adjust the voltage every time we enabled anyway.
>
>> In any other system when Linux disabled the regulator it wouldn't
>> matter that you left it set at 1.4V.  Thus RPMh is special and IMO the
>> workaround belongs there.
>
> Without the core doing something the regulator isn't going to get told
> that anything updated voltages anyway...

I was just suggesting that when the core tells the regulator driver to
disable itself that the regulator driver tell RPMh to not only disable
itself but also (temporarily) vote for a lower voltage.  When the core
tells the regulator to re-enable itself then the regulator driver
restores the original voltage vote (or applies any vote that might
have been attempted while the regulator was disabled).  That wouldn't
require any regulator core changes.

-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Wed, May 30, 2018 at 09:06:16AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 8:48 AM, Mark Brown <broonie@kernel.org> wrote:

> > Without the core doing something the regulator isn't going to get told
> > that anything updated voltages anyway...

> I was just suggesting that when the core tells the regulator driver to
> disable itself that the regulator driver tell RPMh to not only disable
> itself but also (temporarily) vote for a lower voltage.  When the core
> tells the regulator to re-enable itself then the regulator driver
> restores the original voltage vote (or applies any vote that might
> have been attempted while the regulator was disabled).  That wouldn't
> require any regulator core changes.

It needs something to tell it what the new voltage to set is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 30, 2018 at 9:07 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 09:06:16AM -0700, Doug Anderson wrote:
>> On Wed, May 30, 2018 at 8:48 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > Without the core doing something the regulator isn't going to get told
>> > that anything updated voltages anyway...
>
>> I was just suggesting that when the core tells the regulator driver to
>> disable itself that the regulator driver tell RPMh to not only disable
>> itself but also (temporarily) vote for a lower voltage.  When the core
>> tells the regulator to re-enable itself then the regulator driver
>> restores the original voltage vote (or applies any vote that might
>> have been attempted while the regulator was disabled).  That wouldn't
>> require any regulator core changes.
>
> It needs something to tell it what the new voltage to set is.

The regulator driver has its own cache of what voltage was most
recently requested by Linux.  It can use that, can't it?

-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

On Wed, May 30, 2018 at 09:09:02AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 9:07 AM, Mark Brown <broonie@kernel.org> wrote:

> > It needs something to tell it what the new voltage to set is.

> The regulator driver has its own cache of what voltage was most
> recently requested by Linux.  It can use that, can't it?

If we're just going to use the most recently set voltage then hopefully
the hardware already knew that, and it might not be the lowest available
voltage if the last consumer to get turned off was holding the voltage
higher.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 30, 2018 at 9:13 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 09:09:02AM -0700, Doug Anderson wrote:
>> On Wed, May 30, 2018 at 9:07 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > It needs something to tell it what the new voltage to set is.
>
>> The regulator driver has its own cache of what voltage was most
>> recently requested by Linux.  It can use that, can't it?
>
> If we're just going to use the most recently set voltage then hopefully
> the hardware already knew that, and it might not be the lowest available
> voltage if the last consumer to get turned off was holding the voltage
> higher.

To circle back to the original point: the problem is that (IMHO) the
hardware is doing the wrong thing by still counting Linux's vote for a
voltage even though Linux also voted that the regulator should be
disabled.  So basically we're working around the hardware by
pretending to vote for a dummy lower voltage whenever Linux wants the
regulator disabled.  From Linux's point of view everything works as
normal--we just tell the hardware a falsehood so it doesn't count our
vote for a voltage while the regulator is disabled.

-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On Wed, May 30, 2018 at 09:31:55AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 9:13 AM, Mark Brown <broonie@kernel.org> wrote:

> > If we're just going to use the most recently set voltage then hopefully
> > the hardware already knew that, and it might not be the lowest available
> > voltage if the last consumer to get turned off was holding the voltage
> > higher.

> To circle back to the original point: the problem is that (IMHO) the
> hardware is doing the wrong thing by still counting Linux's vote for a
> voltage even though Linux also voted that the regulator should be
> disabled.  So basically we're working around the hardware by
> pretending to vote for a dummy lower voltage whenever Linux wants the
> regulator disabled.  From Linux's point of view everything works as
> normal--we just tell the hardware a falsehood so it doesn't count our
> vote for a voltage while the regulator is disabled.

Yeah, and I don't think that's unreasonable for the core to do - just
drop the voltage to the constraint minimum after it has turned off the
regulator, then recheck and raise if needed before it enables again.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Doug Anderson]

Hi,

On Wed, May 30, 2018 at 9:36 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 09:31:55AM -0700, Doug Anderson wrote:
>> On Wed, May 30, 2018 at 9:13 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > If we're just going to use the most recently set voltage then hopefully
>> > the hardware already knew that, and it might not be the lowest available
>> > voltage if the last consumer to get turned off was holding the voltage
>> > higher.
>
>> To circle back to the original point: the problem is that (IMHO) the
>> hardware is doing the wrong thing by still counting Linux's vote for a
>> voltage even though Linux also voted that the regulator should be
>> disabled.  So basically we're working around the hardware by
>> pretending to vote for a dummy lower voltage whenever Linux wants the
>> regulator disabled.  From Linux's point of view everything works as
>> normal--we just tell the hardware a falsehood so it doesn't count our
>> vote for a voltage while the regulator is disabled.
>
> Yeah, and I don't think that's unreasonable for the core to do - just
> drop the voltage to the constraint minimum after it has turned off the
> regulator, then recheck and raise if needed before it enables again.

Would it do this for all regulators, though?  Most regulators are
hooked up over a slow i2c bus, so that means that every regulator
disable would now do an extra i2c transfer even though for all
regulators (other than RPMh) the voltage of a regulator doesn't matter
when it's been "disabled' (from Linux's point of view).

Hrmmm, I suppose maybe it'd be OK though because for most regulators
we'd use "regcache" and most regulators that we enabled/disable a lot
are not expected to change voltage in Linux (so the regcache write
would hopefully be a noop), so maybe it wouldn't be _that_
inefficient...

-Doug

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

On Wed, May 30, 2018 at 09:41:55AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 9:36 AM, Mark Brown <broonie@kernel.org> wrote:

> > Yeah, and I don't think that's unreasonable for the core to do - just
> > drop the voltage to the constraint minimum after it has turned off the
> > regulator, then recheck and raise if needed before it enables again.

> Would it do this for all regulators, though?  Most regulators are
> hooked up over a slow i2c bus, so that means that every regulator
> disable would now do an extra i2c transfer even though for all
> regulators (other than RPMh) the voltage of a regulator doesn't matter
> when it's been "disabled' (from Linux's point of view).

It'd only affect regulators that can vary in voltage and actually get
disabled which is a pretty small subset.  Most regulators are either
fixed voltage or always on.

> Hrmmm, I suppose maybe it'd be OK though because for most regulators
> we'd use "regcache" and most regulators that we enabled/disable a lot
> are not expected to change voltage in Linux (so the regcache write
> would hopefully be a noop), so maybe it wouldn't be _that_
> inefficient...

Even if the regulator lacks a cache we'd at least skip out on the write
when we're not changing voltage as we'd do a read/modify/update cycle
and stop at the modify.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings

[Rob Herring]

On Thu, May 17, 2018 at 05:16:13PM -0700, David Collins wrote:
> On 05/17/2018 02:22 PM, Doug Anderson wrote:
> > On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
> >> +- qcom,regulator-initial-microvolt
> >> +       Usage:      optional; VRM regulators only
> >> +       Value type: <u32>
> >> +       Definition: Specifies the initial voltage in microvolts to request for a
> >> +                   VRM regulator.
> > 
> > Now that Mark has landed the patch adding support for the
> > -ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
> > we still need the qcom,regulator-initial-microvolt property?
> 
> Yes, this is still needed.  The -ENOTRECOVERABLE patch ensures that
> qcom-rpmh-regulator devices can be registered even if
> qcom,regulator-initial-microvolt is not specified.  However, that will
> result in the regulators being configured for the minimum voltage
> supported in the DT specified min/max range.  The
> qcom,regulator-initial-microvolt property allows us to set a specific
> voltage that is larger than the min constraint.
> 
> > If this is really still needed, can it be moved to the regulator core?
> 
> I'm not opposed to the idea, but I think that Mark is [1]:
> 
> >> Do you have a preference for qcom,regulator-initial-microvolt vs a generic
> >> framework supported regulator-initial-microvolt property for configuring a
> >> specific voltage at registration time?  We'll need to have support for one
> >> or the other in order for the qcom_rpmh-regulator driver to be functional.
> > 
> > This is basically specific to Qualcomm, I can't off hand think of any
> > other devices with similar issues.
> 
> 
> >> +- regulator-initial-mode
> >> +       Usage:      optional; VRM regulators only
> >> +       Value type: <u32>
> >> +       Definition: Specifies the initial mode to request for a VRM regulator.
> >> +                   Supported values are RPMH_REGULATOR_MODE_* which are defined
> >> +                   in [1] (i.e. 0 to 3).  This property may be specified even
> >> +                   if the regulator-allow-set-load property is not specified.
> > 
> > Every time I read the above I wonder why you're documenting a standard
> > regulator regulator property in your bindings.  ...then I realize it's
> > because you're doing it because you want to explicitly document what
> > the valid modes are.  I wonder if it makes sense to just put a
> > reference somewhere else in this document to go look at the header
> > file where these are all nicely documented.
> 
> Isn't that what the [1] in the above snippet is currently doing.  Further
> down in qcom,rpmh-regulator.txt is this line:
> 
> +[1] include/dt-bindings/regulator/qcom,rpmh-regulator.h
> 
> 
> > Speaking of documenting things like that, it might be worth finding
> > somewhere in this doc to mention that the "bob" regulator on PMI8998
> > can support "regulator-allow-bypass".  That tidbit got lost when we
> > moved to the standard regulator bindings for bypass.
> 
> I suppose that I could add something like this:
> 
> +- regulator-allow-bypass
> +	Usage:      optional; BOB type VRM regulators only
> +	Value type: <empty>
> +	Definition: See [2] for details.
> ...
> +[2]: Documentation/devicetree/bindings/regulator.txt
> 
> However, I don't want the patch to get NACKed because it is defining a
> property that is already defined in the common regulator.txt file.

If all constraints are defined in the common doc, just "see 
regulator.txt" is fine. You just need to say what properties this 
binding uses.

Rob

[PATCH v3 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

Add the QCOM RPMh regulator driver to manage PMIC regulators
which are controlled via RPMh on some Qualcomm Technologies, Inc.
SoCs.  RPMh is a hardware block which contains several
accelerators which are used to manage various hardware resources
that are shared between the processors of the SoC.  The final
hardware state of a regulator is determined within RPMh by
performing max aggregation of the requests made by all of the
processors.

Add support for PMIC regulator control via the voltage regulator
manager (VRM) and oscillator buffer (XOB) RPMh accelerators.  VRM
supports manipulation of enable state, voltage, mode, and
headroom voltage.  XOB supports manipulation of enable state.

Signed-off-by: David Collins <collinsd@codeaurora.org>
---
 drivers/regulator/Kconfig               |   9 +
 drivers/regulator/Makefile              |   1 +
 drivers/regulator/qcom-rpmh-regulator.c | 925 ++++++++++++++++++++++++++++++++
 3 files changed, 935 insertions(+)
 create mode 100644 drivers/regulator/qcom-rpmh-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 4efae3b..1a69bdc 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
 	  Qualcomm RPM as a module. The module will be named
 	  "qcom_rpm-regulator".
 
+config REGULATOR_QCOM_RPMH
+	tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
+	depends on QCOM_RPMH || COMPILE_TEST
+	help
+	  This driver supports control of PMIC regulators via the RPMh hardware
+	  block found on Qualcomm Technologies Inc. SoCs.  RPMh regulator
+	  control allows for voting on regulator state between multiple
+	  processors within the SoC.
+
 config REGULATOR_QCOM_SMD_RPM
 	tristate "Qualcomm SMD based RPM regulator driver"
 	depends on QCOM_SMD_RPM
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index d81fb02..906f048 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_REGULATOR_MT6323)	+= mt6323-regulator.o
 obj-$(CONFIG_REGULATOR_MT6380)	+= mt6380-regulator.o
 obj-$(CONFIG_REGULATOR_MT6397)	+= mt6397-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o
 obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
new file mode 100644
index 0000000..991ecc1
--- /dev/null
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -0,0 +1,925 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
+#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+
+/**
+ * enum rpmh_regulator_type - supported RPMh accelerator types
+ * %VRM:	RPMh VRM accelerator which supports voting on enable, voltage,
+ *		mode, and headroom voltage of LDO, SMPS, and BOB type PMIC
+ *		regulators.
+ * %XOB:	RPMh XOB accelerator which supports voting on the enable state
+ *		of PMIC regulators.
+ */
+enum rpmh_regulator_type {
+	VRM,
+	XOB,
+};
+
+#define RPMH_VRM_HEADROOM_MAX_UV		511000
+
+#define RPMH_REGULATOR_REG_VRM_VOLTAGE		0x0
+#define RPMH_REGULATOR_REG_ENABLE		0x4
+#define RPMH_REGULATOR_REG_VRM_MODE		0x8
+#define RPMH_REGULATOR_REG_VRM_HEADROOM		0xC
+
+#define RPMH_REGULATOR_MODE_COUNT		4
+
+#define PMIC4_LDO_MODE_RETENTION		4
+#define PMIC4_LDO_MODE_LPM			5
+#define PMIC4_LDO_MODE_HPM			7
+
+#define PMIC4_SMPS_MODE_RETENTION		4
+#define PMIC4_SMPS_MODE_PFM			5
+#define PMIC4_SMPS_MODE_AUTO			6
+#define PMIC4_SMPS_MODE_PWM			7
+
+#define PMIC4_BOB_MODE_PASS			0
+#define PMIC4_BOB_MODE_PFM			1
+#define PMIC4_BOB_MODE_AUTO			2
+#define PMIC4_BOB_MODE_PWM			3
+
+/**
+ * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
+ * @regulator_type:		RPMh accelerator type used to manage this
+ *				regulator
+ * @ops:			Pointer to regulator ops callback structure
+ * @voltage_range:		The single range of voltages supported by this
+ *				PMIC regulator type
+ * @n_voltages:			The number of unique voltage set points defined
+ *				by voltage_range
+ * @pmic_mode_map:		Array indexed by regulator framework mode
+ *				containing PMIC hardware modes.  Must be large
+ *				enough to index all framework modes supported
+ *				by this regulator hardware type.
+ * @of_map_mode:		Maps an RPMH_REGULATOR_MODE_* mode value defined
+ *				in device tree to a regulator framework mode
+ */
+struct rpmh_vreg_hw_data {
+	enum rpmh_regulator_type		regulator_type;
+	const struct regulator_ops		*ops;
+	const struct regulator_linear_range	voltage_range;
+	int					n_voltages;
+	const int				*pmic_mode_map;
+	unsigned int			      (*of_map_mode)(unsigned int mode);
+};
+
+/**
+ * struct rpmh_vreg - individual RPMh regulator data structure encapsulating a
+ *		single regulator device
+ * @dev:			Device pointer for the top-level PMIC RPMh
+ *				regulator parent device.  This is used as a
+ *				handle in RPMh write requests.
+ * @addr:			Base address of the regulator resource within
+ *				an RPMh accelerator
+ * @rdesc:			Regulator descriptor
+ * @hw_data:			PMIC regulator configuration data for this RPMh
+ *				regulator
+ * @always_wait_for_ack:	Boolean flag indicating if a request must always
+ *				wait for an ACK from RPMh before continuing even
+ *				if it corresponds to a strictly lower power
+ *				state (e.g. enabled --> disabled).
+ * @drms_modes:			Array of regulator framework modes which can
+ *				be configured dynamically for this regulator
+ *				via the set_load() callback.
+ * @drms_mode_max_uAs:		Array of maximum load currents in microamps
+ *				supported by the corresponding modes in
+ *				drms_mode.  Elements must be specified in
+ *				strictly increasing order.
+ * @drms_mode_count:		The number of elements in drms_mode array.
+ * @enabled:			Boolean indicating if the regulator is enabled
+ *				or not
+ * @bypassed:			Boolean indicating if the regulator is in
+ *				bypass (pass-through) mode or not.  This is
+ *				only used by BOB rpmh-regulator resources.
+ * @voltage_selector:		Selector used for get_voltage_sel() and
+ *				set_voltage_sel() callbacks
+ * @mode:			RPMh VRM regulator current framework mode
+ */
+struct rpmh_vreg {
+	struct device			*dev;
+	u32				addr;
+	struct regulator_desc		rdesc;
+	const struct rpmh_vreg_hw_data	*hw_data;
+	bool				always_wait_for_ack;
+	unsigned int			*drms_modes;
+	int				*drms_mode_max_uAs;
+	size_t				drms_mode_count;
+
+	bool				enabled;
+	bool				bypassed;
+	int				voltage_selector;
+	unsigned int			mode;
+};
+
+/**
+ * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
+ * @name:			Name for the regulator which also corresponds
+ *				to the device tree subnode name of the regulator
+ * @resource_name:		RPMh regulator resource name format string.
+ *				This must include exactly one field: '%s' which
+ *				is filled at run-time with the PMIC ID provided
+ *				by device tree property qcom,pmic-id.  Example:
+ *				"ldo%s1" for RPMh resource "ldoa1".
+ * @supply_name:		Parent supply regulator name
+ * @hw_data:			Configuration data for this PMIC regulator type
+ */
+struct rpmh_vreg_init_data {
+	const char			*name;
+	const char			*resource_name;
+	const char			*supply_name;
+	const struct rpmh_vreg_hw_data	*hw_data;
+};
+
+/**
+ * rpmh_regulator_send_request() - send the request to RPMh
+ * @vreg:		Pointer to the RPMh regulator
+ * @cmd:		RPMh commands to send
+ * @count:		Size of cmd array
+ * @wait_for_ack:	Boolean indicating if execution must wait until the
+ *			request has been acknowledged as complete
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
+			struct tcs_cmd *cmd, int count, bool wait_for_ack)
+{
+	int ret;
+
+	if (wait_for_ack || vreg->always_wait_for_ack)
+		ret = rpmh_write(vreg->dev, RPMH_ACTIVE_ONLY_STATE, cmd, count);
+	else
+		ret = rpmh_write_async(vreg->dev, RPMH_ACTIVE_ONLY_STATE, cmd,
+					count);
+
+	return ret;
+}
+
+static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+	return vreg->enabled;
+}
+
+static int rpmh_regulator_enable(struct regulator_dev *rdev)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+		.data = 1,
+	};
+	int ret;
+
+	ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
+
+	if (!ret)
+		vreg->enabled = true;
+
+	return ret;
+}
+
+static int rpmh_regulator_disable(struct regulator_dev *rdev)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+		.data = 0,
+	};
+	int ret;
+
+	ret = rpmh_regulator_send_request(vreg, &cmd, 1, false);
+
+	if (!ret)
+		vreg->enabled = false;
+
+	return ret;
+}
+
+static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
+				unsigned int selector)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+	};
+	int ret;
+
+	/* VRM voltage control register is set with voltage in millivolts. */
+	cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
+							selector), 1000);
+
+	ret = rpmh_regulator_send_request(vreg, &cmd, 1,
+					  selector > vreg->voltage_selector);
+	if (!ret)
+		vreg->voltage_selector = selector;
+
+	return 0;
+}
+
+static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+	return vreg->voltage_selector;
+}
+
+static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
+					unsigned int mode, bool bypassed)
+{
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+	};
+	int pmic_mode;
+
+	if (mode > REGULATOR_MODE_STANDBY)
+		return -EINVAL;
+
+	pmic_mode = vreg->hw_data->pmic_mode_map[mode];
+	if (pmic_mode < 0)
+		return pmic_mode;
+
+	cmd.data = bypassed ? PMIC4_BOB_MODE_PASS : pmic_mode;
+
+	return rpmh_regulator_send_request(vreg, &cmd, 1, true);
+}
+
+static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
+					unsigned int mode)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	int ret;
+
+	if (mode == vreg->mode)
+		return 0;
+
+	ret = rpmh_regulator_vrm_set_mode_bypass(vreg, mode, vreg->bypassed);
+	if (!ret)
+		vreg->mode = mode;
+
+	return ret;
+}
+
+static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+	return vreg->mode;
+}
+
+/**
+ * rpmh_regulator_vrm_set_load() - set the regulator mode based upon the load
+ *		current requested
+ * @rdev:		Regulator device pointer for the rpmh-regulator
+ * @load_uA:		Aggregated load current in microamps
+ *
+ * This function is used in the regulator_ops for VRM type RPMh regulator
+ * devices.  It updates the mode of the regulator using a table of maximum
+ * load currents per mode specified in device tree properties.
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	int i;
+
+	for (i = 0; i < vreg->drms_mode_count - 1; i++)
+		if (vreg->drms_mode_max_uAs[i] >= load_uA)
+			break;
+	if (load_uA > vreg->drms_mode_max_uAs[vreg->drms_mode_count - 1])
+		dev_warn(vreg->dev, "%s: requested load=%d uA greater than max=%d uA\n",
+			vreg->rdesc.name, load_uA,
+			vreg->drms_mode_max_uAs[vreg->drms_mode_count - 1]);
+
+	return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_modes[i]);
+}
+
+static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
+				bool enable)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	int ret;
+
+	if (vreg->bypassed == enable)
+		return 0;
+
+	ret = rpmh_regulator_vrm_set_mode_bypass(vreg, vreg->mode, enable);
+	if (!ret)
+		vreg->bypassed = enable;
+
+	return ret;
+}
+
+static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
+				bool *enable)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+	*enable = vreg->bypassed;
+
+	return 0;
+}
+
+static const struct regulator_ops rpmh_regulator_vrm_ops = {
+	.enable			= rpmh_regulator_enable,
+	.disable		= rpmh_regulator_disable,
+	.is_enabled		= rpmh_regulator_is_enabled,
+	.set_voltage_sel	= rpmh_regulator_vrm_set_voltage_sel,
+	.get_voltage_sel	= rpmh_regulator_vrm_get_voltage_sel,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.set_mode		= rpmh_regulator_vrm_set_mode,
+	.get_mode		= rpmh_regulator_vrm_get_mode,
+	.set_load		= rpmh_regulator_vrm_set_load,
+};
+
+static const struct regulator_ops rpmh_regulator_vrm_bypass_ops = {
+	.enable			= rpmh_regulator_enable,
+	.disable		= rpmh_regulator_disable,
+	.is_enabled		= rpmh_regulator_is_enabled,
+	.set_voltage_sel	= rpmh_regulator_vrm_set_voltage_sel,
+	.get_voltage_sel	= rpmh_regulator_vrm_get_voltage_sel,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.set_mode		= rpmh_regulator_vrm_set_mode,
+	.get_mode		= rpmh_regulator_vrm_get_mode,
+	.set_load		= rpmh_regulator_vrm_set_load,
+	.set_bypass		= rpmh_regulator_vrm_set_bypass,
+	.get_bypass		= rpmh_regulator_vrm_get_bypass,
+};
+
+static const struct regulator_ops rpmh_regulator_xob_ops = {
+	.enable			= rpmh_regulator_enable,
+	.disable		= rpmh_regulator_disable,
+	.is_enabled		= rpmh_regulator_is_enabled,
+};
+
+/**
+ * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
+ *		for a VRM RPMh resource from device tree
+ * vreg:		Pointer to the individual rpmh-regulator resource
+ * dev:			Pointer to the top level rpmh-regulator PMIC device
+ * node:		Pointer to the individual rpmh-regulator resource
+ *			device node
+ *
+ * This function initializes the drms_modes[] and drms_mode_max_uAs[] arrays of
+ * vreg based upon the values of optional device tree properties.
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
+				struct device *dev, struct device_node *node)
+{
+	const char *prop;
+	int i, len, ret, mode;
+	u32 *buf;
+
+	/* qcom,allowed-drms-modes is optional */
+	prop = "qcom,allowed-drms-modes";
+	len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+	if (len < 0)
+		return 0;
+
+	vreg->drms_modes = devm_kcalloc(dev, len, sizeof(*vreg->drms_modes),
+					GFP_KERNEL);
+	vreg->drms_mode_max_uAs = devm_kcalloc(dev, len,
+					       sizeof(*vreg->drms_mode_max_uAs),
+					       GFP_KERNEL);
+	if (!vreg->drms_modes || !vreg->drms_mode_max_uAs)
+		return -ENOMEM;
+	vreg->drms_mode_count = len;
+
+	buf = kcalloc(len, sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(node, prop, buf, len);
+	if (ret < 0) {
+		dev_err(dev, "%s: unable to read %s, ret=%d\n",
+			node->name, prop, ret);
+		goto done;
+	}
+
+	for (i = 0; i < len; i++) {
+		mode = vreg->hw_data->of_map_mode(buf[i]);
+		if (mode == REGULATOR_MODE_INVALID) {
+			dev_err(dev, "%s: element %d of %s = %u is invalid for this regulator\n",
+				node->name, i, prop, buf[i]);
+			ret = -EINVAL;
+			goto done;
+		}
+
+		vreg->drms_modes[i] = mode;
+	}
+
+	prop = "qcom,drms-mode-max-microamps";
+	len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+	if (len != vreg->drms_mode_count) {
+		dev_err(dev, "%s: invalid element count=%d for %s\n",
+			node->name, len, prop);
+		ret = -EINVAL;
+		goto done;
+	}
+
+	ret = of_property_read_u32_array(node, prop, buf, len);
+	if (ret < 0) {
+		dev_err(dev, "%s: unable to read %s, ret=%d\n",
+			node->name, prop, ret);
+		goto done;
+	}
+
+	for (i = 0; i < len; i++) {
+		vreg->drms_mode_max_uAs[i] = buf[i];
+
+		if (i > 0 && vreg->drms_mode_max_uAs[i]
+				<= vreg->drms_mode_max_uAs[i - 1]) {
+			dev_err(dev, "%s: %s elements are not in ascending order\n",
+				node->name, prop);
+			ret = -EINVAL;
+			goto done;
+		}
+	}
+
+done:
+	kfree(buf);
+	return ret;
+}
+
+/**
+ * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
+ *		request for this regulator based on optional device tree
+ *		properties
+ * vreg:		Pointer to the individual rpmh-regulator resource
+ * dev:			Pointer to the top level rpmh-regulator PMIC device
+ * node:		Pointer to the individual rpmh-regulator resource
+ *			device node
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg,
+				struct device *dev, struct device_node *node)
+{
+	struct tcs_cmd cmd[2] = {};
+	const struct regulator_linear_range *range;
+	const char *prop;
+	int cmd_count = 0;
+	int ret, selector;
+	u32 uV;
+
+	if (vreg->hw_data->regulator_type == VRM) {
+		/*
+		 * This will be set when devm_regulator_register() is called
+		 * if regulator-initial-mode is specified in device tree.
+		 */
+		vreg->mode = REGULATOR_MODE_INVALID;
+
+		prop = "qcom,headroom-microvolt";
+		ret = of_property_read_u32(node, prop, &uV);
+		if (!ret) {
+			if (uV > RPMH_VRM_HEADROOM_MAX_UV) {
+				dev_err(dev, "%s: %s=%u is invalid\n",
+					node->name, prop, uV);
+				return -EINVAL;
+			}
+
+			cmd[cmd_count].addr
+				= vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
+			cmd[cmd_count++].data = DIV_ROUND_UP(uV, 1000);
+		}
+
+		prop = "qcom,regulator-initial-microvolt";
+		ret = of_property_read_u32(node, prop, &uV);
+		if (!ret) {
+			range = &vreg->hw_data->voltage_range;
+			selector = DIV_ROUND_UP(uV - range->min_uV,
+					range->uV_step) + range->min_sel;
+			if (uV < range->min_uV || selector > range->max_sel) {
+				dev_err(dev, "%s: %s=%u is invalid\n",
+					node->name, prop, uV);
+				return -EINVAL;
+			}
+
+			vreg->voltage_selector = selector;
+
+			cmd[cmd_count].addr
+				= vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
+			cmd[cmd_count++].data
+				= DIV_ROUND_UP(selector * range->uV_step
+						+ range->min_uV, 1000);
+		} else {
+			/*
+			 * Default the voltage selector to an error value in the
+			 * case that qcom,regulator-initial-microvolt is not
+			 * specified in device tree since the true voltage is
+			 * not known.  Note that this value causes
+			 * devm_regulator_register() to fail in the case that
+			 * regulator-min-microvolt and regulator-max-microvolt
+			 * are specified in device tree due to
+			 * machine_constraints_voltage() bailing when the
+			 * get_voltage_sel() callback returns this error value.
+			 */
+			vreg->voltage_selector = -EINVAL;
+		}
+	}
+
+	if (cmd_count) {
+		ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true);
+		if (ret < 0) {
+			dev_err(dev, "%s: could not send default config, ret=%d\n",
+				node->name, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * rpmh_regulator_init_vreg() - initialize all attributes of an rpmh-regulator
+ * vreg:		Pointer to the individual rpmh-regulator resource
+ * dev:			Pointer to the top level rpmh-regulator PMIC device
+ * node:		Pointer to the individual rpmh-regulator resource
+ *			device node
+ * pmic_id:		String used to identify the top level rpmh-regulator
+ *			PMIC device on the board
+ * rpmh_data:		Pointer to a null-terminated array of rpmh-regulator
+ *			resources defined for the top level PMIC device
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
+				struct device_node *node, const char *pmic_id,
+				const struct rpmh_vreg_init_data *rpmh_data)
+{
+	struct regulator_config reg_config = {};
+	char rpmh_resource_name[20] = "";
+	struct regulator_dev *rdev;
+	enum rpmh_regulator_type type;
+	struct regulator_init_data *init_data;
+	int ret;
+
+	vreg->dev = dev;
+
+	for (; rpmh_data->name; rpmh_data++)
+		if (!strcmp(rpmh_data->name, node->name))
+			break;
+
+	if (!rpmh_data->name) {
+		dev_err(dev, "Unknown regulator %s\n", node->name);
+		return -EINVAL;
+	}
+
+	scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
+		rpmh_data->resource_name, pmic_id);
+
+	vreg->addr = cmd_db_read_addr(rpmh_resource_name);
+	if (!vreg->addr) {
+		dev_err(dev, "%s: could not find RPMh address for resource %s\n",
+			node->name, rpmh_resource_name);
+		return -ENODEV;
+	}
+
+	vreg->rdesc.name = rpmh_data->name;
+	vreg->rdesc.supply_name = rpmh_data->supply_name;
+	vreg->hw_data = rpmh_data->hw_data;
+
+	if (rpmh_data->hw_data->n_voltages) {
+		vreg->rdesc.linear_ranges = &rpmh_data->hw_data->voltage_range;
+		vreg->rdesc.n_linear_ranges = 1;
+		vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages;
+	}
+
+	type = rpmh_data->hw_data->regulator_type;
+	if (type == VRM) {
+		ret = rpmh_regulator_parse_vrm_modes(vreg, dev, node);
+		if (ret < 0)
+			return ret;
+	}
+
+	vreg->always_wait_for_ack = of_property_read_bool(node,
+						"qcom,always-wait-for-ack");
+
+	vreg->rdesc.owner	= THIS_MODULE;
+	vreg->rdesc.type	= REGULATOR_VOLTAGE;
+	vreg->rdesc.ops		= vreg->hw_data->ops;
+	vreg->rdesc.of_map_mode	= vreg->hw_data->of_map_mode;
+
+	init_data = of_get_regulator_init_data(dev, node, &vreg->rdesc);
+	if (!init_data)
+		return -ENOMEM;
+
+	if (type == XOB && init_data->constraints.min_uV &&
+	    init_data->constraints.min_uV == init_data->constraints.max_uV) {
+		vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
+		vreg->rdesc.n_voltages = 1;
+	}
+
+	reg_config.dev		= dev;
+	reg_config.init_data	= init_data;
+	reg_config.of_node	= node;
+	reg_config.driver_data	= vreg;
+
+	ret = rpmh_regulator_load_default_parameters(vreg, dev, node);
+	if (ret < 0)
+		return ret;
+
+	rdev = devm_regulator_register(dev, &vreg->rdesc, &reg_config);
+	if (IS_ERR(rdev)) {
+		ret = PTR_ERR(rdev);
+		rdev = NULL;
+		dev_err(dev, "%s: devm_regulator_register() failed, ret=%d\n",
+			node->name, ret);
+		return ret;
+	}
+
+	dev_dbg(dev, "%s regulator registered for RPMh resource %s @ 0x%05X\n",
+		node->name, rpmh_resource_name, vreg->addr);
+
+	return ret;
+}
+
+static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
+	[REGULATOR_MODE_INVALID] = -EINVAL,
+	[REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
+	[REGULATOR_MODE_IDLE]    = PMIC4_LDO_MODE_LPM,
+	[REGULATOR_MODE_NORMAL]  = -EINVAL,
+	[REGULATOR_MODE_FAST]    = PMIC4_LDO_MODE_HPM,
+};
+
+static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode)
+{
+	static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+		[RPMH_REGULATOR_MODE_RET]  = REGULATOR_MODE_STANDBY,
+		[RPMH_REGULATOR_MODE_LPM]  = REGULATOR_MODE_IDLE,
+		[RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_INVALID,
+		[RPMH_REGULATOR_MODE_HPM]  = REGULATOR_MODE_FAST,
+	};
+
+	if (mode >= RPMH_REGULATOR_MODE_COUNT)
+		return -EINVAL;
+
+	return of_mode_map[mode];
+}
+
+static const int pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
+	[REGULATOR_MODE_INVALID] = -EINVAL,
+	[REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
+	[REGULATOR_MODE_IDLE]    = PMIC4_SMPS_MODE_PFM,
+	[REGULATOR_MODE_NORMAL]  = PMIC4_SMPS_MODE_AUTO,
+	[REGULATOR_MODE_FAST]    = PMIC4_SMPS_MODE_PWM,
+};
+
+static unsigned int rpmh_regulator_pmic4_smps_of_map_mode(unsigned int mode)
+{
+	static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+		[RPMH_REGULATOR_MODE_RET]  = REGULATOR_MODE_STANDBY,
+		[RPMH_REGULATOR_MODE_LPM]  = REGULATOR_MODE_IDLE,
+		[RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
+		[RPMH_REGULATOR_MODE_HPM]  = REGULATOR_MODE_FAST,
+	};
+
+	if (mode >= RPMH_REGULATOR_MODE_COUNT)
+		return -EINVAL;
+
+	return of_mode_map[mode];
+}
+
+static const int pmic_mode_map_pmic4_bob[REGULATOR_MODE_STANDBY + 1] = {
+	[REGULATOR_MODE_INVALID] = -EINVAL,
+	[REGULATOR_MODE_STANDBY] = -EINVAL,
+	[REGULATOR_MODE_IDLE]    = PMIC4_BOB_MODE_PFM,
+	[REGULATOR_MODE_NORMAL]  = PMIC4_BOB_MODE_AUTO,
+	[REGULATOR_MODE_FAST]    = PMIC4_BOB_MODE_PWM,
+};
+
+static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
+{
+	static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+		[RPMH_REGULATOR_MODE_RET]  = REGULATOR_MODE_INVALID,
+		[RPMH_REGULATOR_MODE_LPM]  = REGULATOR_MODE_IDLE,
+		[RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
+		[RPMH_REGULATOR_MODE_HPM]  = REGULATOR_MODE_FAST,
+	};
+
+	if (mode >= RPMH_REGULATOR_MODE_COUNT)
+		return -EINVAL;
+
+	return of_mode_map[mode];
+}
+
+static const struct rpmh_vreg_hw_data pmic4_pldo = {
+	.regulator_type = VRM,
+	.ops = &rpmh_regulator_vrm_ops,
+	.voltage_range = REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),
+	.n_voltages = 256,
+	.pmic_mode_map = pmic_mode_map_pmic4_ldo,
+	.of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_pldo_lv = {
+	.regulator_type = VRM,
+	.ops = &rpmh_regulator_vrm_ops,
+	.voltage_range = REGULATOR_LINEAR_RANGE(1256000, 0, 127, 8000),
+	.n_voltages = 128,
+	.pmic_mode_map = pmic_mode_map_pmic4_ldo,
+	.of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_nldo = {
+	.regulator_type = VRM,
+	.ops = &rpmh_regulator_vrm_ops,
+	.voltage_range = REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000),
+	.n_voltages = 128,
+	.pmic_mode_map = pmic_mode_map_pmic4_ldo,
+	.of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_hfsmps3 = {
+	.regulator_type = VRM,
+	.ops = &rpmh_regulator_vrm_ops,
+	.voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
+	.n_voltages = 216,
+	.pmic_mode_map = pmic_mode_map_pmic4_smps,
+	.of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_ftsmps426 = {
+	.regulator_type = VRM,
+	.ops = &rpmh_regulator_vrm_ops,
+	.voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 258, 4000),
+	.n_voltages = 259,
+	.pmic_mode_map = pmic_mode_map_pmic4_smps,
+	.of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_bob = {
+	.regulator_type = VRM,
+	.ops = &rpmh_regulator_vrm_bypass_ops,
+	.voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
+	.n_voltages = 84,
+	.pmic_mode_map = pmic_mode_map_pmic4_bob,
+	.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_lvs = {
+	.regulator_type = XOB,
+	.ops = &rpmh_regulator_xob_ops,
+	/* LVS hardware does not support voltage or mode configuration. */
+};
+
+#define RPMH_VREG(_name, _resource_name, _hw_data, _supply_name) \
+{ \
+	.name		= _name, \
+	.resource_name	= _resource_name, \
+	.hw_data	= _hw_data, \
+	.supply_name	= _supply_name, \
+}
+
+static const struct rpmh_vreg_init_data pm8998_vreg_data[] = {
+	RPMH_VREG("smps1",  "smp%s1",  &pmic4_ftsmps426, "vdd-s1"),
+	RPMH_VREG("smps2",  "smp%s2",  &pmic4_ftsmps426, "vdd-s2"),
+	RPMH_VREG("smps3",  "smp%s3",  &pmic4_hfsmps3,   "vdd-s3"),
+	RPMH_VREG("smps4",  "smp%s4",  &pmic4_hfsmps3,   "vdd-s4"),
+	RPMH_VREG("smps5",  "smp%s5",  &pmic4_hfsmps3,   "vdd-s5"),
+	RPMH_VREG("smps6",  "smp%s6",  &pmic4_ftsmps426, "vdd-s6"),
+	RPMH_VREG("smps7",  "smp%s7",  &pmic4_ftsmps426, "vdd-s7"),
+	RPMH_VREG("smps8",  "smp%s8",  &pmic4_ftsmps426, "vdd-s8"),
+	RPMH_VREG("smps9",  "smp%s9",  &pmic4_ftsmps426, "vdd-s9"),
+	RPMH_VREG("smps10", "smp%s10", &pmic4_ftsmps426, "vdd-s10"),
+	RPMH_VREG("smps11", "smp%s11", &pmic4_ftsmps426, "vdd-s11"),
+	RPMH_VREG("smps12", "smp%s12", &pmic4_ftsmps426, "vdd-s12"),
+	RPMH_VREG("smps13", "smp%s13", &pmic4_ftsmps426, "vdd-s13"),
+	RPMH_VREG("ldo1",   "ldo%s1",  &pmic4_nldo,      "vdd-l1-l27"),
+	RPMH_VREG("ldo2",   "ldo%s2",  &pmic4_nldo,      "vdd-l2-l8-l17"),
+	RPMH_VREG("ldo3",   "ldo%s3",  &pmic4_nldo,      "vdd-l3-l11"),
+	RPMH_VREG("ldo4",   "ldo%s4",  &pmic4_nldo,      "vdd-l4-l5"),
+	RPMH_VREG("ldo5",   "ldo%s5",  &pmic4_nldo,      "vdd-l4-l5"),
+	RPMH_VREG("ldo6",   "ldo%s6",  &pmic4_pldo,      "vdd-l6"),
+	RPMH_VREG("ldo7",   "ldo%s7",  &pmic4_pldo_lv,   "vdd-l7-l12-l14-l15"),
+	RPMH_VREG("ldo8",   "ldo%s8",  &pmic4_nldo,      "vdd-l2-l8-l17"),
+	RPMH_VREG("ldo9",   "ldo%s9",  &pmic4_pldo,      "vdd-l9"),
+	RPMH_VREG("ldo10",  "ldo%s10", &pmic4_pldo,      "vdd-l10-l23-l25"),
+	RPMH_VREG("ldo11",  "ldo%s11", &pmic4_nldo,      "vdd-l3-l11"),
+	RPMH_VREG("ldo12",  "ldo%s12", &pmic4_pldo_lv,   "vdd-l7-l12-l14-l15"),
+	RPMH_VREG("ldo13",  "ldo%s13", &pmic4_pldo,      "vdd-l13-l19-l21"),
+	RPMH_VREG("ldo14",  "ldo%s14", &pmic4_pldo_lv,   "vdd-l7-l12-l14-l15"),
+	RPMH_VREG("ldo15",  "ldo%s15", &pmic4_pldo_lv,   "vdd-l7-l12-l14-l15"),
+	RPMH_VREG("ldo16",  "ldo%s16", &pmic4_pldo,      "vdd-l16-l28"),
+	RPMH_VREG("ldo17",  "ldo%s17", &pmic4_nldo,      "vdd-l2-l8-l17"),
+	RPMH_VREG("ldo18",  "ldo%s18", &pmic4_pldo,      "vdd-l18-l22"),
+	RPMH_VREG("ldo19",  "ldo%s19", &pmic4_pldo,      "vdd-l13-l19-l21"),
+	RPMH_VREG("ldo20",  "ldo%s20", &pmic4_pldo,      "vdd-l20-l24"),
+	RPMH_VREG("ldo21",  "ldo%s21", &pmic4_pldo,      "vdd-l13-l19-l21"),
+	RPMH_VREG("ldo22",  "ldo%s22", &pmic4_pldo,      "vdd-l18-l22"),
+	RPMH_VREG("ldo23",  "ldo%s23", &pmic4_pldo,      "vdd-l10-l23-l25"),
+	RPMH_VREG("ldo24",  "ldo%s24", &pmic4_pldo,      "vdd-l20-l24"),
+	RPMH_VREG("ldo25",  "ldo%s25", &pmic4_pldo,      "vdd-l10-l23-l25"),
+	RPMH_VREG("ldo26",  "ldo%s26", &pmic4_nldo,      "vdd-l26"),
+	RPMH_VREG("ldo27",  "ldo%s27", &pmic4_nldo,      "vdd-l1-l27"),
+	RPMH_VREG("ldo28",  "ldo%s28", &pmic4_pldo,      "vdd-l16-l28"),
+	RPMH_VREG("lvs1",   "vs%s1",   &pmic4_lvs,       "vin-lvs-1-2"),
+	RPMH_VREG("lvs2",   "vs%s2",   &pmic4_lvs,       "vin-lvs-1-2"),
+	{},
+};
+
+static const struct rpmh_vreg_init_data pmi8998_vreg_data[] = {
+	RPMH_VREG("bob",    "bob%s1",  &pmic4_bob,       "vdd-bob"),
+	{},
+};
+
+static const struct rpmh_vreg_init_data pm8005_vreg_data[] = {
+	RPMH_VREG("smps1",  "smp%s1",  &pmic4_ftsmps426, "vdd-s1"),
+	RPMH_VREG("smps2",  "smp%s2",  &pmic4_ftsmps426, "vdd-s2"),
+	RPMH_VREG("smps3",  "smp%s3",  &pmic4_ftsmps426, "vdd-s3"),
+	RPMH_VREG("smps4",  "smp%s4",  &pmic4_ftsmps426, "vdd-s4"),
+	{},
+};
+
+static int rpmh_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct rpmh_vreg_init_data *vreg_data;
+	struct device_node *node;
+	struct rpmh_vreg *vreg;
+	const char *pmic_id;
+	int ret;
+
+	ret = cmd_db_ready();
+	if (ret < 0) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Command DB not available, ret=%d\n", ret);
+		return ret;
+	}
+
+	vreg_data = of_device_get_match_data(dev);
+	if (!vreg_data)
+		return -ENODEV;
+
+	ret = of_property_read_string(dev->of_node, "qcom,pmic-id", &pmic_id);
+	if (ret < 0) {
+		dev_err(dev, "qcom,pmic-id missing in DT node\n");
+		return ret;
+	}
+
+	for_each_available_child_of_node(dev->of_node, node) {
+		vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
+		if (!vreg) {
+			of_node_put(node);
+			return -ENOMEM;
+		}
+
+		ret = rpmh_regulator_init_vreg(vreg, dev, node, pmic_id,
+						vreg_data);
+		if (ret < 0) {
+			of_node_put(node);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id rpmh_regulator_match_table[] = {
+	{
+		.compatible = "qcom,pm8998-rpmh-regulators",
+		.data = pm8998_vreg_data,
+	},
+	{
+		.compatible = "qcom,pmi8998-rpmh-regulators",
+		.data = pmi8998_vreg_data,
+	},
+	{
+		.compatible = "qcom,pm8005-rpmh-regulators",
+		.data = pm8005_vreg_data,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table);
+
+static struct platform_driver rpmh_regulator_driver = {
+	.driver = {
+		.name = "qcom-rpmh-regulator",
+		.of_match_table	= of_match_ptr(rpmh_regulator_match_table),
+	},
+	.probe = rpmh_regulator_probe,
+};
+module_platform_driver(rpmh_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] regulator: add QCOM RPMh regulator driver

[Doug Anderson]

Hi,

On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
> +                               struct device *dev, struct device_node *node)
> +{
> +       const char *prop;
> +       int i, len, ret, mode;
> +       u32 *buf;
> +
> +       /* qcom,allowed-drms-modes is optional */
> +       prop = "qcom,allowed-drms-modes";

As per comments in bindings patch: this is a duplicate of your new
attribute you added to the regulator core.  Makes no sense to have a
private attribute too with the same value.


> +       prop = "qcom,drms-mode-max-microamps";

As per comments in the bindings patch, I think we should move
"qcom,drms-mode-max-microamps" to the regulator core.


> +               prop = "qcom,regulator-initial-microvolt";

As per comments in bindings patch: seems like we should get rid of
"qcom,regulator-initial-microvolt" or move to the core.


> +                       /*
> +                        * Default the voltage selector to an error value in the
> +                        * case that qcom,regulator-initial-microvolt is not
> +                        * specified in device tree since the true voltage is
> +                        * not known.  Note that this value causes
> +                        * devm_regulator_register() to fail in the case that
> +                        * regulator-min-microvolt and regulator-max-microvolt
> +                        * are specified in device tree due to
> +                        * machine_constraints_voltage() bailing when the
> +                        * get_voltage_sel() callback returns this error value.
> +                        */
> +                       vreg->voltage_selector = -EINVAL;

As per comments in other threads, adjust this comment and use
-ENOTRECOVERABLE now.


NOTE: I think this driver is looking really good now.  Hopefully the
above things should be quick to spin (even getting "max-microamps" in
the core should be quick I think)  and we can get something landed!
:)

-Doug

Re: [PATCH v3 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

On 05/17/2018 02:23 PM, Doug Anderson wrote:
> On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
>> +                       /*
>> +                        * Default the voltage selector to an error value in the
>> +                        * case that qcom,regulator-initial-microvolt is not
>> +                        * specified in device tree since the true voltage is
>> +                        * not known.  Note that this value causes
>> +                        * devm_regulator_register() to fail in the case that
>> +                        * regulator-min-microvolt and regulator-max-microvolt
>> +                        * are specified in device tree due to
>> +                        * machine_constraints_voltage() bailing when the
>> +                        * get_voltage_sel() callback returns this error value.
>> +                        */
>> +                       vreg->voltage_selector = -EINVAL;
> 
> As per comments in other threads, adjust this comment and use
> -ENOTRECOVERABLE now.

I'll make this change.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project