| Message ID | 41d5df73ddac772551d2966e0752bb0c357b1ded.1526088081.git.collinsd@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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