regulator: add QCOM RPMh regulator driver

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 97

Quoted 5 lines · David Collins

+- qcom,regulator-initial-microvolt

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +4 -0

+       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?

Quoted 4 lines

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.

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 105

Quoted 7 lines

+- regulator-initial-mode

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +6 -0

+       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.

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 114

Quoted 8 lines

+- qcom,allowed-drms-modes

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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...

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 123

Quoted 8 lines

+- qcom,drms-mode-max-microamps

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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.

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 132

Quoted 8 lines

+- qcom,headroom-microvolt

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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.

Quoted 4 lines

include/dt-bindings/regulator/qcom,rpmh-regulator.h new file +0 -1

new file mode 100644
index 0000000..4378c4b
--- /dev/null

Quoted 4 lines

+++ 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?

Quoted 10 lines · Doug Anderson

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 97

Quoted 5 lines · David Collins

+- qcom,regulator-initial-microvolt

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +4 -0

+       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.

Quoted 1 line

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]:

Quoted 8 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 105

Quoted 7 lines

+- regulator-initial-mode

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +6 -0

+       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.

Quoted 6 lines

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

Quoted 4 lines

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

(quoted diff) +4 -0

+       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.

Quoted 9 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 114

Quoted 8 lines

+- qcom,allowed-drms-modes

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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).

Quoted 4 lines

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.

Quoted 9 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 123

Quoted 8 lines

+- qcom,drms-mode-max-microamps

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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.

Quoted 3 lines

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.

Quoted 22 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 132

Quoted 8 lines

+- qcom,headroom-microvolt

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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.

Quoted 5 lines

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).

Quoted 12 lines · David Collins

Quoted 10 lines · Doug Anderson

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 97

Quoted 5 lines · David Collins

+- qcom,regulator-initial-microvolt

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +4 -0

+       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?

Quoted 7 lines

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?

Quoted 15 lines

Quoted 8 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 105

Quoted 7 lines

+- regulator-initial-mode

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +6 -0

+       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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 14 lines

Quoted 4 lines

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

(quoted diff) +4 -0

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

Quoted 2 lines

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.

Quoted 19 lines

Quoted 9 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 114

Quoted 8 lines

+- qcom,allowed-drms-modes

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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).

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 13 lines

Quoted 9 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 123

Quoted 8 lines

+- qcom,drms-mode-max-microamps

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 28 lines

Quoted 22 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 132

Quoted 8 lines

+- qcom,headroom-microvolt

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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.

Quoted 5 lines

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.

Quoted 6 lines

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?

Quoted 21 lines · Doug Anderson

Quoted 12 lines · David Collins

Quoted 10 lines · Doug Anderson

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 97

Quoted 5 lines · David Collins

+- qcom,regulator-initial-microvolt

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +4 -0

+       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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 27 lines

Quoted 19 lines

Quoted 9 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 114

Quoted 8 lines

+- qcom,allowed-drms-modes

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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).

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

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.

Quoted 19 lines

Quoted 13 lines

Quoted 9 lines

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 123

Quoted 8 lines

+- qcom,drms-mode-max-microamps

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +7 -0

+       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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

On Fri, May 18, 2018 at 5:46 PM, David Collins <[email protected]> wrote:

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 1 line · Doug Anderson

On Fri, May 18, 2018 at 5:46 PM, David Collins <[email protected]> wrote:

...

Quoted 13 lines

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 6 lines · David Collins

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.

Quoted 8 lines

Quoted 1 line

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

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.

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).

On Mon, May 21, 2018 at 5:00 PM, David Collins <[email protected]> wrote:

Quoted 6 lines

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 3 lines

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).

Quoted 1 line · Doug Anderson

On Mon, May 21, 2018 at 5:00 PM, David Collins <[email protected]> wrote:

...

Quoted 7 lines

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 14 lines

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.

Quoted 6 lines

...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.

Quoted 12 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

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

Quoted 2 lines

...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().

Quoted 7 lines

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.

Quoted 2 lines · David Collins

Quoted 1 line · Doug Anderson

On Mon, May 21, 2018 at 5:00 PM, David Collins <[email protected]> wrote:

Quoted 4 lines

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.

Quoted 36 lines

Quoted 14 lines

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.

Quoted 6 lines

...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?

Quoted 5 lines

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.

Quoted 12 lines

Quoted 12 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 12 lines

Quoted 11 lines

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.

Quoted 3 lines

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.

Quoted 49 lines

Quoted 36 lines

Quoted 14 lines

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.

Quoted 6 lines

...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?

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines

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.

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

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'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.

Hi,

Quoted 6 lines · Mark Brown

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...

Quoted 11 lines · Doug Anderson

Hi,

Quoted 6 lines · Mark Brown

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.

Quoted 8 lines

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.

Quoted 5 lines · Mark Brown

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.

Quoted 8 lines · Doug Anderson

Quoted 5 lines · Mark Brown

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.

Quoted 8 lines

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.

Quoted 3 lines · Mark Brown

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):

Quoted 6 lines · Doug Anderson

Quoted 3 lines · Mark Brown

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.

Quoted 6 lines

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.

Quoted 4 lines · Mark Brown

Quoted 4 lines

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):

Quoted 2 lines

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?

Quoted 9 lines · Doug Anderson

Quoted 7 lines · Mark Brown

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines · Mark Brown

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

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.

Quoted 8 lines · Doug Anderson

Quoted 2 lines · Mark Brown

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.

Quoted 3 lines

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...

Quoted 2 lines · Mark Brown

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.

Quoted 5 lines · Doug Anderson

Quoted 2 lines · Mark Brown

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

Quoted 7 lines

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.

Quoted 1 line · Mark Brown

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?

Quoted 3 lines · Doug Anderson

Quoted 1 line · Mark Brown

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

Quoted 2 lines

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.

Quoted 4 lines · Mark Brown

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.

Quoted 7 lines · Doug Anderson

Quoted 4 lines · Mark Brown

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.

Quoted 8 lines

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.

Quoted 3 lines · Mark Brown

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...

Quoted 10 lines · Doug Anderson

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 97

Quoted 5 lines · David Collins

+- qcom,regulator-initial-microvolt

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +4 -0

+       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.

Quoted 1 line

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]:

Quoted 17 lines

Quoted 4 lines

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.

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt line 105

Quoted 7 lines

+- regulator-initial-mode

Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +6 -0

+       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.

Quoted 6 lines

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

Quoted 4 lines

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

(quoted diff) +4 -0

+	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.

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

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

drivers/regulator/qcom-rpmh-regulator.c line 537

Quoted 12 lines · David Collins

drivers/regulator/qcom-rpmh-regulator.c new file +12 -0

+                       /*
+                        * 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.