regulator: add QCOM RPMh regulator driver
Subsystems (3)
v8 cover regulator: add QCOM RPMh regulator driver
This patch series adds a driver and device tree binding documentation for PMIC regulator control via Resource Power Manager-hardened (RPMh) on some Qualcomm Technologies, Inc. SoCs such as SDM845. RPMh is a hardware block which contains several accelerators which are used to manage various hardware resources that are shared between the processors of the SoC. The final hardware state of a regulator is determined within RPMh by performing max aggregation of the requests made by all of the processors.
The RPMh regulator driver depends upon the RPMh driver [1] and command DB driver [2] which are both still undergoing review. It also depends upon three recent regulator patches: [3], [4], and [5] as well as an RPMh helper patch [6].
Changes since v7 [7]:
- Removed the cmd_db_ready() call from the driver probe function as it is now handled by the RPMh driver itself [6]
- Added Reviewed-by tag for driver patch 2/2
Changes since v6 [8]:
- Removed 'count' parameter from rpmh_regulator_send_request() since it is always 1
- Fixed _rpmh_regulator_vrm_set_voltage_sel() return value
- Added a helper function to capture common code between rpmh_regulator_enable() and rpmh_regulator_disable()
- Added an iterator for pmic_rpmh_data in rpmh_regulator_init_vreg()
- Added Reviewed-by tag for both patches
Changes since v5 [9]:
- Removed unused constants
- Added Reviewed-by tag for DT patch 1/2
Changes since v4 [10]:
- Removed support for DT properties qcom,regulator-drms-modes and qcom,drms-mode-max-microamps
- Specified fixed DRMS high power mode minimum limits for LDO type regulators
- Removed DRMS support for SMPS and BOB type regulators
- Simplified voltage caching logic
Changes since v3 [11]:
- Removed support for DT properties qcom,regulator-initial-microvolt and qcom,headroom-microvolt
- Renamed DT property qcom,allowed-drms-modes to be qcom,regulator-drms-modes
- Updated DT binding documentation to mention which common regulator bindings can be used for qcom-rpmh-regulator devices
- Added voltage caching so that voltage requests are only sent to RPMh after the regulator has been enabled at least once
- Changed 'voltage_selector' default value to be -ENOTRECOVERABLE to interact with [5]
- Initialized 'enabled' to -EINVAL so that unused regulators are disabled by regulator_late_cleanup()
- Removed rpmh_regulator_load_default_parameters() as it is no longer needed
- Updated the mode usage description in qcom,rpmh-regulator.h
Changes since v2 [12]:
- Replaced '_' with '-' in device tree supply property names
- Renamed qcom_rpmh-regulator.c to be qcom-rpmh-regulator.c
- Updated various DT property names to use "microvolt" and "microamp"
- Moved allowed modes constraint specification out of the driver [4]
- Replaced rpmh_client with device pointer to match new RPMh API [1]
- Corrected drms mode threshold checking
- Initialized voltage_selector to -EINVAL when not specified in DT
- Added constants for PMIC regulator hardware modes
- Corrected type sign of mode mapping tables
- Made variable names for mode arrays plural
- Simplified Kconfig depends on
- Removed unnecessary constants and struct fields
- Added some descriptive comments
Changes since v1 [13]:
- Addressed review feedback from Doug, Mark, and Stephen
- Replaced set_voltage()/get_voltage() callbacks with set_voltage_sel()/ get_voltage_sel()
- Added set_bypass()/get_bypass() callbacks for BOB pass-through mode control
- Removed top-level PMIC data structures
- Removed initialization variables from structs and passed them as function parameters
- Removed various comments and error messages
- Simplified mode handling
- Refactored per-PMIC rpmh-regulator data specification
- Simplified probe function
- Moved header into DT patch
- Removed redundant property listings from DT binding documentation
[1]: https://lkml.org/lkml/2018/6/20/519
[2]: https://lkml.org/lkml/2018/4/10/714
[3]: https://lkml.org/lkml/2018/4/18/556
[4]: https://lkml.org/lkml/2018/5/11/696
[5]: https://lkml.org/lkml/2018/5/15/1005
[6]: https://lkml.org/lkml/2018/6/18/1188
[7]: https://lkml.org/lkml/2018/6/8/573
[8]: https://lkml.org/lkml/2018/6/4/879
[9]: https://lkml.org/lkml/2018/6/1/895
[10]: https://lkml.org/lkml/2018/5/22/1168
[11]: https://lkml.org/lkml/2018/5/11/701
[12]: https://lkml.org/lkml/2018/4/13/687
[13]: https://lkml.org/lkml/2018/3/16/1431
David Collins (2):
regulator: dt-bindings: add QCOM RPMh regulator bindings
regulator: add QCOM RPMh regulator driver
.../bindings/regulator/qcom,rpmh-regulator.txt | 160 +++++ drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/qcom-rpmh-regulator.c | 746 +++++++++++++++++++++ .../dt-bindings/regulator/qcom,rpmh-regulator.h | 36 + 5 files changed, 952 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt create mode 100644 drivers/regulator/qcom-rpmh-regulator.c create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v8 1/2 regulator: dt-bindings: add QCOM RPMh regulator bindings
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.
--- .../bindings/regulator/qcom,rpmh-regulator.txt | 160 +++++++++++++++++++++ .../dt-bindings/regulator/qcom,rpmh-regulator.h | 36 +++++ 2 files changed, 196 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h
Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt new file +160 -0
@@ -0,0 +1,160 @@ +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 three +parameters for a given regulator: enable state, output voltage, and operating +mode. 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,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. regulator-initial-mode and regulator-allowed-modes may be +specified for VRM regulators using mode values from +include/dt-bindings/regulator/qcom,rpmh-regulator.h. regulator-allow-bypass +may be specified for BOB type regulators managed via VRM. +regulator-allow-set-load may be specified for LDO type regulators managed via +VRM. + +======== +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>; + }; + + pm8998_s5: smps5 { + regulator-min-microvolt = <1904000>; + regulator-max-microvolt = <2040000>; + }; + + ldo7 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; + regulator-allowed-modes = + <RPMH_REGULATOR_MODE_LPM + RPMH_REGULATOR_MODE_HPM>; + regulator-allow-set-load; + }; + + 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>; + regulator-allowed-modes = + <RPMH_REGULATOR_MODE_AUTO + RPMH_REGULATOR_MODE_HPM>; + regulator-initial-mode = <RPMH_REGULATOR_MODE_AUTO>; + }; + }; +};
include/dt-bindings/regulator/qcom,rpmh-regulator.h new file +36 -0
@@ -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 to specify modes for various RPMh regulator +* device tree properties (e.g. regulator-initial-mode). 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
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v8 2/2 regulator: add QCOM RPMh regulator driver
In reply to David CollinsAdd the QCOM RPMh regulator driver to manage PMIC regulators which are controlled via RPMh on some Qualcomm Technologies, Inc. SoCs. RPMh is a hardware block which contains several accelerators which are used to manage various hardware resources that are shared between the processors of the SoC. The final hardware state of a regulator is determined within RPMh by performing max aggregation of the requests made by all of the processors.
Add support for PMIC regulator control via the voltage regulator manager (VRM) and oscillator buffer (XOB) RPMh accelerators. VRM supports manipulation of enable state, voltage, and mode. XOB supports manipulation of enable state.
--- drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/qcom-rpmh-regulator.c | 746 ++++++++++++++++++++++++++++++++ 3 files changed, 756 insertions(+) create mode 100644 drivers/regulator/qcom-rpmh-regulator.c
drivers/regulator/Kconfig +9 -0
@@ -682,6 +682,15 @@ config REGULATOR_QCOM_RPM Qualcomm RPM as a module. The module will be named "qcom_rpm-regulator". +config REGULATOR_QCOM_RPMH + tristate "Qualcomm Technologies, Inc. RPMh regulator driver" + depends on QCOM_RPMH || COMPILE_TEST + help + This driver supports control of PMIC regulators via the RPMh hardware + block found on Qualcomm Technologies Inc. SoCs. RPMh regulator + control allows for voting on regulator state between multiple + processors within the SoC. + config REGULATOR_QCOM_SMD_RPM tristate "Qualcomm SMD based RPM regulator driver" depends on QCOM_SMD_RPM
drivers/regulator/Makefile +1 -0
@@ -78,6 +78,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o +obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
drivers/regulator/qcom-rpmh-regulator.c new file +746 -0
746 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi,
Quoted 14 lines · David Collins
Add the QCOM RPMh regulator driver to manage PMIC regulators which are controlled via RPMh on some Qualcomm Technologies, Inc. SoCs. RPMh is a hardware block which contains several accelerators which are used to manage various hardware resources that are shared between the processors of the SoC. The final hardware state of a regulator is determined within RPMh by performing max aggregation of the requests made by all of the processors.
Add support for PMIC regulator control via the voltage regulator manager (VRM) and oscillator buffer (XOB) RPMh accelerators. VRM supports manipulation of enable state, voltage, and mode. XOB supports manipulation of enable state.
Quoted 8 lines
Signed-off-by David Collins <[email protected]>Reviewed-by Douglas Anderson <[email protected]>Reviewed-by Matthias Kaehlcke <[email protected]>--- drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/qcom-rpmh-regulator.c | 746 ++++++++++++++++++++++++++++++++ 3 files changed, 756 insertions(+)
I've been working with David's v7 patch series for a bit of time now and things have been working well. v8 looks good. Feel free to add:
...to this patch. Speaking of which it might be getting very close to time for this series to land along with David's other series, AKA:
* [1/2] regulator: of: add property for allowed modes specification
https://patchwork.kernel.org/patch/10395731/
* [2/2] regulator: of: add support for allowed modes configuration
https://patchwork.kernel.org/patch/10395725/
Specifically I think we are mostly waiting for RPMh to land. This is on Andy's plate right now but I think it's happy and just waiting for the actual push from Andy.
NOTE: I don't know if there anything special we need to do for landing this and RPMh across two git repositories. Offline Stephen Boyd mentioned to me that maybe it was fine to land this in Mark's tree without the RPMh code since it depends on "QCOM_RPMH", but the "|| COMPILE_TEST" worries me and makes me think auto-builders will start yelling. Andy and Mark: what do you think?
If it's helpful to have a link to the latest RPMh, here are patchwork IDs on linux-arm-msm:
10477501 [v13,01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs 10477559 [v13,02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs 10477545 [v13,03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE 10477543 [v13,04/10] drivers: qcom: rpmh: add RPMH helper functions 10477537 [v13,05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS 10477539 [v13,06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS
10477533 [v13,07/10] drivers: qcom: rpmh: cache sleep/wake state requests 10477529 [v13,08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously
10477525 [v13,09/10] drivers: qcom: rpmh: add support for batch RPMH request 10477519 [v13,10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS
-Doug
Mark Brown
maintainerQuoted 8 lines · Doug Anderson
...to this patch. Speaking of which it might be getting very close to time for this series to land along with David's other series, AKA:
* [1/2] regulator: of: add property for allowed modes specification
https://patchwork.kernel.org/patch/10395731/* [2/2] regulator: of: add support for allowed modes configuration
https://patchwork.kernel.org/patch/10395725/
Please do not submit new versions of already applied patches, please
submit incremental updates to the existing code. Modifying existing
commits creates problems for other users building on top of those
commits so it's best practice to only change pubished git commits if
absolutely essential.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180626/756ecee7/attachment-0001.sig>
Hi,
Quoted 8 lines · Mark Brown
Quoted 5 lines · Doug Anderson
...to this patch. Speaking of which it might be getting very close to time for this series to land along with David's other series, AKA:
* [1/2] regulator: of: add property for allowed modes specification
https://patchwork.kernel.org/patch/10395731/
Quoted 8 lines
Quoted 2 lines
* [2/2] regulator: of: add support for allowed modes configuration
https://patchwork.kernel.org/patch/10395725/Please do not submit new versions of already applied patches, please submit incremental updates to the existing code. Modifying existing commits creates problems for other users building on top of those commits so it's best practice to only change pubished git commits if absolutely essential.
Sorry, wasn't suggesting making any changes to those two patches, just was noting the dependency. ...but, as you said, the two dependent patches have already landed and I just didn't notice. :( Sorry for the noise.
-Doug
Mark Brown
maintainerQuoted 8 lines · Doug Anderson
Quoted 5 lines · Mark Brown
Please do not submit new versions of already applied patches, please submit incremental updates to the existing code. Modifying existing commits creates problems for other users building on top of those commits so it's best practice to only change pubished git commits if absolutely essential.
Quoted 4 lines
Sorry, wasn't suggesting making any changes to those two patches, just was noting the dependency. ...but, as you said, the two dependent patches have already landed and I just didn't notice. :( Sorry for the noise.
Ah, so there's no revisions that need merging? That's great.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180626/19b698d6/attachment.sig>
Hi,
Quoted 9 lines · Mark Brown
Quoted 7 lines · Doug Anderson
Quoted 5 lines · Mark Brown
Please do not submit new versions of already applied patches, please submit incremental updates to the existing code. Modifying existing commits creates problems for other users building on top of those commits so it's best practice to only change pubished git commits if absolutely essential.
Quoted 6 lines
Quoted 4 lines
Sorry, wasn't suggesting making any changes to those two patches, just was noting the dependency. ...but, as you said, the two dependent patches have already landed and I just didn't notice. :( Sorry for the noise.
Ah, so there's no revisions that need merging? That's great.
Right. So all that's left to do is decide if ${SUBJECT} patch is ready to land and how to land it. Andy has landed RPMh into his for-next tree. You can see it at
<https://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git/log/?h=for-next>.
I think he's still planning on re-shuffling his tree a bit. When he does this, do you need him to put the RPMh patches somewhere you can merge into your tree?
Thanks!
-Doug
Mark Brown
maintainerQuoted 3 lines · Doug Anderson
I think he's still planning on re-shuffling his tree a bit. When he does this, do you need him to put the RPMh patches somewhere you can merge into your tree?
Well, I *think* there's no actual dependency here since it's a new
driver with a Kconfig dependency. It really just needs me to get round
to trawling through what's a fairly large patch with a troubled history
now you've reviewed it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180627/813bc040/attachment.sig>
Hi,
Quoted 10 lines · Mark Brown
Quoted 3 lines · Doug Anderson
I think he's still planning on re-shuffling his tree a bit. When he does this, do you need him to put the RPMh patches somewhere you can merge into your tree?
Well, I *think* there's no actual dependency here since it's a new driver with a
Kconfigdependency. It really just needs me to get round to trawling through what's a fairly large patch with a troubled history now you've reviewed it.
OK, great. I guess I'm confused about the "|| COMPILE_TEST" causing problems then? I was worried that anyone trying to do "COMPILE_TEST" on your tree (or linuxnext if RPMh isn't there) would get failures due to the lack of header files. I guess if it's a problem you could just gut the "|| COMPILE_TEST" and it could be added back in later?
Hoping my reviews saved you time overall.
-Doug
Mark Brown
maintainerQuoted 5 lines · Doug Anderson
OK, great. I guess I'm confused about the "||
COMPILE_TEST" causing problems then? I was worried that anyone trying to do "COMPILE_TEST" on your tree (or linuxnext if RPMh isn't there) would get failures due to the lack of header files. I guess if it's a problem you could just gut the "||COMPILE_TEST" and it could be added back in later?
Ugh, yes - that'll break things. In that case I can't apply this
without a signed tag from Andy's tree with the dependency stuff in.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180628/f43b7989/attachment.sig>
Hello Mark,
Quoted 11 lines · Mark Brown
Quoted 5 lines · Doug Anderson
OK, great. I guess I'm confused about the "||
COMPILE_TEST" causing problems then? I was worried that anyone trying to do "COMPILE_TEST" on your tree (or linuxnext if RPMh isn't there) would get failures due to the lack of header files. I guess if it's a problem you could just gut the "||COMPILE_TEST" and it could be added back in later?Ugh, yes - that'll break things. In that case I can't apply this without a signed tag from Andy's tree with the dependency stuff in.
Do you have any remaining concerns with the qcom-rpmh-regulator binding and driver patches that would keep you from applying them (other than the dependency patches being applied first)?
Thanks,
David
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Mark Brown
maintainerQuoted 3 lines · David Collins
Do you have any remaining concerns with the qcom-rpmh-regulator binding and driver patches that would keep you from applying them (other than the dependency patches being applied first)?
To repeat I need to find the time to sit down and go through them
properly, the problems this driver has had mean I feel I need to be
extra careful there.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180629/a1daa314/attachment-0001.sig>
Mark Brown
maintainerdrivers/regulator/qcom-rpmh-regulator.c line 3
Quoted 6 lines · David Collins
drivers/regulator/qcom-rpmh-regulator.c new file +3
-0@@ -0,0 +1,746 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ +
Please make the entire header block C++ so it looks intentional.
drivers/regulator/qcom-rpmh-regulator.c line 265
Quoted 1 line
drivers/regulator/qcom-rpmh-regulator.c new file +1
-0+ cmd.data = bypassed ?PMIC4_BOB_MODE_PASS : pmic_mode;
Please just write normal if statements, the ternery operator isn't really helping legibility.
drivers/regulator/qcom-rpmh-regulator.c line 488
Quoted 7 lines
drivers/regulator/qcom-rpmh-regulator.c new file +7
-0+static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY +1 ] = { + [REGULATOR_MODE_INVALID ] = -EINVAL , + [REGULATOR_MODE_STANDBY ] =PMIC4_LDO_MODE_RETENTION , + [REGULATOR_MODE_IDLE ] =PMIC4_LDO_MODE_LPM , + [REGULATOR_MODE_NORMAL ] = -EINVAL , + [REGULATOR_MODE_FAST ] =PMIC4_LDO_MODE_HPM , +};
This mapping is really weird, I'd expect one of the modes to correspond to the normal operating mode of the regulator.
drivers/regulator/qcom-rpmh-regulator.c line 497
Quoted 8 lines
drivers/regulator/qcom-rpmh-regulator.c new file +8
-0+static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode (unsigned int mode) +{ +static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT ] = { + [RPMH_REGULATOR_MODE_RET ] =REGULATOR_MODE_STANDBY , + [RPMH_REGULATOR_MODE_LPM ] =REGULATOR_MODE_IDLE , + [RPMH_REGULATOR_MODE_AUTO ] =REGULATOR_MODE_INVALID , + [RPMH_REGULATOR_MODE_HPM ] =REGULATOR_MODE_FAST , + };
Same here, based on that it looks like auto mode is a good map for normal.
drivers/regulator/qcom-rpmh-regulator.c line 21
Quoted 2 lines
drivers/regulator/qcom-rpmh-regulator.c new file +2
-0+if (mode >=RPMH_REGULATOR_MODE_COUNT ) +return -EINVAL ;
Why not use ARRAY_SIZE?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180702/2b53c489/attachment.sig>
Hello Mark,
Quoted 10 lines · Mark Brown
drivers/regulator/qcom-rpmh-regulator.c line 3
Quoted 6 lines · David Collins
drivers/regulator/qcom-rpmh-regulator.c new file +3
-0@@ -0,0 +1,746 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ +Please make the entire header block C++ so it looks intentional.
Sure, I'll change this.
I was trying to follow the guideline that kernel C source files should use C style comments while at the same time following the SPDX guideline that C++ style comments are needed for the SPDX line in C source files [1].
Quoted 4 lines
drivers/regulator/qcom-rpmh-regulator.c line 265
Quoted 1 line
drivers/regulator/qcom-rpmh-regulator.c new file +1
-0+ cmd.data = bypassed ?PMIC4_BOB_MODE_PASS : pmic_mode;Please just write normal if statements, the ternery operator isn't really helping legibility.
I'll change this.
Quoted 10 lines
drivers/regulator/qcom-rpmh-regulator.c line 488
Quoted 7 lines
drivers/regulator/qcom-rpmh-regulator.c new file +7
-0+static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY +1 ] = { + [REGULATOR_MODE_INVALID ] = -EINVAL , + [REGULATOR_MODE_STANDBY ] =PMIC4_LDO_MODE_RETENTION , + [REGULATOR_MODE_IDLE ] =PMIC4_LDO_MODE_LPM , + [REGULATOR_MODE_NORMAL ] = -EINVAL , + [REGULATOR_MODE_FAST ] =PMIC4_LDO_MODE_HPM , +};This mapping is really weird, I'd expect one of the modes to correspond to the normal operating mode of the regulator.
My thinking here was to have a consistent mapping for consumers to use between REGULATOR_MODE_* and physical regulator modes for both LDO and SMPS type regulators:
REGULATOR_MODE_STANDBY --> Retention (if supported)REGULATOR_MODE_IDLE --> Low power mode (if supported)
LPM for LDO and PFM for SMPSREGULATOR_MODE_NORMAL --> Auto HW switching between low and high power
mode (if supported)REGULATOR_MODE_FAST --> High power mode
HPM for LDO and PWM for SMPS
This allows a consumer to request NORMAL in typical use cases and FAST in use cases that require low voltage ripple. If NORMAL is not supported, then it automatically gets upgraded to FAST by the regulator framework.
I could change it so that REGULATOR_MODE_NORMAL maps to LDO HPM mode. However, doing so would make it so that REGULATOR_MODE_FAST requests would fail for LDOs. Thus, consumers would need to know if their supply is an LDO or an SMPS (which seems undesirable).
Would it be acceptable to have both NORMAL and FAST map to LDO HPM?
Quoted 11 lines
drivers/regulator/qcom-rpmh-regulator.c line 497
Quoted 8 lines
drivers/regulator/qcom-rpmh-regulator.c new file +8
-0+static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode (unsigned int mode) +{ +static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT ] = { + [RPMH_REGULATOR_MODE_RET ] =REGULATOR_MODE_STANDBY , + [RPMH_REGULATOR_MODE_LPM ] =REGULATOR_MODE_IDLE , + [RPMH_REGULATOR_MODE_AUTO ] =REGULATOR_MODE_INVALID , + [RPMH_REGULATOR_MODE_HPM ] =REGULATOR_MODE_FAST , + };Same here, based on that it looks like auto mode is a good map for normal.
LDO type regulators physically do not support AUTO mode. That is why I specified REGULATOR_MODE_INVALID in the mapping.
Quoted 4 lines
drivers/regulator/qcom-rpmh-regulator.c line 21
Quoted 2 lines
drivers/regulator/qcom-rpmh-regulator.c new file +2
-0+if (mode >=RPMH_REGULATOR_MODE_COUNT ) +return -EINVAL ;Why not use
ARRAY_SIZE?
I'll change this.
Thanks,
David
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Mark Brown
maintainerQuoted 14 lines · David Collins
Quoted 12 lines · Mark Brown
drivers/regulator/qcom-rpmh-regulator.c line 488
Quoted 7 lines · David Collins
drivers/regulator/qcom-rpmh-regulator.c new file +7
-0+static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY +1 ] = { + [REGULATOR_MODE_INVALID ] = -EINVAL , + [REGULATOR_MODE_STANDBY ] =PMIC4_LDO_MODE_RETENTION , + [REGULATOR_MODE_IDLE ] =PMIC4_LDO_MODE_LPM , + [REGULATOR_MODE_NORMAL ] = -EINVAL , + [REGULATOR_MODE_FAST ] =PMIC4_LDO_MODE_HPM , +};This mapping is really weird, I'd expect one of the modes to correspond to the normal operating mode of the regulator.
Quoted 3 lines
My thinking here was to have a consistent mapping for consumers to use between REGULATOR_MODE_* and physical regulator modes for both LDO and SMPS type regulators:
No, that's not useful or helpful - if there's any modes I'd *really* expect to see one of them be _NORMAL.
Quoted 8 lines
This allows a consumer to request NORMAL in typical use cases and FAST in use cases that require low voltage ripple. If NORMAL is not supported, then it automatically gets upgraded to FAST by the regulator framework.
I could change it so that
REGULATOR_MODE_NORMALmaps to LDO HPM mode. However, doing so would make it so thatREGULATOR_MODE_FASTrequests would fail for LDOs. Thus, consumers would need to know if their supply is an LDO or an SMPS (which seems undesirable).
You've just discovered why it's a bad idea for consumers to do anything with modes directly! The mappings are just never going to be consistent given the massive variation in what regulators can support, the retention mode of one regulator might be able to deliver more power than the normal mode of another.
Quoted 1 line
Would it be acceptable to have both NORMAL and FAST map to LDO HPM?
Having something other than a 1:1 mapping is going to lead to pain at some point.
Quoted 12 lines
Quoted 11 lines
drivers/regulator/qcom-rpmh-regulator.c line 497
Quoted 8 lines
drivers/regulator/qcom-rpmh-regulator.c new file +8
-0+static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode (unsigned int mode) +{ +static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT ] = { + [RPMH_REGULATOR_MODE_RET ] =REGULATOR_MODE_STANDBY , + [RPMH_REGULATOR_MODE_LPM ] =REGULATOR_MODE_IDLE , + [RPMH_REGULATOR_MODE_AUTO ] =REGULATOR_MODE_INVALID , + [RPMH_REGULATOR_MODE_HPM ] =REGULATOR_MODE_FAST , + };Same here, based on that it looks like auto mode is a good map for normal.
Quoted 2 lines
LDO type regulators physically do not support AUTO mode. That is why I specified
REGULATOR_MODE_INVALIDin the mapping.
The other question here is why this is even in the table if it's not
valid (I'm not seeing a need for the MODE_COUNT define)?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180712/aa913186/attachment.sig>
Quoted 15 lines · Mark Brown
Quoted 13 lines · David Collins
Quoted 12 lines · Mark Brown
drivers/regulator/qcom-rpmh-regulator.c line 497
Quoted 8 lines · David Collins
drivers/regulator/qcom-rpmh-regulator.c new file +8
-0+static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode (unsigned int mode) +{ +static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT ] = { + [RPMH_REGULATOR_MODE_RET ] =REGULATOR_MODE_STANDBY , + [RPMH_REGULATOR_MODE_LPM ] =REGULATOR_MODE_IDLE , + [RPMH_REGULATOR_MODE_AUTO ] =REGULATOR_MODE_INVALID , + [RPMH_REGULATOR_MODE_HPM ] =REGULATOR_MODE_FAST , + };Same here, based on that it looks like auto mode is a good map for normal.
Quoted 5 lines
Quoted 2 lines
LDO type regulators physically do not support AUTO mode. That is why I specified
REGULATOR_MODE_INVALIDin the mapping.The other question here is why this is even in the table if it's not valid (I'm not seeing a need for the
MODE_COUNTdefine)?
I thought that having a table would be more concise and easier to follow. I can change this to a switch case statement.
Take care,
David
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project