LinuxLists.cc - [PATCH 0/2] regulator: add QCOM RPMh regulator driver

[permalink] [raw]

Subject: [PATCH 2/2] dt-bindings: regulator: 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.

Signed-off-by: David Collins <[email protected]>
---
.../bindings/regulator/qcom,rpmh-regulator.txt | 246 +++++++++++++++++++++
1 file changed, 246 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt

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..2d86306
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
@@ -0,0 +1,246 @@
+Qualcomm Technologies, Inc. RPMh Regulators
+
+rpmh-regulator devices support PMIC regulator management via the VRM and XOB
+RPMh accelerators. The APPS processor communicates with these hardware blocks
+via an 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.
+
+=======================
+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
+- 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
+- vdd_lvs1_lvs2-supply
+- vdd_lvs1_lvs2-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: phandle of the parent supply regulator of one or more of the
+ regulators for this PMIC.
+
+- vdd_s1-supply
+- vdd_s2-supply
+- vdd_s3-supply
+- vdd_s4-supply
+ Usage: optional (PM8005 only)
+ Value type: <phandle>
+ Definition: phandle of the parent supply regulator of one or more of the
+ regulators for this PMIC.
+
+=========================================
+Second Level Nodes - Regulators
+=========================================
+
+- regulator-name
+ Usage: optional
+ Value type: <string>
+ Definition: Specifies the name for this RPMh regulator. If not
+ specified, then the regulator's name is equal to its subnode
+ name.
+
+- regulator-min-microvolt
+ Usage: required
+ Value type: <u32>
+ Definition: For VRM resources, this is the minimum supported voltage in
+ microvolts. For XOB resources, this is the fixed output
+ voltage.
+
+- regulator-max-microvolt
+ Usage: required
+ Value type: <u32>
+ Definition: For VRM resources, this is the maximum supported voltage in
+ microvolts. For XOB resources, this is the fixed output
+ voltage.
+
+- qcom,regulator-initial-voltage
+ Usage: optional; VRM regulators only
+ Value type: <u32>
+ Definition: Specifies the initial voltage in microvolts to request for a
+ VRM regulator. Supported values are 0 to 8191000.
+
+- 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 4).
+
+- regulator-allow-set-load
+ Usage: optional
+ Value type: <empty>
+ Definition: Boolean flag indicating that the the mode of this regulator
+ may be configured at runtime based upon consumer load needs.
+
+- qcom,allowed-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 4). Elements must be specified in order
+ from lowest to highest value.
+
+- qcom,mode-threshold-currents
+ Usage: required if regulator-allow-set-load is specified;
+ VRM regulators only
+ Value type: <prop-encoded-array>
+ Definition: A list of integers specifying the minimum allowed load
+ current in microamps for each of the modes listed in
+ qcom,allowed-modes. The first element should always be 0.
+ Elements must be specified in order from lowest to highest
+ value.
+
+- qcom,headroom-voltage
+ 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.
+
+ - regulator-enable-ramp-delay
+ Usage: optional
+ Value type: <u32>
+ Definition: The time in microseconds to delay after enabling a
+ regulator. Note that RPMh hardware ensures that regulator
+ output has stabilized before acknowledging a given regulator
+ enable request.
+
+- qcom,rpmh-resource-type
+ Usage: optional
+ Value type: <string>
+ Definition: RPMh accelerator type for this regulator. If not specified,
+ then the default type associated with this regulator will be
+ used. Supported values: "vrm" or "xob".
+
+- 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 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-voltage = <1100000>;
+ };
+
+ pm8998_s5: smps5 {
+ regulator-min-microvolt = <1904000>;
+ regulator-max-microvolt = <2040000>;
+ qcom,regulator-initial-voltage = <1904000>;
+ };
+
+ ldo7 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ qcom,regulator-initial-voltage = <1800000>;
+ qcom,headroom-voltage = <56000>;
+ regulator-initial-mode = <RPMH_REGULATOR_MODE_LPM>;
+ regulator-allow-set-load;
+ qcom,allowed-modes =
+ <RPMH_REGULATOR_MODE_LPM
+ RPMH_REGULATOR_MODE_HPM>;
+ qcom,mode-threshold-currents = <0 10000>;
+ };
+
+ 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-voltage = <3312000>;
+ regulator-initial-mode = <RPMH_REGULATOR_MODE_PASS>;
+ };
+ };
+};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-03-17 01:10:59

[permalink] [raw]

Subject: [PATCH 1/2] regulator: add QCOM RPMh regulator driver

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

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

Signed-off-by: David Collins <[email protected]>
---
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom_rpmh-regulator.c | 1124 ++++++++++++++++++++
.../dt-bindings/regulator/qcom,rpmh-regulator.h | 40 +
4 files changed, 1174 insertions(+)
create mode 100644 drivers/regulator/qcom_rpmh-regulator.c
create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 097f617..e0ecd0a 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
Qualcomm RPM as a module. The module will be named
"qcom_rpm-regulator".

+config REGULATOR_QCOM_RPMH
+ tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
+ depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+ help
+ This driver supports control of PMIC regulators via the RPMh hardware
+ block found on Qualcomm Technologies Inc. SoCs. RPMh regulator
+ control allows for voting on regulator state between multiple
+ processors within the SoC.
+
config REGULATOR_QCOM_SMD_RPM
tristate "Qualcomm SMD based RPM regulator driver"
depends on QCOM_SMD_RPM
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 590674f..c2274dd 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o
obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom_rpmh-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o
obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c
new file mode 100644
index 0000000..808f949
--- /dev/null
+++ b/drivers/regulator/qcom_rpmh-regulator.c
@@ -0,0 +1,1124 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+
+/**
+ * enum rpmh_regulator_type - supported RPMh accelerator types
+ * %RPMH_REGULATOR_TYPE_VRM: RPMh VRM accelerator which supports voting on
+ * enable, voltage, mode, and headroom voltage of
+ * LDO, SMPS, VS, and BOB type PMIC regulators.
+ * %RPMH_REGULATOR_TYPE_XOB: RPMh XOB accelerator which supports voting on
+ * the enable state of PMIC regulators.
+ */
+enum rpmh_regulator_type {
+ RPMH_REGULATOR_TYPE_VRM,
+ RPMH_REGULATOR_TYPE_XOB,
+};
+
+/* Min and max limits of VRM resource request parameters */
+#define RPMH_VRM_MIN_UV 0
+#define RPMH_VRM_MAX_UV 8191000
+
+#define RPMH_VRM_HEADROOM_MIN_UV 0
+#define RPMH_VRM_HEADROOM_MAX_UV 511000
+
+#define RPMH_VRM_MODE_MIN 0
+#define RPMH_VRM_MODE_MAX 7
+
+/* Register offsets: */
+#define RPMH_REGULATOR_REG_VRM_VOLTAGE 0x0
+#define RPMH_REGULATOR_REG_ENABLE 0x4
+#define RPMH_REGULATOR_REG_VRM_MODE 0x8
+#define RPMH_REGULATOR_REG_VRM_HEADROOM 0xC
+
+/* Enable register values: */
+#define RPMH_REGULATOR_DISABLE 0x0
+#define RPMH_REGULATOR_ENABLE 0x1
+
+/* Number of unique hardware modes supported: */
+#define RPMH_REGULATOR_MODE_COUNT 5
+
+/**
+ * struct rpmh_regulator_mode - RPMh VRM mode attributes
+ * @pmic_mode: Raw PMIC mode value written into VRM mode voting
+ * register (i.e. RPMH_REGULATOR_MODE_*)
+ * @framework_mode: Regulator framework mode value
+ * (i.e. REGULATOR_MODE_*)
+ * @min_load_ua: The minimum load current in microamps which
+ * would utilize this mode
+ *
+ * Software selects the lowest mode for which aggr_load_ua >= min_load_ua.
+ */
+struct rpmh_regulator_mode {
+ u32 pmic_mode;
+ u32 framework_mode;
+ int min_load_ua;
+};
+
+/**
+ * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
+ * @mode_map: Array of size RPMH_REGULATOR_MODE_COUNT which
+ * maps RPMH_REGULATOR_MODE_* indices into PMIC
+ * mode and regulator framework mode that are
+ * supported by this PMIC regulator type
+ * @voltage_range: The single range of voltages supported by this
+ * PMIC regulator type
+ * @n_voltages: The number of unique voltage set points defined
+ * by voltage_range
+ * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
+ * in device tree to a regulator framework mode
+ */
+struct rpmh_vreg_hw_data {
+ const struct rpmh_regulator_mode *mode_map;
+ const struct regulator_linear_range *voltage_range;
+ int n_voltages;
+ unsigned int (*of_map_mode)(unsigned int mode);
+};
+
+struct rpmh_pmic;
+
+/**
+ * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
+ * single regulator device
+ * @of_node: Device tree node pointer of the regulator
+ * @pmic: Pointer to the PMIC containing the regulator
+ * @resource_name: Name of the RPMh regulator resource which is
+ * mapped to an RPMh accelerator address via
+ * command DB. This name must match to one that is
+ * defined by the bootloader.
+ * @addr: Base address of the regulator resource within
+ * an RPMh accelerator
+ * @rdesc: Regulator descriptor
+ * @rdev: Regulator device pointer returned by
+ * devm_regulator_register()
+ * @hw_data: PMIC regulator configuration data for this RPMh
+ * regulator
+ * @regulator_type: RPMh accelerator type for this regulator
+ * resource
+ * @always_wait_for_ack: Boolean flag indicating if a request must always
+ * wait for an ACK from RPMh before continuing even
+ * if it corresponds to a strictly lower power
+ * state (e.g. enabled --> disabled).
+ * @mode_map: An array of modes which may be configured at
+ * runtime by setting the load current
+ * @mode_count: The number of entries in the mode_map array.
+ * @enabled: Boolean indicating if the regulator is enabled
+ * or not
+ * @voltage: RPMh VRM regulator voltage in microvolts
+ * @mode: RPMh VRM regulator current framework mode
+ * @headroom_voltage: RPMh VRM regulator minimum headroom voltage
+ * required
+ */
+struct rpmh_vreg {
+ struct device_node *of_node;
+ struct rpmh_pmic *pmic;
+ const char *resource_name;
+ u32 addr;
+ struct regulator_desc rdesc;
+ struct regulator_dev *rdev;
+ const struct rpmh_vreg_hw_data *hw_data;
+ enum rpmh_regulator_type regulator_type;
+ bool always_wait_for_ack;
+ struct rpmh_regulator_mode *mode_map;
+ int mode_count;
+
+ bool enabled;
+ int voltage;
+ unsigned int mode;
+ int headroom_voltage;
+};
+
+/**
+ * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
+ * @name: Name for the regulator which also corresponds
+ * to the device tree subnode name of the regulator
+ * @resource_name_base: RPMh regulator resource name prefix. E.g.
+ * "ldo" for RPMh resource "ldoa1".
+ * @supply_name: Parent supply regulator name
+ * @id: Regulator number within the PMIC
+ * @regulator_type: RPMh accelerator type used to manage this
+ * regulator
+ * @hw_data: Configuration data for this PMIC regulator type
+ */
+struct rpmh_vreg_init_data {
+ const char *name;
+ const char *resource_name_base;
+ const char *supply_name;
+ int id;
+ enum rpmh_regulator_type regulator_type;
+ const struct rpmh_vreg_hw_data *hw_data;
+};
+
+/**
+ * struct rpmh_pmic_init_data - initialization data for a PMIC
+ * @name: PMIC name
+ * @vreg_data: Array of data for each regulator in the PMIC
+ * @count: Number of entries in vreg_data
+ */
+struct rpmh_pmic_init_data {
+ const char *name;
+ const struct rpmh_vreg_init_data *vreg_data;
+ int count;
+};
+
+/**
+ * struct rpmh_pmic - top level data structure of all regulators found on a PMIC
+ * @dev: Device pointer of the PMIC device for the
+ * regulators
+ * @rpmh_client: Handle used for rpmh communications
+ * @vreg: Array of rpmh regulator structs representing the
+ * individual regulators found on this PMIC chip
+ * which are configured via device tree.
+ * @vreg_count: The number of entries in the vreg array.
+ * @pmic_id: Letter used to identify this PMIC within the
+ * system. This is dictated by boot loader
+ * specifications on a given target.
+ * @init_data: Pointer to the matched PMIC initialization data
+ */
+struct rpmh_pmic {
+ struct device *dev;
+ struct rpmh_client *rpmh_client;
+ struct rpmh_vreg *vreg;
+ int vreg_count;
+ const char *pmic_id;
+ const struct rpmh_pmic_init_data *init_data;
+};
+
+#define vreg_err(vreg, message, ...) \
+ pr_err("%s %s: " message, (vreg)->pmic->init_data->name, \
+ (vreg)->rdesc.name, ##__VA_ARGS__)
+#define vreg_info(vreg, message, ...) \
+ pr_info("%s %s: " message, (vreg)->pmic->init_data->name, \
+ (vreg)->rdesc.name, ##__VA_ARGS__)
+#define vreg_debug(vreg, message, ...) \
+ pr_debug("%s %s: " message, (vreg)->pmic->init_data->name, \
+ (vreg)->rdesc.name, ##__VA_ARGS__)
+
+/**
+ * rpmh_regulator_send_request() - send the request to RPMh
+ * @vreg: Pointer to the RPMh regulator
+ * @cmd: RPMh commands to send
+ * @count: Size of cmd array
+ * @wait_for_ack: Boolean indicating if execution must wait until the
+ * request has been acknowledged as complete
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
+ struct tcs_cmd *cmd, int count, bool wait_for_ack)
+{
+ int ret;
+
+ if (wait_for_ack || vreg->always_wait_for_ack)
+ ret = rpmh_write(vreg->pmic->rpmh_client,
+ RPMH_ACTIVE_ONLY_STATE, cmd, count);
+ else
+ ret = rpmh_write_async(vreg->pmic->rpmh_client,
+ RPMH_ACTIVE_ONLY_STATE, cmd, count);
+ if (ret < 0)
+ vreg_err(vreg, "rpmh_write() failed, ret=%d\n", ret);
+
+ return ret;
+}
+
+static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->enabled;
+}
+
+static int rpmh_regulator_enable(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+ .data = RPMH_REGULATOR_ENABLE,
+ };
+ int ret;
+
+ if (vreg->enabled)
+ return 0;
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
+ if (ret < 0) {
+ vreg_err(vreg, "enable failed, ret=%d\n", ret);
+ return ret;
+ }
+
+ vreg->enabled = true;
+
+ return 0;
+}
+
+static int rpmh_regulator_disable(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+ .data = RPMH_REGULATOR_DISABLE,
+ };
+ int ret;
+
+ if (!vreg->enabled)
+ return 0;
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1, false);
+ if (ret < 0) {
+ vreg_err(vreg, "disable failed, ret=%d\n", ret);
+ return ret;
+ }
+
+ vreg->enabled = false;
+
+ return 0;
+}
+
+static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
+ int min_uv, int max_uv, unsigned int *selector)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+ };
+ const struct regulator_linear_range *range;
+ int mv, uv, ret;
+ bool wait_for_ack;
+
+ mv = DIV_ROUND_UP(min_uv, 1000);
+ uv = mv * 1000;
+ if (uv > max_uv) {
+ vreg_err(vreg, "no set points available in range %d-%d uV\n",
+ min_uv, max_uv);
+ return -EINVAL;
+ }
+
+ range = vreg->hw_data->voltage_range;
+ *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step);
+
+ if (uv == vreg->voltage)
+ return 0;
+
+ wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage;
+ cmd.data = mv;
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1, wait_for_ack);
+ if (ret < 0) {
+ vreg_err(vreg, "set voltage=%d uV failed, ret=%d\n", uv, ret);
+ return ret;
+ }
+
+ vreg->voltage = uv;
+
+ return 0;
+}
+
+static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->voltage;
+}
+
+static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
+ unsigned int mode)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+ };
+ int i, ret;
+
+ if (mode == vreg->mode)
+ return 0;
+
+ for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
+ if (vreg->hw_data->mode_map[i].framework_mode == mode)
+ break;
+ if (i >= RPMH_REGULATOR_MODE_COUNT) {
+ vreg_err(vreg, "invalid mode=%u\n", mode);
+ return -EINVAL;
+ }
+
+ cmd.data = vreg->hw_data->mode_map[i].pmic_mode;
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1,
+ mode < vreg->mode || !vreg->mode);
+ if (ret < 0) {
+ vreg_err(vreg, "set mode=%u failed, ret=%d\n", cmd.data, ret);
+ return ret;
+ }
+
+ vreg->mode = mode;
+
+ return ret;
+}
+
+static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->mode;
+}
+
+/**
+ * rpmh_regulator_vrm_set_load() - set the PMIC mode based upon the maximum load
+ * required from the VRM rpmh-regulator
+ * @rdev: Regulator device pointer for the rpmh-regulator
+ * @load_ua: Maximum current required from all consumers in microamps
+ *
+ * This function is passed as a callback function into the regulator ops that
+ * are registered for each VRM rpmh-regulator device.
+ *
+ * This function sets the mode of the regulator to that which has the highest
+ * min support load less than or equal to load_ua. Example:
+ * mode_count = 3
+ * mode_map[].min_load_ua = 0, 100000, 6000000
+ *
+ * load_ua = 10000 --> i = 0
+ * load_ua = 250000 --> i = 1
+ * load_ua = 7000000 --> i = 2
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_ua)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ int i;
+
+ /* No need to check element 0 as it will be the default. */
+ for (i = vreg->mode_count - 1; i > 0; i--)
+ if (vreg->mode_map[i].min_load_ua <= load_ua)
+ break;
+
+ return rpmh_regulator_vrm_set_mode(rdev,
+ vreg->mode_map[i].framework_mode);
+}
+
+static const struct regulator_ops rpmh_regulator_vrm_ops = {
+ .enable = rpmh_regulator_enable,
+ .disable = rpmh_regulator_disable,
+ .is_enabled = rpmh_regulator_is_enabled,
+ .set_voltage = rpmh_regulator_vrm_set_voltage,
+ .get_voltage = rpmh_regulator_vrm_get_voltage,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_mode = rpmh_regulator_vrm_set_mode,
+ .get_mode = rpmh_regulator_vrm_get_mode,
+ .set_load = rpmh_regulator_vrm_set_load,
+};
+
+static const struct regulator_ops rpmh_regulator_xob_ops = {
+ .enable = rpmh_regulator_enable,
+ .disable = rpmh_regulator_disable,
+ .is_enabled = rpmh_regulator_is_enabled,
+};
+
+static const struct regulator_ops *rpmh_regulator_ops[] = {
+ [RPMH_REGULATOR_TYPE_VRM] = &rpmh_regulator_vrm_ops,
+ [RPMH_REGULATOR_TYPE_XOB] = &rpmh_regulator_xob_ops,
+};
+
+/**
+ * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
+ * for a VRM RPMh resource from device tree
+ * vreg: Pointer to the rpmh regulator resource
+ *
+ * This function initializes the mode[] array of vreg based upon the values
+ * of optional device tree properties.
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
+{
+ struct device_node *node = vreg->of_node;
+ const struct rpmh_regulator_mode *map;
+ const char *prop;
+ int i, len, ret;
+ u32 *buf;
+
+ map = vreg->hw_data->mode_map;
+ if (!map)
+ return 0;
+
+ /* qcom,allowed-modes is optional */
+ prop = "qcom,allowed-modes";
+ len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+ if (len < 0)
+ return 0;
+
+ vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len,
+ sizeof(*vreg->mode_map), GFP_KERNEL);
+ if (!vreg->mode_map)
+ return -ENOMEM;
+ vreg->mode_count = len;
+
+ buf = kcalloc(len, sizeof(*buf), GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = of_property_read_u32_array(node, prop, buf, len);
+ if (ret < 0) {
+ vreg_err(vreg, "unable to read %s, ret=%d\n",
+ prop, ret);
+ goto done;
+ }
+
+ for (i = 0; i < len; i++) {
+ if (buf[i] >= RPMH_REGULATOR_MODE_COUNT
+ || !map[buf[i]].framework_mode) {
+ vreg_err(vreg, "element %d of %s = %u is invalid for this regulator\n",
+ i, prop, buf[i]);
+ ret = -EINVAL;
+ goto done;
+ }
+
+ vreg->mode_map[i].pmic_mode = map[buf[i]].pmic_mode;
+ vreg->mode_map[i].framework_mode = map[buf[i]].framework_mode;
+
+ if (i > 0 && vreg->mode_map[i].pmic_mode
+ <= vreg->mode_map[i - 1].pmic_mode) {
+ vreg_err(vreg, "%s elements are not in ascending order\n",
+ prop);
+ ret = -EINVAL;
+ goto done;
+ }
+ }
+
+ prop = "qcom,mode-threshold-currents";
+ ret = of_property_read_u32_array(node, prop, buf, len);
+ if (ret < 0) {
+ vreg_err(vreg, "unable to read %s, ret=%d\n",
+ prop, ret);
+ goto done;
+ }
+
+ for (i = 0; i < len; i++) {
+ vreg->mode_map[i].min_load_ua = buf[i];
+
+ if (i > 0 && vreg->mode_map[i].min_load_ua
+ <= vreg->mode_map[i - 1].min_load_ua) {
+ vreg_err(vreg, "%s elements are not in ascending order\n",
+ prop);
+ ret = -EINVAL;
+ goto done;
+ }
+ }
+
+done:
+ kfree(buf);
+ return ret;
+}
+
+/**
+ * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated
+ * with the PMIC and initialize important pointers for each
+ * regulator
+ * @pmic: Pointer to the RPMh regulator PMIC
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic)
+{
+ struct device_node *node;
+ int i;
+
+ pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node);
+ if (pmic->vreg_count == 0) {
+ dev_err(pmic->dev, "could not find any regulator subnodes\n");
+ return -ENODEV;
+ }
+
+ pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count,
+ sizeof(*pmic->vreg), GFP_KERNEL);
+ if (!pmic->vreg)
+ return -ENOMEM;
+
+ i = 0;
+ for_each_available_child_of_node(pmic->dev->of_node, node) {
+ pmic->vreg[i].of_node = node;
+ pmic->vreg[i].pmic = pmic;
+
+ i++;
+ }
+
+ return 0;
+}
+
+/**
+ * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
+ * request for this regulator based on optional device tree
+ * properties
+ * @vreg: Pointer to the RPMh regulator
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
+{
+ struct tcs_cmd cmd[2] = { };
+ const char *prop;
+ int cmd_count = 0;
+ int ret;
+ u32 temp;
+
+ if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
+ prop = "qcom,headroom-voltage";
+ ret = of_property_read_u32(vreg->of_node, prop, &temp);
+ if (!ret) {
+ if (temp < RPMH_VRM_HEADROOM_MIN_UV ||
+ temp > RPMH_VRM_HEADROOM_MAX_UV) {
+ vreg_err(vreg, "%s=%u is invalid\n",
+ prop, temp);
+ return -EINVAL;
+ }
+ vreg->headroom_voltage = temp;
+
+ cmd[cmd_count].addr
+ = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
+ cmd[cmd_count++].data
+ = DIV_ROUND_UP(vreg->headroom_voltage, 1000);
+ }
+
+ prop = "qcom,regulator-initial-voltage";
+ ret = of_property_read_u32(vreg->of_node, prop, &temp);
+ if (!ret) {
+ if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) {
+ vreg_err(vreg, "%s=%u is invalid\n",
+ prop, temp);
+ return -EINVAL;
+ }
+ vreg->voltage = temp;
+
+ cmd[cmd_count].addr
+ = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
+ cmd[cmd_count++].data
+ = DIV_ROUND_UP(vreg->voltage, 1000);
+ }
+ }
+
+ if (cmd_count) {
+ ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true);
+ if (ret < 0) {
+ vreg_err(vreg, "could not send default config, ret=%d\n",
+ ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator
+ * @vreg: Pointer to the RPMh regulator
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg)
+{
+ struct device *dev = vreg->pmic->dev;
+ struct regulator_config reg_config = {};
+ const struct rpmh_vreg_init_data *rpmh_data = NULL;
+ const char *type_name = NULL;
+ enum rpmh_regulator_type type;
+ struct regulator_init_data *init_data;
+ int ret, i;
+
+ for (i = 0; i < vreg->pmic->init_data->count; i++) {
+ if (!strcmp(vreg->pmic->init_data->vreg_data[i].name,
+ vreg->of_node->name)) {
+ rpmh_data = &vreg->pmic->init_data->vreg_data[i];
+ break;
+ }
+ }
+
+ if (!rpmh_data) {
+ dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n",
+ vreg->of_node->name, vreg->pmic->init_data->name);
+ return -EINVAL;
+ }
+
+ vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d",
+ rpmh_data->resource_name_base, vreg->pmic->pmic_id,
+ rpmh_data->id);
+ if (!vreg->resource_name)
+ return -ENOMEM;
+
+ vreg->addr = cmd_db_read_addr(vreg->resource_name);
+ if (!vreg->addr) {
+ vreg_err(vreg, "could not find RPMh address for resource %s\n",
+ vreg->resource_name);
+ return -ENODEV;
+ }
+
+ vreg->rdesc.name = rpmh_data->name;
+ vreg->rdesc.supply_name = rpmh_data->supply_name;
+ vreg->regulator_type = rpmh_data->regulator_type;
+ vreg->hw_data = rpmh_data->hw_data;
+
+ if (rpmh_data->hw_data->voltage_range) {
+ vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range;
+ vreg->rdesc.n_linear_ranges = 1;
+ vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages;
+ }
+
+ /* Optional override for the default RPMh accelerator type */
+ ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",
+ &type_name);
+ if (!ret) {
+ if (!strcmp("vrm", type_name)) {
+ vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM;
+ } else if (!strcmp("xob", type_name)) {
+ vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB;
+ } else {
+ vreg_err(vreg, "Unknown RPMh accelerator type %s\n",
+ type_name);
+ return -EINVAL;
+ }
+ }
+
+ type = vreg->regulator_type;
+
+ if (type == RPMH_REGULATOR_TYPE_VRM) {
+ ret = rpmh_regulator_parse_vrm_modes(vreg);
+ if (ret < 0) {
+ vreg_err(vreg, "could not parse vrm mode mapping, ret=%d\n",
+ ret);
+ return ret;
+ }
+ }
+
+ vreg->always_wait_for_ack = of_property_read_bool(vreg->of_node,
+ "qcom,always-wait-for-ack");
+
+ vreg->rdesc.owner = THIS_MODULE;
+ vreg->rdesc.type = REGULATOR_VOLTAGE;
+ vreg->rdesc.ops = rpmh_regulator_ops[type];
+ vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode;
+
+ init_data = of_get_regulator_init_data(dev, vreg->of_node,
+ &vreg->rdesc);
+ if (!init_data)
+ return -ENOMEM;
+
+ if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) {
+ vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
+ init_data->constraints.apply_uV = 0;
+ vreg->rdesc.n_voltages = 1;
+ }
+
+ if (vreg->hw_data->mode_map) {
+ init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
+ for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
+ init_data->constraints.valid_modes_mask
+ |= vreg->hw_data->mode_map[i].framework_mode;
+ }
+
+ reg_config.dev = dev;
+ reg_config.init_data = init_data;
+ reg_config.of_node = vreg->of_node;
+ reg_config.driver_data = vreg;
+
+ ret = rpmh_regulator_load_default_parameters(vreg);
+ if (ret < 0) {
+ vreg_err(vreg, "unable to load default parameters, ret=%d\n",
+ ret);
+ return ret;
+ }
+
+ vreg->rdev = devm_regulator_register(dev, &vreg->rdesc, &reg_config);
+ if (IS_ERR(vreg->rdev)) {
+ ret = PTR_ERR(vreg->rdev);
+ vreg->rdev = NULL;
+ vreg_err(vreg, "devm_regulator_register() failed, ret=%d\n",
+ ret);
+ return ret;
+ }
+
+ vreg_debug(vreg, "registered RPMh resource %s @ 0x%05X\n",
+ vreg->resource_name, vreg->addr);
+
+ return ret;
+}
+
+/*
+ * Mappings from RPMh generic modes to VRM accelerator modes and regulator
+ * framework modes for each regulator type.
+ */
+static const struct rpmh_regulator_mode
+rpmh_regulator_mode_map_pmic4_ldo[RPMH_REGULATOR_MODE_COUNT] = {
+ [RPMH_REGULATOR_MODE_RET] = {
+ .pmic_mode = 4,
+ .framework_mode = REGULATOR_MODE_STANDBY,
+ },
+ [RPMH_REGULATOR_MODE_LPM] = {
+ .pmic_mode = 5,
+ .framework_mode = REGULATOR_MODE_IDLE,
+ },
+ [RPMH_REGULATOR_MODE_HPM] = {
+ .pmic_mode = 7,
+ .framework_mode = REGULATOR_MODE_FAST,
+ },
+};
+
+static const struct rpmh_regulator_mode
+rpmh_regulator_mode_map_pmic4_smps[RPMH_REGULATOR_MODE_COUNT] = {
+ [RPMH_REGULATOR_MODE_RET] = {
+ .pmic_mode = 4,
+ .framework_mode = REGULATOR_MODE_STANDBY,
+ },
+ [RPMH_REGULATOR_MODE_LPM] = {
+ .pmic_mode = 5,
+ .framework_mode = REGULATOR_MODE_IDLE,
+ },
+ [RPMH_REGULATOR_MODE_AUTO] = {
+ .pmic_mode = 6,
+ .framework_mode = REGULATOR_MODE_NORMAL,
+ },
+ [RPMH_REGULATOR_MODE_HPM] = {
+ .pmic_mode = 7,
+ .framework_mode = REGULATOR_MODE_FAST,
+ },
+};
+
+static const struct rpmh_regulator_mode
+rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = {
+ [RPMH_REGULATOR_MODE_PASS] = {
+ .pmic_mode = 0,
+ .framework_mode = REGULATOR_MODE_STANDBY,
+ },
+ [RPMH_REGULATOR_MODE_LPM] = {
+ .pmic_mode = 1,
+ .framework_mode = REGULATOR_MODE_IDLE,
+ },
+ [RPMH_REGULATOR_MODE_AUTO] = {
+ .pmic_mode = 2,
+ .framework_mode = REGULATOR_MODE_NORMAL,
+ },
+ [RPMH_REGULATOR_MODE_HPM] = {
+ .pmic_mode = 3,
+ .framework_mode = REGULATOR_MODE_FAST,
+ },
+};
+
+static unsigned int rpmh_regulator_vrm_of_map_mode(unsigned int mode,
+ const struct rpmh_regulator_mode *mode_map)
+{
+ if (mode > RPMH_REGULATOR_MODE_COUNT)
+ return -EINVAL;
+ else if (mode_map[mode].framework_mode == 0)
+ return -EINVAL;
+
+ return mode_map[mode].framework_mode;
+}
+
+static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode)
+{
+ return rpmh_regulator_vrm_of_map_mode(mode,
+ rpmh_regulator_mode_map_pmic4_ldo);
+}
+
+static unsigned int rpmh_regulator_pmic4_smps_of_map_mode(unsigned int mode)
+{
+ return rpmh_regulator_vrm_of_map_mode(mode,
+ rpmh_regulator_mode_map_pmic4_smps);
+}
+
+static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
+{
+ return rpmh_regulator_vrm_of_map_mode(mode,
+ rpmh_regulator_mode_map_pmic4_bob);
+}
+
+static const struct rpmh_vreg_hw_data pmic4_pldo_hw_data = {
+ .voltage_range = &(const struct regulator_linear_range)
+ REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),
+ .n_voltages = 256,
+ .mode_map = rpmh_regulator_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_pldo_lv_hw_data = {
+ .voltage_range = &(const struct regulator_linear_range)
+ REGULATOR_LINEAR_RANGE(1256000, 0, 127, 8000),
+ .n_voltages = 128,
+ .mode_map = rpmh_regulator_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_nldo_hw_data = {
+ .voltage_range = &(const struct regulator_linear_range)
+ REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000),
+ .n_voltages = 128,
+ .mode_map = rpmh_regulator_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_hfsmps3_hw_data = {
+ .voltage_range = &(const struct regulator_linear_range)
+ REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
+ .n_voltages = 216,
+ .mode_map = rpmh_regulator_mode_map_pmic4_smps,
+ .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_ftsmps426_hw_data = {
+ .voltage_range = &(const struct regulator_linear_range)
+ REGULATOR_LINEAR_RANGE(320000, 0, 258, 4000),
+ .n_voltages = 259,
+ .mode_map = rpmh_regulator_mode_map_pmic4_smps,
+ .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_bob_hw_data = {
+ .voltage_range = &(const struct regulator_linear_range)
+ REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
+ .n_voltages = 84,
+ .mode_map = rpmh_regulator_mode_map_pmic4_bob,
+ .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_lvs_hw_data = {
+ /* LVS hardware does not support voltage or mode configuration. */
+};
+
+#define RPMH_VREG(_name, _hw_type, _type, _base_name, _id, _supply_name) \
+{ \
+ .name = #_name, \
+ .hw_data = &_hw_type##_hw_data, \
+ .regulator_type = RPMH_REGULATOR_TYPE_##_type, \
+ .resource_name_base = #_base_name, \
+ .id = _id, \
+ .supply_name = #_supply_name, \
+}
+
+static const struct rpmh_vreg_init_data pm8998_vreg_data[] = {
+ RPMH_VREG(smps1, pmic4_ftsmps426, VRM, smp, 1, vdd_s1),
+ RPMH_VREG(smps2, pmic4_ftsmps426, VRM, smp, 2, vdd_s2),
+ RPMH_VREG(smps3, pmic4_hfsmps3, VRM, smp, 3, vdd_s3),
+ RPMH_VREG(smps4, pmic4_hfsmps3, VRM, smp, 4, vdd_s4),
+ RPMH_VREG(smps5, pmic4_hfsmps3, VRM, smp, 5, vdd_s5),
+ RPMH_VREG(smps6, pmic4_ftsmps426, VRM, smp, 6, vdd_s6),
+ RPMH_VREG(smps7, pmic4_ftsmps426, VRM, smp, 7, vdd_s7),
+ RPMH_VREG(smps8, pmic4_ftsmps426, VRM, smp, 8, vdd_s8),
+ RPMH_VREG(smps9, pmic4_ftsmps426, VRM, smp, 9, vdd_s9),
+ RPMH_VREG(smps10, pmic4_ftsmps426, VRM, smp, 10, vdd_s10),
+ RPMH_VREG(smps11, pmic4_ftsmps426, VRM, smp, 11, vdd_s11),
+ RPMH_VREG(smps12, pmic4_ftsmps426, VRM, smp, 12, vdd_s12),
+ RPMH_VREG(smps13, pmic4_ftsmps426, VRM, smp, 13, vdd_s13),
+ RPMH_VREG(ldo1, pmic4_nldo, VRM, ldo, 1, vdd_l1_l27),
+ RPMH_VREG(ldo2, pmic4_nldo, VRM, ldo, 2, vdd_l2_l8_l17),
+ RPMH_VREG(ldo3, pmic4_nldo, VRM, ldo, 3, vdd_l3_l11),
+ RPMH_VREG(ldo4, pmic4_nldo, VRM, ldo, 4, vdd_l4_l5),
+ RPMH_VREG(ldo5, pmic4_nldo, VRM, ldo, 5, vdd_l4_l5),
+ RPMH_VREG(ldo6, pmic4_pldo, VRM, ldo, 6, vdd_l6),
+ RPMH_VREG(ldo7, pmic4_pldo_lv, VRM, ldo, 7, vdd_l7_l12_l14_l15),
+ RPMH_VREG(ldo8, pmic4_nldo, VRM, ldo, 8, vdd_l2_l8_l17),
+ RPMH_VREG(ldo9, pmic4_pldo, VRM, ldo, 9, vdd_l9),
+ RPMH_VREG(ldo10, pmic4_pldo, VRM, ldo, 10, vdd_l10_l23_l25),
+ RPMH_VREG(ldo11, pmic4_nldo, VRM, ldo, 11, vdd_l3_l11),
+ RPMH_VREG(ldo12, pmic4_pldo_lv, VRM, ldo, 12, vdd_l7_l12_l14_l15),
+ RPMH_VREG(ldo13, pmic4_pldo, VRM, ldo, 13, vdd_l13_l19_l21),
+ RPMH_VREG(ldo14, pmic4_pldo_lv, VRM, ldo, 14, vdd_l7_l12_l14_l15),
+ RPMH_VREG(ldo15, pmic4_pldo_lv, VRM, ldo, 15, vdd_l7_l12_l14_l15),
+ RPMH_VREG(ldo16, pmic4_pldo, VRM, ldo, 16, vdd_l16_l28),
+ RPMH_VREG(ldo17, pmic4_nldo, VRM, ldo, 17, vdd_l2_l8_l17),
+ RPMH_VREG(ldo18, pmic4_pldo, VRM, ldo, 18, vdd_l18_l22),
+ RPMH_VREG(ldo19, pmic4_pldo, VRM, ldo, 19, vdd_l13_l19_l21),
+ RPMH_VREG(ldo20, pmic4_pldo, VRM, ldo, 20, vdd_l20_l24),
+ RPMH_VREG(ldo21, pmic4_pldo, VRM, ldo, 21, vdd_l13_l19_l21),
+ RPMH_VREG(ldo22, pmic4_pldo, VRM, ldo, 22, vdd_l18_l22),
+ RPMH_VREG(ldo23, pmic4_pldo, VRM, ldo, 23, vdd_l10_l23_l25),
+ RPMH_VREG(ldo24, pmic4_pldo, VRM, ldo, 24, vdd_l20_l24),
+ RPMH_VREG(ldo25, pmic4_pldo, VRM, ldo, 25, vdd_l10_l23_l25),
+ RPMH_VREG(ldo26, pmic4_nldo, VRM, ldo, 26, vdd_l26),
+ RPMH_VREG(ldo27, pmic4_nldo, VRM, ldo, 27, vdd_l1_l27),
+ RPMH_VREG(ldo28, pmic4_pldo, VRM, ldo, 28, vdd_l16_l28),
+ RPMH_VREG(lvs1, pmic4_lvs, XOB, vs, 1, vdd_lvs1_lvs2),
+ RPMH_VREG(lvs2, pmic4_lvs, XOB, vs, 2, vdd_lvs1_lvs2),
+};
+
+static const struct rpmh_vreg_init_data pmi8998_vreg_data[] = {
+ RPMH_VREG(bob, pmic4_bob, VRM, bob, 1, vdd_bob),
+};
+
+static const struct rpmh_vreg_init_data pm8005_vreg_data[] = {
+ RPMH_VREG(smps1, pmic4_ftsmps426, VRM, smp, 1, vdd_s1),
+ RPMH_VREG(smps2, pmic4_ftsmps426, VRM, smp, 2, vdd_s2),
+ RPMH_VREG(smps3, pmic4_ftsmps426, VRM, smp, 3, vdd_s3),
+ RPMH_VREG(smps4, pmic4_ftsmps426, VRM, smp, 4, vdd_s4),
+};
+
+static const struct rpmh_pmic_init_data pm8998_pmic_data = {
+ .name = "PM8998",
+ .vreg_data = pm8998_vreg_data,
+ .count = ARRAY_SIZE(pm8998_vreg_data),
+};
+
+static const struct rpmh_pmic_init_data pmi8998_pmic_data = {
+ .name = "PMI8998",
+ .vreg_data = pmi8998_vreg_data,
+ .count = ARRAY_SIZE(pmi8998_vreg_data),
+};
+
+static const struct rpmh_pmic_init_data pm8005_pmic_data = {
+ .name = "PM8005",
+ .vreg_data = pm8005_vreg_data,
+ .count = ARRAY_SIZE(pm8005_vreg_data),
+};
+
+static const struct of_device_id rpmh_regulator_match_table[] = {
+ {
+ .compatible = "qcom,pm8998-rpmh-regulators",
+ .data = &pm8998_pmic_data,
+ },
+ {
+ .compatible = "qcom,pmi8998-rpmh-regulators",
+ .data = &pmi8998_pmic_data,
+ },
+ {
+ .compatible = "qcom,pm8005-rpmh-regulators",
+ .data = &pm8005_pmic_data,
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table);
+
+/**
+ * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each
+ * of the regulator nodes associated with it
+ * @pdev: Pointer to the platform device of the RPMh PMIC
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ const struct of_device_id *match;
+ struct rpmh_pmic *pmic;
+ struct device_node *node;
+ int ret, i;
+
+ node = dev->of_node;
+
+ if (!node) {
+ dev_err(dev, "Device tree node is missing\n");
+ return -EINVAL;
+ }
+
+ ret = cmd_db_ready();
+ if (ret < 0) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Command DB not available, ret=%d\n", ret);
+ return ret;
+ }
+
+ pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+ if (!pmic)
+ return -ENOMEM;
+
+ pmic->dev = dev;
+ platform_set_drvdata(pdev, pmic);
+
+ pmic->rpmh_client = rpmh_get_client(pdev);
+ if (IS_ERR(pmic->rpmh_client)) {
+ ret = PTR_ERR(pmic->rpmh_client);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to request RPMh client, ret=%d\n",
+ ret);
+ return ret;
+ }
+
+ match = of_match_node(rpmh_regulator_match_table, node);
+ if (match) {
+ pmic->init_data = match->data;
+ } else {
+ dev_err(dev, "could not find compatible string match\n");
+ ret = -ENODEV;
+ goto cleanup;
+ }
+
+ ret = of_property_read_string(node, "qcom,pmic-id",
+ &pmic->pmic_id);
+ if (ret < 0) {
+ dev_err(dev, "qcom,pmic-id missing in DT node\n");
+ goto cleanup;
+ }
+
+ ret = rpmh_regulator_allocate_vreg(pmic);
+ if (ret < 0) {
+ dev_err(dev, "failed to allocate regulator subnode array, ret=%d\n",
+ ret);
+ goto cleanup;
+ }
+
+ for (i = 0; i < pmic->vreg_count; i++) {
+ ret = rpmh_regulator_init_vreg(&pmic->vreg[i]);
+ if (ret < 0) {
+ dev_err(dev, "unable to initialize rpmh-regulator vreg %s, ret=%d\n",
+ pmic->vreg[i].of_node->name, ret);
+ goto cleanup;
+ }
+ }
+
+ dev_dbg(dev, "successfully probed %d %s regulators\n",
+ pmic->vreg_count, pmic->init_data->name);
+
+ return ret;
+
+cleanup:
+ rpmh_release(pmic->rpmh_client);
+
+ return ret;
+}
+
+static int rpmh_regulator_remove(struct platform_device *pdev)
+{
+ struct rpmh_pmic *pmic = platform_get_drvdata(pdev);
+
+ rpmh_release(pmic->rpmh_client);
+
+ return 0;
+}
+
+static struct platform_driver rpmh_regulator_driver = {
+ .driver = {
+ .name = "qcom-rpmh-regulator",
+ .of_match_table = rpmh_regulator_match_table,
+ .owner = THIS_MODULE,
+ },
+ .probe = rpmh_regulator_probe,
+ .remove = rpmh_regulator_remove,
+};
+
+static int rpmh_regulator_init(void)
+{
+ return platform_driver_register(&rpmh_regulator_driver);
+}
+
+static void rpmh_regulator_exit(void)
+{
+ platform_driver_unregister(&rpmh_regulator_driver);
+}
+
+arch_initcall(rpmh_regulator_init);
+module_exit(rpmh_regulator_exit);
+
+MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
+MODULE_LICENSE("GPL v2");
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..f854e0e
--- /dev/null
+++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
@@ -0,0 +1,40 @@
+/* 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 qcom,allowed-modes and qcom,init-mode
+ * properties of an RPMh resource. Each type of regulator supports a subset of
+ * the possible modes.
+ *
+ * %RPMH_REGULATOR_MODE_PASS: Pass-through mode in which output is directly
+ * tied to input. This mode is only supported by
+ * BOB type regulators.
+ * %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_PASS 0
+#define RPMH_REGULATOR_MODE_RET 1
+#define RPMH_REGULATOR_MODE_LPM 2
+#define RPMH_REGULATOR_MODE_AUTO 3
+#define RPMH_REGULATOR_MODE_HPM 4
+
+#endif
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-03-18 20:40:33

by kernel test robot

[permalink] [raw]

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

Hi David,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180316]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/David-Collins/regulator-add-QCOM-RPMh-regulator-driver/20180319-011147
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All errors (new ones prefixed by >>):

>> drivers/regulator/qcom_rpmh-regulator.c:18:10: fatal error: soc/qcom/cmd-db.h: No such file or directory
#include <soc/qcom/cmd-db.h>
^~~~~~~~~~~~~~~~~~~
compilation terminated.

coccinelle warnings: (new ones prefixed by >>)

>> drivers/regulator/qcom_rpmh-regulator.c:1104:3-8: No need to set .owner here. The core will do it.

Please review and possibly fold the followup patch.

vim +18 drivers/regulator/qcom_rpmh-regulator.c

5
6 #include <linux/bitops.h>
7 #include <linux/err.h>
8 #include <linux/kernel.h>
9 #include <linux/module.h>
10 #include <linux/of.h>
11 #include <linux/of_device.h>
12 #include <linux/platform_device.h>
13 #include <linux/slab.h>
14 #include <linux/string.h>
15 #include <linux/regulator/driver.h>
16 #include <linux/regulator/machine.h>
17 #include <linux/regulator/of_regulator.h>
> 18 #include <soc/qcom/cmd-db.h>
19 #include <soc/qcom/rpmh.h>
20 #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
21

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (1.79 kB)
.config.gz (61.60 kB)
Download all attachments

2018-03-18 20:54:32

by kernel test robot

[permalink] [raw]

Subject: [PATCH] regulator: fix platform_no_drv_owner.cocci warnings

From: Fengguang Wu <[email protected]>

drivers/regulator/qcom_rpmh-regulator.c:1104:3-8: No need to set .owner here. The core will do it.

Remove .owner field if calls are used which set it automatically

Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci

Fixes: 835fda5b6c24 ("regulator: add QCOM RPMh regulator driver")
CC: David Collins <[email protected]>
Signed-off-by: Fengguang Wu <[email protected]>
---

qcom_rpmh-regulator.c | 1 -
1 file changed, 1 deletion(-)

--- a/drivers/regulator/qcom_rpmh-regulator.c
+++ b/drivers/regulator/qcom_rpmh-regulator.c
@@ -1101,7 +1101,6 @@ static struct platform_driver rpmh_regul
.driver = {
.name = "qcom-rpmh-regulator",
.of_match_table = rpmh_regulator_match_table,
- .owner = THIS_MODULE,
},
.probe = rpmh_regulator_probe,
.remove = rpmh_regulator_remove,

2018-03-19 21:49:33

[permalink] [raw]

Subject: Re: [PATCH] regulator: fix platform_no_drv_owner.cocci warnings

On 03/18/2018 01:38 PM, kbuild test robot wrote:
> From: Fengguang Wu <[email protected]>
>
> drivers/regulator/qcom_rpmh-regulator.c:1104:3-8:
> No need to set .owner here. The core will do it.

Thanks for pointing this out.

> --- a/drivers/regulator/qcom_rpmh-regulator.c
> +++ b/drivers/regulator/qcom_rpmh-regulator.c
> @@ -1101,7 +1101,6 @@ static struct platform_driver rpmh_regul
> .driver = {
> .name = "qcom-rpmh-regulator",
> .of_match_table = rpmh_regulator_match_table,
> - .owner = THIS_MODULE,
> },
> .probe = rpmh_regulator_probe,
> .remove = rpmh_regulator_remove,

I'll make this modification in [1] if I end up sending out a second
version of the patch.

Take care,
David

[1]: https://lkml.org/lkml/2018/3/16/1430

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

2018-03-21 02:17:32

[permalink] [raw]

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

Hi

On Fri, Mar 16, 2018 at 6:09 PM, David Collins <[email protected]> wrote:
> 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 <[email protected]>
> ---
> .../bindings/regulator/qcom,rpmh-regulator.txt | 246 +++++++++++++++++++++
> 1 file changed, 246 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
>
> 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..2d86306
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
> @@ -0,0 +1,246 @@
> +Qualcomm Technologies, Inc. RPMh Regulators
> +
> +rpmh-regulator devices support PMIC regulator management via the VRM and XOB
> +RPMh accelerators. The APPS processor communicates with these hardware blocks
> +via an 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.

Somewhere in here can you give some context of what VRM and XOB stand
for. From the other patch desc it's "voltage regulator manager (VRM)
and oscillator buffer (XOB)", but nice to sprinkle that around the
first time it's used in documents.

...and, ummmmm, what's an oscillator buffer? Is this really a
regulator? It sounds a lot more like a clock enable knob. Are you
sure this shouldn't be exposed through the common clock framework?
Many other PMICs expose oscillator clocks through CCF. MAX77686 comes
to mind.

> +- regulator-name
> + Usage: optional
> + Value type: <string>
> + Definition: Specifies the name for this RPMh regulator. If not
> + specified, then the regulator's name is equal to its subnode
> + name.

Probably don't need to include "regulator-name" since you say below
"Other properties defined in regulator.txt may also be used" and this
isn't anything special for your regulator.

> +- regulator-min-microvolt
> + Usage: required
> + Value type: <u32>
> + Definition: For VRM resources, this is the minimum supported voltage in
> + microvolts. For XOB resources, this is the fixed output
> + voltage.
> +
> +- regulator-max-microvolt
> + Usage: required
> + Value type: <u32>
> + Definition: For VRM resources, this is the maximum supported voltage in
> + microvolts. For XOB resources, this is the fixed output
> + voltage.

regulator-min-microvolt / regulator-max-microvolt are really required?
What happens if you leave them off? In general the regulator
framework supports this concept--it just lets you enable/disable
without changing voltage.

> +- qcom,regulator-initial-voltage
> + Usage: optional; VRM regulators only
> + Value type: <u32>
> + Definition: Specifies the initial voltage in microvolts to request for a
> + VRM regulator. Supported values are 0 to 8191000.

The "supported values" here is a strange statement to make. Not all
regulators will support all those voltages, right? Do you really need
to list this here?

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

Explicitly state whether this is allowed even if
"regulator-allow-set-load" is not set.

> +- regulator-allow-set-load
> + Usage: optional
> + Value type: <empty>
> + Definition: Boolean flag indicating that the the mode of this regulator
> + may be configured at runtime based upon consumer load needs.
> +
> +- qcom,allowed-modes

It would be up to Mark Brown, but my guess is that he will say "please
add this to the core". The regulator core already has the concept of
modes and you're already using the standard core concepts in most
places. Get rid of the special case code in your driver and add this
to the core.

> + 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 4). Elements must be specified in order
> + from lowest to highest value.

As far as I can tell this sorting constraint should be removed.
Really the constraints should be:

* qcom,mode-threshold-currents should be sorted from lowest to highest
* qcom,allowed-modes must match qcom,mode-threshold-currents

Sure, today it happens that the numbers are always lower for lower
currents, but it doesn't seem like something you'd have to encode into
the dt.

> +- qcom,headroom-voltage
> + 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.

Is this just "regulator-microvolt-offset", but supported in hardware?

> +- qcom,rpmh-resource-type
> + Usage: optional
> + Value type: <string>
> + Definition: RPMh accelerator type for this regulator. If not specified,
> + then the default type associated with this regulator will be
> + used. Supported values: "vrm" or "xob".

I still don't have the big picture I guess, but it seems weird that
you can override this. You're saying that someone might stick an
oscillator buffer the place of something that's normally a regulator?
...or a regulator in the place of something that's usually an
oscillator buffer? When would you use this?

-Doug

2018-03-21 02:17:52

[permalink] [raw]

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

Hi,

On Fri, Mar 16, 2018 at 6:09 PM, David Collins <[email protected]> wrote:
> +/**
> + * struct rpmh_regulator_mode - RPMh VRM mode attributes
> + * @pmic_mode: Raw PMIC mode value written into VRM mode voting
> + * register (i.e. RPMH_REGULATOR_MODE_*)
> + * @framework_mode: Regulator framework mode value
> + * (i.e. REGULATOR_MODE_*)
> + * @min_load_ua: The minimum load current in microamps which
> + * would utilize this mode

Thinking of this as "min load" seems backward to me. Shouldn't we be
keeping track of "max load". AKA:

Use "idle" if the load is less than 1000 mA
Use "normal" if the load is more than 1000 mA but less than 5000 mA
Use "fast" if the load is more than 5000 mA.

I'd think of "1000 mA as the max load that "idle" can handle". The
reason I don't think of it as "min" is that you can't say "1000 mA is
the min load the "normal" can handle". Normal can handle smaller
loads just fine, it's just not ideal.

Thinking of it in terms of "max" would also get rid of that weird
"needs to be 0" entry in the device tree too. You could either leave
the number off the top one (and set to INT_MAX in software?) or you
could use that to put in some sort of sane maximum current. I'd bet
there's something in the datasheet for this. If someone in software
got mixed up and kept requesting more and more current, this could
catch their error.

> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->enabled;

You can't read hardware? The regulator framework expects you to read
hardware. If it was common not to be able to read hardware then the
regulator framework would just keep track of the enabled state for
you. Right now if a regulator was left on by the BIOS (presumably
some have to be since otherwise you're running on a computer that
takes no power), you'll still return false at bootup? Seems
non-ideal.

> +}
> +
> +static int rpmh_regulator_enable(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
> + .data = RPMH_REGULATOR_ENABLE,
> + };
> + int ret;
> +
> + if (vreg->enabled)
> + return 0;

Does the "if" test above ever hit? I'd think the regulator framework
would handle this.

> +static int rpmh_regulator_disable(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
> + .data = RPMH_REGULATOR_DISABLE,
> + };
> + int ret;
> +
> + if (!vreg->enabled)
> + return 0;

Does the "if" test above ever hit? I'd think the regulator framework
would handle this.

> +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
> + int min_uv, int max_uv, unsigned int *selector)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> + };
> + const struct regulator_linear_range *range;
> + int mv, uv, ret;
> + bool wait_for_ack;
> +
> + mv = DIV_ROUND_UP(min_uv, 1000);
> + uv = mv * 1000;
> + if (uv > max_uv) {
> + vreg_err(vreg, "no set points available in range %d-%d uV\n",
> + min_uv, max_uv);
> + return -EINVAL;
> + }
> +
> + range = vreg->hw_data->voltage_range;
> + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step);

It seems like you should remove the existing check you have for "if
(uv > max_uv)" and replace with a check here. Specifically, it seems
like the DIV_ROUND_UP for the selector could also bump you over the
max. AKA:

...
bool wait_for_ack;

range = vreg->hw_data->voltage_range;
*selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step);
uv = *selector * range->uV_step + range->min_uV
if (uv > max_uv) {
...
}

Hold up, though. Why don't you implement set_voltage_sel() instead of
set_voltage()? That's what literally everyone else does, well except
PWM regulators. Using that will get rid of most of this code, won't
it? Even the check to see if perhaps the voltage isn't changing.

> +
> + if (uv == vreg->voltage)
> + return 0;
> +
> + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage;

Do you often see "wait_for_ack = false" in reality? Most regulator
usage I've seen requests a fairly tight range. AKA: I don't often
see:

set_voltage(min=3000mV, max=3300mV)
set_voltage(min=1800mV, max=3300mV)

Instead, I see:

set_voltage(min=3000mV, max=3300mV)
set_voltage(min=1800mV, max=1900mV)

So you'll always have wait_for_ack = true in the cases I've seen.

...but are you certain it's useful to wait for an ack anyway when the
voltage is falling? Most regulators won't guarantee that the voltage
has actually fallen even after they ack you. Specifically if a
regulator is under light load and it doesn't have an active discharge
circuit then the regulator might fall very slowly over time. As a
specific example, see commit 7c5209c315ea ("mmc: core: Increase delay
for voltage to stabilize from 3.3V to 1.8V").

That was a lot of words, so to sum it all up:

* If you have no actual examples where you see "wait_for_ack = false"
then remove this code and always wait.

* If you have evidence that the time spent waiting for the ack is
slowing you down, consider always setting wait_for_ack to false when
you're lowering the voltage. Anyone who truly cares could just set
something like the device tree property
regulator-settling-time-down-us. ...or, assuming Mark doesn't hate
it, they could set the always-wait-for-ack property in the device
tree.

NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy
ARC things for this?), we're probably talking about a small handful of
voltage transitions per boot, right?

> +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->voltage;

I guess there's no way to read the voltage at bootup? So this will
return 0 until someone sets it? ...maybe less of a big deal due to
the "qcom,regulator-initial-voltage" property?

> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
> + unsigned int mode)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> + };
> + int i, ret;
> +
> + if (mode == vreg->mode)
> + return 0;
> +
> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
> + if (vreg->hw_data->mode_map[i].framework_mode == mode)
> + break;
> + if (i >= RPMH_REGULATOR_MODE_COUNT) {
> + vreg_err(vreg, "invalid mode=%u\n", mode);
> + return -EINVAL;
> + }
> +
> + cmd.data = vreg->hw_data->mode_map[i].pmic_mode;
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
> + mode < vreg->mode || !vreg->mode);

Please explain the "mode < vreg->mode || !vreg->mode" test in words.

> +static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> + return vreg->mode;

You'll probably guess that I'd expect you to read this from hardware.

> +/**
> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
> + * for a VRM RPMh resource from device tree
> + * vreg: Pointer to the rpmh regulator resource
> + *
> + * This function initializes the mode[] array of vreg based upon the values
> + * of optional device tree properties.
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
> +{
> + struct device_node *node = vreg->of_node;
> + const struct rpmh_regulator_mode *map;
> + const char *prop;
> + int i, len, ret;
> + u32 *buf;
> +
> + map = vreg->hw_data->mode_map;
> + if (!map)
> + return 0;
> +
> + /* qcom,allowed-modes is optional */
> + prop = "qcom,allowed-modes";
> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
> + if (len < 0)
> + return 0;

Seems like it might be worth it to count
"qcom,mode-threshold-currents" too and confirm the count is the same?
If someone left in an extra threshold current you won't notice
otherwise (right?)

> +
> + vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len,
> + sizeof(*vreg->mode_map), GFP_KERNEL);

I keep getting myself confused because you have two things called
"mode_map" and they work differently:

vreg->mode_map - contains 1 element per allowed mode. Need to iterate
through this to map "framework mode" to "pmic mode". Note: because of
the need to iterate this isn't really a "map" in my mind.

vreg->hw_data->mode_map - contains 1 element for each possible "device
tree mode". Index into this using "device tree mode" and you get both
a "framework mode" and "pmic mode".

IMHO it would be better to have a table like "dt_to_linux_mode" that
was just a simple list:

static const int dt_to_linux_mode_bob[] = [
[RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY,
[RPMH_REGULATOR_MODE_RET] = -EINVAL,
[RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
[RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
[RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
];

static const int dt_to_linux_mode_ldo_smps[] =
[RPMH_REGULATOR_MODE_PASS] = -EINVAL,
[RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
[RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
[RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
[RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
];

You'd only use that in the "map_mode" functions and when parsing
qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could
actually just call the map_mode function. This would be especially a
good way to do this if you moved "allowed-modes" into the regulator
core, which seems like a good idea.

The nice thing about this is that only place you need to conceptually
keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing
code. Otherwise you just think of the Linux version.

---

Once you do the above, then your other list could just be named
"allowed_modes". This would make it obvious that this isn't a map
itself but that you could iterate over it to accomplish a mapping.

> +/**
> + * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated
> + * with the PMIC and initialize important pointers for each
> + * regulator
> + * @pmic: Pointer to the RPMh regulator PMIC
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic)
> +{
> + struct device_node *node;
> + int i;
> +
> + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node);
> + if (pmic->vreg_count == 0) {
> + dev_err(pmic->dev, "could not find any regulator subnodes\n");
> + return -ENODEV;
> + }
> +
> + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count,
> + sizeof(*pmic->vreg), GFP_KERNEL);
> + if (!pmic->vreg)
> + return -ENOMEM;
> +
> + i = 0;
> + for_each_available_child_of_node(pmic->dev->of_node, node) {
> + pmic->vreg[i].of_node = node;
> + pmic->vreg[i].pmic = pmic;
> +
> + i++;
> + }

While I can believe that things don't crash, you're not quite using
for_each_available_child_of_node() correctly. You need to reorganize
your code structure to fix. Specifically the "node" that's provided
to each iteration only has its refcount held for each iteration. By
the end of your function the refcount of all the "of_node"s that you
stored wil be 0. Doh.

You could try to fix this by adding a of_node_get() before storing the
node and that would work, but that would complicate your error
handling. You'll need to do this in a "cleanup" error path of probe
and in remove. :(

A better solution is to get rid of the rpmh_regulator_allocate_vreg()
function and move the functionality into probe. There, instead of
iterating over pmic->vreg_count, just use the
for_each_available_child_of_node() function. If
rpmh_regulator_init_vreg() you'll have to manually of_node_put() but
that should be OK.

Why will that work? You'll call rpmh_regulator_init_vreg() while the
reference count is still held. In that function you'll call
devm_regulator_register(), which will grab the refcount if it needs
it. Since that's a devm_ function you can be sure that it will
properly drop the refcount at the right times.

NOTE: with this you presumably should remove the "of_node" from the
"struct rpmh_vreg". It seems like it shouldn't be needed anymore and
it's good not to keep the pointer around if you didn't call
of_node_get() yourself.

> +/**
> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
> + * request for this regulator based on optional device tree
> + * properties
> + * @vreg: Pointer to the RPMh regulator
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
> +{
> + struct tcs_cmd cmd[2] = { };
> + const char *prop;
> + int cmd_count = 0;
> + int ret;
> + u32 temp;
> +
> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
> + prop = "qcom,headroom-voltage";
> + ret = of_property_read_u32(vreg->of_node, prop, &temp);
> + if (!ret) {
> + if (temp < RPMH_VRM_HEADROOM_MIN_UV ||
> + temp > RPMH_VRM_HEADROOM_MAX_UV) {
> + vreg_err(vreg, "%s=%u is invalid\n",
> + prop, temp);
> + return -EINVAL;
> + }
> + vreg->headroom_voltage = temp;
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
> + cmd[cmd_count++].data
> + = DIV_ROUND_UP(vreg->headroom_voltage, 1000);
> + }
> +
> + prop = "qcom,regulator-initial-voltage";
> + ret = of_property_read_u32(vreg->of_node, prop, &temp);
> + if (!ret) {
> + if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) {
> + vreg_err(vreg, "%s=%u is invalid\n",
> + prop, temp);
> + return -EINVAL;
> + }
> + vreg->voltage = temp;
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
> + cmd[cmd_count++].data
> + = DIV_ROUND_UP(vreg->voltage, 1000);

It seems like you should set vreg->voltage to the actual result of the
division * 1000. AKA: if the user said the initial voltage was
123,456 then vreg->voltage should become 124,000.

Actually, shouldn't you somehow resolve this with "struct
rpmh_vreg_hw_data". It seems like some regulators have 128 steps,
some have 256 steps, etc. You should have a function that reconciles
the requested voltage with the one that hardware will actually
provide.

...and, actually, you should share code for this reconciliation with
rpmh_regulator_vrm_set_voltage().

> +/**
> + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator
> + * @vreg: Pointer to the RPMh regulator
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg)
> +{
> + struct device *dev = vreg->pmic->dev;
> + struct regulator_config reg_config = {};
> + const struct rpmh_vreg_init_data *rpmh_data = NULL;
> + const char *type_name = NULL;
> + enum rpmh_regulator_type type;
> + struct regulator_init_data *init_data;
> + int ret, i;
> +
> + for (i = 0; i < vreg->pmic->init_data->count; i++) {
> + if (!strcmp(vreg->pmic->init_data->vreg_data[i].name,
> + vreg->of_node->name)) {
> + rpmh_data = &vreg->pmic->init_data->vreg_data[i];
> + break;
> + }
> + }
> +
> + if (!rpmh_data) {
> + dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n",
> + vreg->of_node->name, vreg->pmic->init_data->name);
> + return -EINVAL;
> + }
> +
> + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d",
> + rpmh_data->resource_name_base, vreg->pmic->pmic_id,
> + rpmh_data->id);
> + if (!vreg->resource_name)
> + return -ENOMEM;
> +
> + vreg->addr = cmd_db_read_addr(vreg->resource_name);
> + if (!vreg->addr) {
> + vreg_err(vreg, "could not find RPMh address for resource %s\n",
> + vreg->resource_name);
> + return -ENODEV;
> + }
> +
> + vreg->rdesc.name = rpmh_data->name;
> + vreg->rdesc.supply_name = rpmh_data->supply_name;
> + vreg->regulator_type = rpmh_data->regulator_type;
> + vreg->hw_data = rpmh_data->hw_data;
> +
> + if (rpmh_data->hw_data->voltage_range) {
> + vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range;
> + vreg->rdesc.n_linear_ranges = 1;
> + vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages;
> + }
> +
> + /* Optional override for the default RPMh accelerator type */
> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",
> + &type_name);
> + if (!ret) {
> + if (!strcmp("vrm", type_name)) {
> + vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM;
> + } else if (!strcmp("xob", type_name)) {
> + vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB;
> + } else {
> + vreg_err(vreg, "Unknown RPMh accelerator type %s\n",
> + type_name);
> + return -EINVAL;
> + }

As per comment in device tree patch, it seems really weird that you
can override this. Are you sure?

> +static const struct rpmh_regulator_mode
> +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = {
> + [RPMH_REGULATOR_MODE_PASS] = {
> + .pmic_mode = 0,
> + .framework_mode = REGULATOR_MODE_STANDBY,

Is "PASS" truly the same concept as the Linux concept of STANDBY. If
so, why do you need a separate define for it?

If it truly is the same, it seems like you can simplify everything by
just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and
"RPMH_REGULATOR_MODE_PASS" and just call it
"RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that
"standby" maps to "retention" for some regulators and maps to "pass"
for other regulators if you want to map PMIC documentation. ...but
getting rid of this distinction simply means less error checking and
fewer tables in Linux.

If "pass" really shouldn't map to "standby" then this seems like a
hack and you should add the concept of "pass" to the core regulator
framework.

> +static const struct rpmh_vreg_hw_data pmic4_pldo_hw_data = {
> + .voltage_range = &(const struct regulator_linear_range)
> + REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),

This seems pretty iffy to me. You're relying on the compiler to make
an anonymous chunk of memory with a "struct regulator_linear_range" in
it and then storing a pointer to said anonymous chunk of memory?
Nobody else using REGULATOR_LINEAR_RANGE() does this.

Why not just get rid of the pointer and put the structure right inside
"struct rpmh_vreg_hw_data"? It'll get rid of the weird cast / strange
anonymous chunk, save an indirection, and save 4 bytes for a pointer.

> +/**
> + * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each
> + * of the regulator nodes associated with it
> + * @pdev: Pointer to the platform device of the RPMh PMIC
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct of_device_id *match;
> + struct rpmh_pmic *pmic;
> + struct device_node *node;
> + int ret, i;
> +
> + node = dev->of_node;
> +
> + if (!node) {
> + dev_err(dev, "Device tree node is missing\n");
> + return -EINVAL;
> + }
> +
> + ret = cmd_db_ready();
> + if (ret < 0) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
> + return ret;
> + }
> +
> + pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> + if (!pmic)
> + return -ENOMEM;
> +
> + pmic->dev = dev;
> + platform_set_drvdata(pdev, pmic);
> +
> + pmic->rpmh_client = rpmh_get_client(pdev);

It seems like you'd do yourself (and the other clients of rpmh) a
favor if you added a devm_rpmh_get_client() in a patch before this
one. Adding a devm version of a calls is pretty easy and you'll be
able to completely get rid of your "remove" function. ...and get rid
of the "cleanup" exit here.

> +static struct platform_driver rpmh_regulator_driver = {
> + .driver = {
> + .name = "qcom-rpmh-regulator",
> + .of_match_table = rpmh_regulator_match_table,
> + .owner = THIS_MODULE,

As per the robot, no need to set .owner here. The core will do it.

> + },
> + .probe = rpmh_regulator_probe,
> + .remove = rpmh_regulator_remove,
> +};
> +
> +static int rpmh_regulator_init(void)
> +{
> + return platform_driver_register(&rpmh_regulator_driver);
> +}
> +
> +static void rpmh_regulator_exit(void)
> +{
> + platform_driver_unregister(&rpmh_regulator_driver);
> +}
> +
> +arch_initcall(rpmh_regulator_init);

I always get yelled at when I try to use arch_initcall() for stuff
like this. You should do what everyone else does and use
module_platform_driver() to declare your driver. Yeah, regulators are
important and (as I remember) they get probed slightly early anyway,
but everything else in the system just gotta deal with the fact that
they'll sometimes get deferred probes.

> +module_exit(rpmh_regulator_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
> +MODULE_LICENSE("GPL v2");
> 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..f854e0e
> --- /dev/null
> +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h

Device tree guys will yell at you here. The "include/dt-bindings"
bits are supposed to be together with the bindings. Different
maintainers have different beliefs here, but I think the way that's
least likely to get you yelled at by the most people is:

Patch #1: bindings (.txt and include file)
Patch #2: the driver

...with the idea being that if another operating system wanted just
the bindings they could get it all in one patch.

> @@ -0,0 +1,40 @@
> +/* 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 qcom,allowed-modes and qcom,init-mode

Not "qcom,init-mode". This is actually "regulator-initial-mode" now.

> + * properties of an RPMh resource. Each type of regulator supports a subset of
> + * the possible modes.
> + *
> + * %RPMH_REGULATOR_MODE_PASS: Pass-through mode in which output is directly
> + * tied to input. This mode is only supported by
> + * BOB type regulators.
> + * %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_PASS 0
> +#define RPMH_REGULATOR_MODE_RET 1
> +#define RPMH_REGULATOR_MODE_LPM 2
> +#define RPMH_REGULATOR_MODE_AUTO 3
> +#define RPMH_REGULATOR_MODE_HPM 4
> +
> +#endif

OK, I think that's enough for one review session. Looking forward to
seeing responses / the next version. Let me know if anything I said
looks wrong or if you have questions! :)

-Doug

2018-03-21 02:44:37

[permalink] [raw]

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

On Fri, Mar 16, 2018 at 06:09:11PM -0700, David Collins wrote:
> Introduce bindings for RPMh regulator devices found on some
> Qualcomm Technlogies, Inc. SoCs. These devices allow a given

Please use subject lines matching the style for the subsystem. This
makes it easier for people to identify relevant patches.

> +=========================================
> +Second Level Nodes - Regulators
> +=========================================
> +
> +- regulator-name

This document appears to be reproducing the standard regulator bindings;
there is no need to do that, just reference them.

Attachments:

(No filename) (605.00 B)
signature.asc (499.00 B)
Download all attachments

2018-03-21 19:09:43

by Stephen Boyd

[permalink] [raw]

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

Quoting David Collins (2018-03-16 18:09:10)
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 097f617..e0ecd0a 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
> Qualcomm RPM as a module. The module will be named
> "qcom_rpm-regulator".
>
> +config REGULATOR_QCOM_RPMH
> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST

What's the build dependency on OF?

> + help
> + This driver supports control of PMIC regulators via the RPMh hardware
> + block found on Qualcomm Technologies Inc. SoCs. RPMh regulator
> + control allows for voting on regulator state between multiple
> + processors within the SoC.
> +
> config REGULATOR_QCOM_SMD_RPM
> tristate "Qualcomm SMD based RPM regulator driver"
> depends on QCOM_SMD_RPM
> diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c
> new file mode 100644
> index 0000000..808f949
> --- /dev/null
> +++ b/drivers/regulator/qcom_rpmh-regulator.c
> @@ -0,0 +1,1124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>

Including machine is usually a red flag in regulator drivers.

> +#include <linux/regulator/of_regulator.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> +
> +/**
> + * enum rpmh_regulator_type - supported RPMh accelerator types
> + * %RPMH_REGULATOR_TYPE_VRM: RPMh VRM accelerator which supports voting on
> + * enable, voltage, mode, and headroom voltage of
> + * LDO, SMPS, VS, and BOB type PMIC regulators.
> + * %RPMH_REGULATOR_TYPE_XOB: RPMh XOB accelerator which supports voting on
> + * the enable state of PMIC regulators.
> + */
> +enum rpmh_regulator_type {
> + RPMH_REGULATOR_TYPE_VRM,
> + RPMH_REGULATOR_TYPE_XOB,
> +};
> +
> +/* Min and max limits of VRM resource request parameters */
> +#define RPMH_VRM_MIN_UV 0
> +#define RPMH_VRM_MAX_UV 8191000
> +
> +#define RPMH_VRM_HEADROOM_MIN_UV 0
> +#define RPMH_VRM_HEADROOM_MAX_UV 511000
> +
> +#define RPMH_VRM_MODE_MIN 0
> +#define RPMH_VRM_MODE_MAX 7
> +
> +/* Register offsets: */

Why the colon? ^
Why even have the comment?

> +#define RPMH_REGULATOR_REG_VRM_VOLTAGE 0x0
> +#define RPMH_REGULATOR_REG_ENABLE 0x4
> +#define RPMH_REGULATOR_REG_VRM_MODE 0x8
> +#define RPMH_REGULATOR_REG_VRM_HEADROOM 0xC
> +
> +/* Enable register values: */
> +#define RPMH_REGULATOR_DISABLE 0x0
> +#define RPMH_REGULATOR_ENABLE 0x1
> +
> +/* Number of unique hardware modes supported: */

Both above also look useless.

> +#define RPMH_REGULATOR_MODE_COUNT 5
> +
> +/**
> + * struct rpmh_regulator_mode - RPMh VRM mode attributes
> + * @pmic_mode: Raw PMIC mode value written into VRM mode voting
> + * register (i.e. RPMH_REGULATOR_MODE_*)
> + * @framework_mode: Regulator framework mode value
> + * (i.e. REGULATOR_MODE_*)
> + * @min_load_ua: The minimum load current in microamps which
> + * would utilize this mode
> + *
> + * Software selects the lowest mode for which aggr_load_ua >= min_load_ua.
> + */
> +struct rpmh_regulator_mode {
> + u32 pmic_mode;
> + u32 framework_mode;
> + int min_load_ua;
> +};
> +
> +/**
> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
> + * @mode_map: Array of size RPMH_REGULATOR_MODE_COUNT which
> + * maps RPMH_REGULATOR_MODE_* indices into PMIC
> + * mode and regulator framework mode that are
> + * supported by this PMIC regulator type
> + * @voltage_range: The single range of voltages supported by this
> + * PMIC regulator type
> + * @n_voltages: The number of unique voltage set points defined
> + * by voltage_range
> + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
> + * in device tree to a regulator framework mode
> + */
> +struct rpmh_vreg_hw_data {
> + const struct rpmh_regulator_mode *mode_map;
> + const struct regulator_linear_range *voltage_range;
> + int n_voltages;
> + unsigned int (*of_map_mode)(unsigned int mode);
> +};
> +
> +struct rpmh_pmic;
> +
> +/**
> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
> + * single regulator device
> + * @of_node: Device tree node pointer of the regulator
> + * @pmic: Pointer to the PMIC containing the regulator
> + * @resource_name: Name of the RPMh regulator resource which is
> + * mapped to an RPMh accelerator address via
> + * command DB. This name must match to one that is
> + * defined by the bootloader.
> + * @addr: Base address of the regulator resource within
> + * an RPMh accelerator
> + * @rdesc: Regulator descriptor
> + * @rdev: Regulator device pointer returned by
> + * devm_regulator_register()

Is this used? Why save it around?

> + * @hw_data: PMIC regulator configuration data for this RPMh
> + * regulator
> + * @regulator_type: RPMh accelerator type for this regulator
> + * resource
> + * @always_wait_for_ack: Boolean flag indicating if a request must always
> + * wait for an ACK from RPMh before continuing even
> + * if it corresponds to a strictly lower power
> + * state (e.g. enabled --> disabled).
> + * @mode_map: An array of modes which may be configured at
> + * runtime by setting the load current
> + * @mode_count: The number of entries in the mode_map array.
> + * @enabled: Boolean indicating if the regulator is enabled
> + * or not
> + * @voltage: RPMh VRM regulator voltage in microvolts

So call it uV?

> + * @mode: RPMh VRM regulator current framework mode
> + * @headroom_voltage: RPMh VRM regulator minimum headroom voltage
> + * required

headroom_uV?

> + */
> +struct rpmh_vreg {
> + struct device_node *of_node;
> + struct rpmh_pmic *pmic;
> + const char *resource_name;
> + u32 addr;
> + struct regulator_desc rdesc;
> + struct regulator_dev *rdev;
> + const struct rpmh_vreg_hw_data *hw_data;
> + enum rpmh_regulator_type regulator_type;
> + bool always_wait_for_ack;
> + struct rpmh_regulator_mode *mode_map;
> + int mode_count;

size_t?

> +
> + bool enabled;
> + int voltage;
> + unsigned int mode;
> + int headroom_voltage;

Please try to limit the things that are assigned into these structs and
then never used outside of init. It adds complexity to the code when a
local variable in the function would work just as well.

> +};
> +
> +/**
> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
> + * @name: Name for the regulator which also corresponds
> + * to the device tree subnode name of the regulator
> + * @resource_name_base: RPMh regulator resource name prefix. E.g.
> + * "ldo" for RPMh resource "ldoa1".

Maybe it should be "ldo%c1"? Then we could kasprintf the name with the
pmic_id and drop the 'id' member entirely.

> + * @supply_name: Parent supply regulator name
> + * @id: Regulator number within the PMIC
> + * @regulator_type: RPMh accelerator type used to manage this
> + * regulator
> + * @hw_data: Configuration data for this PMIC regulator type
> + */
> +struct rpmh_vreg_init_data {
> + const char *name;
> + const char *resource_name_base;
> + const char *supply_name;
> + int id;
> + enum rpmh_regulator_type regulator_type;
> + const struct rpmh_vreg_hw_data *hw_data;
> +};
> +
> +/**
> + * struct rpmh_pmic_init_data - initialization data for a PMIC
> + * @name: PMIC name
> + * @vreg_data: Array of data for each regulator in the PMIC
> + * @count: Number of entries in vreg_data
> + */
> +struct rpmh_pmic_init_data {
> + const char *name;
> + const struct rpmh_vreg_init_data *vreg_data;
> + int count;
> +};
> +
> +/**
> + * struct rpmh_pmic - top level data structure of all regulators found on a PMIC
> + * @dev: Device pointer of the PMIC device for the
> + * regulators
> + * @rpmh_client: Handle used for rpmh communications
> + * @vreg: Array of rpmh regulator structs representing the
> + * individual regulators found on this PMIC chip
> + * which are configured via device tree.
> + * @vreg_count: The number of entries in the vreg array.
> + * @pmic_id: Letter used to identify this PMIC within the
> + * system. This is dictated by boot loader
> + * specifications on a given target.
> + * @init_data: Pointer to the matched PMIC initialization data
> + */
> +struct rpmh_pmic {
> + struct device *dev;
> + struct rpmh_client *rpmh_client;
> + struct rpmh_vreg *vreg;

It's a circle! Life is a circle! It's a circle!

> + int vreg_count;
> + const char *pmic_id;
> + const struct rpmh_pmic_init_data *init_data;

Hopefully we don't really need this entire struct and we can just use
local variables instead.

> +};
> +
> +#define vreg_err(vreg, message, ...) \
> + pr_err("%s %s: " message, (vreg)->pmic->init_data->name, \
> + (vreg)->rdesc.name, ##__VA_ARGS__)
> +#define vreg_info(vreg, message, ...) \
> + pr_info("%s %s: " message, (vreg)->pmic->init_data->name, \
> + (vreg)->rdesc.name, ##__VA_ARGS__)
> +#define vreg_debug(vreg, message, ...) \
> + pr_debug("%s %s: " message, (vreg)->pmic->init_data->name, \
> + (vreg)->rdesc.name, ##__VA_ARGS__)
> +
> +/**
> + * rpmh_regulator_send_request() - send the request to RPMh
> + * @vreg: Pointer to the RPMh regulator
> + * @cmd: RPMh commands to send
> + * @count: Size of cmd array
> + * @wait_for_ack: Boolean indicating if execution must wait until the
> + * request has been acknowledged as complete
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
> + struct tcs_cmd *cmd, int count, bool wait_for_ack)
> +{
> + int ret;
> +
> + if (wait_for_ack || vreg->always_wait_for_ack)
> + ret = rpmh_write(vreg->pmic->rpmh_client,
> + RPMH_ACTIVE_ONLY_STATE, cmd, count);
> + else
> + ret = rpmh_write_async(vreg->pmic->rpmh_client,
> + RPMH_ACTIVE_ONLY_STATE, cmd, count);
> + if (ret < 0)
> + vreg_err(vreg, "rpmh_write() failed, ret=%d\n", ret);
> +
> + return ret;
> +}
> +
[...]
> +static int rpmh_regulator_enable(struct regulator_dev *rdev)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + struct tcs_cmd cmd = {
> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
> + .data = RPMH_REGULATOR_ENABLE,
> + };
> + int ret;
> +
> + if (vreg->enabled)
> + return 0;
> +
> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
> + if (ret < 0) {
> + vreg_err(vreg, "enable failed, ret=%d\n", ret);

Do we really need all these error messages deep down in the regulator
drivers? If consumers care, they'll print some error message themselves
anyway.

> + return ret;
> + }
> +
> + vreg->enabled = true;
> +
> + return 0;
> +}
[...]
> +
> +/**
> + * rpmh_regulator_vrm_set_load() - set the PMIC mode based upon the maximum load
> + * required from the VRM rpmh-regulator
> + * @rdev: Regulator device pointer for the rpmh-regulator
> + * @load_ua: Maximum current required from all consumers in microamps
> + *
> + * This function is passed as a callback function into the regulator ops that
> + * are registered for each VRM rpmh-regulator device.
> + *
> + * This function sets the mode of the regulator to that which has the highest
> + * min support load less than or equal to load_ua. Example:

s/support/supported/

> + * mode_count = 3
> + * mode_map[].min_load_ua = 0, 100000, 6000000
> + *
> + * load_ua = 10000 --> i = 0
> + * load_ua = 250000 --> i = 1
> + * load_ua = 7000000 --> i = 2
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_ua)
> +{
> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> + int i;
> +
> + /* No need to check element 0 as it will be the default. */
> + for (i = vreg->mode_count - 1; i > 0; i--)
> + if (vreg->mode_map[i].min_load_ua <= load_ua)
> + break;
> +
> + return rpmh_regulator_vrm_set_mode(rdev,
> + vreg->mode_map[i].framework_mode);
> +}
> +
> +static const struct regulator_ops rpmh_regulator_vrm_ops = {
> + .enable = rpmh_regulator_enable,
> + .disable = rpmh_regulator_disable,
> + .is_enabled = rpmh_regulator_is_enabled,
> + .set_voltage = rpmh_regulator_vrm_set_voltage,
> + .get_voltage = rpmh_regulator_vrm_get_voltage,
> + .list_voltage = regulator_list_voltage_linear_range,
> + .set_mode = rpmh_regulator_vrm_set_mode,
> + .get_mode = rpmh_regulator_vrm_get_mode,
> + .set_load = rpmh_regulator_vrm_set_load,
> +};
> +
> +static const struct regulator_ops rpmh_regulator_xob_ops = {
> + .enable = rpmh_regulator_enable,
> + .disable = rpmh_regulator_disable,
> + .is_enabled = rpmh_regulator_is_enabled,
> +};
> +
> +static const struct regulator_ops *rpmh_regulator_ops[] = {
> + [RPMH_REGULATOR_TYPE_VRM] = &rpmh_regulator_vrm_ops,
> + [RPMH_REGULATOR_TYPE_XOB] = &rpmh_regulator_xob_ops,
> +};
> +
> +/**
> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
> + * for a VRM RPMh resource from device tree
> + * vreg: Pointer to the rpmh regulator resource
> + *
> + * This function initializes the mode[] array of vreg based upon the values
> + * of optional device tree properties.
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
> +{

I have a feeling this should all come from the driver data, not DT.
Doubtful this really changes for each board.

> + struct device_node *node = vreg->of_node;
> + const struct rpmh_regulator_mode *map;
> + const char *prop;
> + int i, len, ret;
> + u32 *buf;
> +
> + map = vreg->hw_data->mode_map;
> + if (!map)
> + return 0;
> +
> + /* qcom,allowed-modes is optional */
> + prop = "qcom,allowed-modes";
> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
> + if (len < 0)
> + return 0;
> +
> + vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len,
> + sizeof(*vreg->mode_map), GFP_KERNEL);
> + if (!vreg->mode_map)
> + return -ENOMEM;
> + vreg->mode_count = len;
> +
> + buf = kcalloc(len, sizeof(*buf), GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32_array(node, prop, buf, len);
> + if (ret < 0) {
> + vreg_err(vreg, "unable to read %s, ret=%d\n",
> + prop, ret);
> + goto done;
> + }
> +
> + for (i = 0; i < len; i++) {
> + if (buf[i] >= RPMH_REGULATOR_MODE_COUNT
> + || !map[buf[i]].framework_mode) {
> + vreg_err(vreg, "element %d of %s = %u is invalid for this regulator\n",
> + i, prop, buf[i]);
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + vreg->mode_map[i].pmic_mode = map[buf[i]].pmic_mode;
> + vreg->mode_map[i].framework_mode = map[buf[i]].framework_mode;
> +
> + if (i > 0 && vreg->mode_map[i].pmic_mode
> + <= vreg->mode_map[i - 1].pmic_mode) {
> + vreg_err(vreg, "%s elements are not in ascending order\n",
> + prop);
> + ret = -EINVAL;
> + goto done;
> + }
> + }
> +
> + prop = "qcom,mode-threshold-currents";
> + ret = of_property_read_u32_array(node, prop, buf, len);
> + if (ret < 0) {
> + vreg_err(vreg, "unable to read %s, ret=%d\n",
> + prop, ret);
> + goto done;
> + }
> +
> + for (i = 0; i < len; i++) {
> + vreg->mode_map[i].min_load_ua = buf[i];
> +
> + if (i > 0 && vreg->mode_map[i].min_load_ua
> + <= vreg->mode_map[i - 1].min_load_ua) {
> + vreg_err(vreg, "%s elements are not in ascending order\n",
> + prop);
> + ret = -EINVAL;
> + goto done;
> + }
> + }
> +
> +done:
> + kfree(buf);
> + return ret;
> +}
> +
> +/**
> + * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated
> + * with the PMIC and initialize important pointers for each
> + * regulator
> + * @pmic: Pointer to the RPMh regulator PMIC
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic)
> +{
> + struct device_node *node;
> + int i;
> +
> + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node);
> + if (pmic->vreg_count == 0) {
> + dev_err(pmic->dev, "could not find any regulator subnodes\n");
> + return -ENODEV;
> + }
> +
> + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count,
> + sizeof(*pmic->vreg), GFP_KERNEL);
> + if (!pmic->vreg)
> + return -ENOMEM;

Please just allocate one at a time. It's not like we have thousands of
these things to worry about.

> +
> + i = 0;
> + for_each_available_child_of_node(pmic->dev->of_node, node) {
> + pmic->vreg[i].of_node = node;
> + pmic->vreg[i].pmic = pmic;
> +
> + i++;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
> + * request for this regulator based on optional device tree
> + * properties
> + * @vreg: Pointer to the RPMh regulator
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
> +{
> + struct tcs_cmd cmd[2] = { };
> + const char *prop;
> + int cmd_count = 0;
> + int ret;
> + u32 temp;
> +
> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
> + prop = "qcom,headroom-voltage";

Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh
registers. This probably needs to be pushed into the framework and come
down through a 'set_headroom' op in the regulator ops via a
regulator-headroom-microvolt property that's parsed in of_regulator.c.

> + ret = of_property_read_u32(vreg->of_node, prop, &temp);
> + if (!ret) {
> + if (temp < RPMH_VRM_HEADROOM_MIN_UV ||
> + temp > RPMH_VRM_HEADROOM_MAX_UV) {
> + vreg_err(vreg, "%s=%u is invalid\n",
> + prop, temp);
> + return -EINVAL;
> + }
> + vreg->headroom_voltage = temp;
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
> + cmd[cmd_count++].data
> + = DIV_ROUND_UP(vreg->headroom_voltage, 1000);
> + }
> +
> + prop = "qcom,regulator-initial-voltage";

DT constraints should take care of this by setting voltages on all
regulators that need them?

> + ret = of_property_read_u32(vreg->of_node, prop, &temp);
> + if (!ret) {
> + if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) {
> + vreg_err(vreg, "%s=%u is invalid\n",
> + prop, temp);
> + return -EINVAL;
> + }
> + vreg->voltage = temp;
> +
> + cmd[cmd_count].addr
> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
> + cmd[cmd_count++].data
> + = DIV_ROUND_UP(vreg->voltage, 1000);
> + }
> + }
> +
> + if (cmd_count) {
> + ret = rpmh_regulator_send_request(vreg, cmd, cmd_count, true);
> + if (ret < 0) {
> + vreg_err(vreg, "could not send default config, ret=%d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator

Heh, abbributes.

> + * @vreg: Pointer to the RPMh regulator
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg)
> +{
> + struct device *dev = vreg->pmic->dev;
> + struct regulator_config reg_config = {};
> + const struct rpmh_vreg_init_data *rpmh_data = NULL;
> + const char *type_name = NULL;
> + enum rpmh_regulator_type type;
> + struct regulator_init_data *init_data;
> + int ret, i;
> +
> + for (i = 0; i < vreg->pmic->init_data->count; i++) {
> + if (!strcmp(vreg->pmic->init_data->vreg_data[i].name,
> + vreg->of_node->name)) {
> + rpmh_data = &vreg->pmic->init_data->vreg_data[i];
> + break;
> + }
> + }
> +
> + if (!rpmh_data) {
> + dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n",
> + vreg->of_node->name, vreg->pmic->init_data->name);
> + return -EINVAL;
> + }
> +
> + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d",
> + rpmh_data->resource_name_base, vreg->pmic->pmic_id,
> + rpmh_data->id);
> + if (!vreg->resource_name)
> + return -ENOMEM;

This isn't used outside of this function, so remove the
vreg::resource_name member and use a local variable that gets freed
on exit please.

> +
> + vreg->addr = cmd_db_read_addr(vreg->resource_name);
> + if (!vreg->addr) {
> + vreg_err(vreg, "could not find RPMh address for resource %s\n",
> + vreg->resource_name);
> + return -ENODEV;
> + }
> +
> + vreg->rdesc.name = rpmh_data->name;
> + vreg->rdesc.supply_name = rpmh_data->supply_name;
> + vreg->regulator_type = rpmh_data->regulator_type;
> + vreg->hw_data = rpmh_data->hw_data;
> +
> + if (rpmh_data->hw_data->voltage_range) {
> + vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range;
> + vreg->rdesc.n_linear_ranges = 1;
> + vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages;
> + }
> +
> + /* Optional override for the default RPMh accelerator type */
> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",

Can this property have override in the name? And then because it is
called override, perhaps it should come from the driver instead of DT
because DT may need an override itself.

Also, is this currently being used? If not I'd prefer we drop this until we
need it.

> + &type_name);
> + if (!ret) {
> + if (!strcmp("vrm", type_name)) {
> + vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM;
> + } else if (!strcmp("xob", type_name)) {
> + vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB;
> + } else {
> + vreg_err(vreg, "Unknown RPMh accelerator type %s\n",
> + type_name);
> + return -EINVAL;
> + }
> + }
> +
> + type = vreg->regulator_type;
> +
> + if (type == RPMH_REGULATOR_TYPE_VRM) {
> + ret = rpmh_regulator_parse_vrm_modes(vreg);
> + if (ret < 0) {
> + vreg_err(vreg, "could not parse vrm mode mapping, ret=%d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + vreg->always_wait_for_ack = of_property_read_bool(vreg->of_node,
> + "qcom,always-wait-for-ack");
> +
> + vreg->rdesc.owner = THIS_MODULE;
> + vreg->rdesc.type = REGULATOR_VOLTAGE;
> + vreg->rdesc.ops = rpmh_regulator_ops[type];
> + vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode;
> +
> + init_data = of_get_regulator_init_data(dev, vreg->of_node,
> + &vreg->rdesc);
> + if (!init_data)
> + return -ENOMEM;
> +
> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) {
> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
> + init_data->constraints.apply_uV = 0;
> + vreg->rdesc.n_voltages = 1;
> + }

What is this doing? Usually constraints aren't touched by the driver.

> +
> + if (vreg->hw_data->mode_map) {
> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;

Huh, I thought this was assigned by the framework.

> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
> + init_data->constraints.valid_modes_mask
> + |= vreg->hw_data->mode_map[i].framework_mode;
> + }
> +
> + reg_config.dev = dev;
> + reg_config.init_data = init_data;
> + reg_config.of_node = vreg->of_node;
> + reg_config.driver_data = vreg;
> +
> + ret = rpmh_regulator_load_default_parameters(vreg);
> + if (ret < 0) {
> + vreg_err(vreg, "unable to load default parameters, ret=%d\n",
> + ret);
> + return ret;
> + }
> +
> + vreg->rdev = devm_regulator_register(dev, &vreg->rdesc, &reg_config);
> + if (IS_ERR(vreg->rdev)) {
> + ret = PTR_ERR(vreg->rdev);
> + vreg->rdev = NULL;
> + vreg_err(vreg, "devm_regulator_register() failed, ret=%d\n",
> + ret);
> + return ret;
> + }
> +
> + vreg_debug(vreg, "registered RPMh resource %s @ 0x%05X\n",
> + vreg->resource_name, vreg->addr);
> +
> + return ret;
> +}
> +
[...]
> +
> +static const struct rpmh_pmic_init_data pm8005_pmic_data = {
> + .name = "PM8005",
> + .vreg_data = pm8005_vreg_data,
> + .count = ARRAY_SIZE(pm8005_vreg_data),
> +};

Kill 'name' please, and then get rid of 'count' and NUL terminate the
array instead. This follows previous rpm regulator driver styles. Also,
drop the macro that does stringification. We should end up with the
match_table pointing to static arrays of structs that look like:

{ "s1", VRM, "smp", 1, &pmic4_ftsmps42, "vdd_s1", },

And yes, drop the RPMH_REGULATOR_TYPE_ prefix and _hw_data postfix.

> +
> +static const struct of_device_id rpmh_regulator_match_table[] = {
> + {
> + .compatible = "qcom,pm8998-rpmh-regulators",
> + .data = &pm8998_pmic_data,
> + },
> + {
> + .compatible = "qcom,pmi8998-rpmh-regulators",
> + .data = &pmi8998_pmic_data,
> + },
> + {
> + .compatible = "qcom,pm8005-rpmh-regulators",
> + .data = &pm8005_pmic_data,
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table);

Please move this array next to the driver structure.

> +
> +/**
> + * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each
> + * of the regulator nodes associated with it
> + * @pdev: Pointer to the platform device of the RPMh PMIC
> + *
> + * Return: 0 on success, errno on failure
> + */
> +static int rpmh_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct of_device_id *match;
> + struct rpmh_pmic *pmic;
> + struct device_node *node;
> + int ret, i;
> +
> + node = dev->of_node;
> +
> + if (!node) {
> + dev_err(dev, "Device tree node is missing\n");
> + return -EINVAL;
> + }

This should never happen. Please remove.

> +
> + ret = cmd_db_ready();
> + if (ret < 0) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
> + return ret;
> + }

We should just make rpmh parent device call cmd_db_ready() so that these
devices aren't even populated until then and so that cmd_db_ready() is
only in one place. Lina?

> +
> + pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> + if (!pmic)
> + return -ENOMEM;
> +
> + pmic->dev = dev;
> + platform_set_drvdata(pdev, pmic);
> +
> + pmic->rpmh_client = rpmh_get_client(pdev);
> + if (IS_ERR(pmic->rpmh_client)) {
> + ret = PTR_ERR(pmic->rpmh_client);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to request RPMh client, ret=%d\n",
> + ret);

I fail to see how this happens given that rpmh creates this device.

> + return ret;
> + }
> +
> + match = of_match_node(rpmh_regulator_match_table, node);
> + if (match) {
> + pmic->init_data = match->data;
> + } else {
> + dev_err(dev, "could not find compatible string match\n");
> + ret = -ENODEV;
> + goto cleanup;
> + }

Use of_device_get_match_data() instead and print nothing on error.

> +
> + ret = of_property_read_string(node, "qcom,pmic-id",
> + &pmic->pmic_id);
> + if (ret < 0) {
> + dev_err(dev, "qcom,pmic-id missing in DT node\n");
> + goto cleanup;
> + }
> +
> + ret = rpmh_regulator_allocate_vreg(pmic);

_allocate_vregs (plural)?

> + if (ret < 0) {
> + dev_err(dev, "failed to allocate regulator subnode array, ret=%d\n",

Please no allocation error messages, kmalloc already prints a bunch of
info.

> + ret);
> + goto cleanup;
> + }
> +
> + for (i = 0; i < pmic->vreg_count; i++) {
> + ret = rpmh_regulator_init_vreg(&pmic->vreg[i]);
> + if (ret < 0) {
> + dev_err(dev, "unable to initialize rpmh-regulator vreg %s, ret=%d\n",
> + pmic->vreg[i].of_node->name, ret);
> + goto cleanup;
> + }
> + }
> +
> + dev_dbg(dev, "successfully probed %d %s regulators\n",
> + pmic->vreg_count, pmic->init_data->name);

Doesn't the regulator framework already print a bunch of constraint
stuff when regulators are registered? Seems sort of spammy even for
debug mode. Plus it's the only reason for pmic::name right now.

> +
> + return ret;
> +
> +cleanup:
> + rpmh_release(pmic->rpmh_client);
> +
> + return ret;
> +}
> +
> +static int rpmh_regulator_remove(struct platform_device *pdev)
> +{
> + struct rpmh_pmic *pmic = platform_get_drvdata(pdev);
> +
> + rpmh_release(pmic->rpmh_client);

I'm still lost on what rpmh_client is giving us besides more code we
don't need. I'll ping the rpmh thread again.

> +
> + return 0;
> +}

2018-03-21 23:55:53

[permalink] [raw]

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

Hello Doug,

On 03/20/2018 07:16 PM, Doug Anderson wrote:
>> +Qualcomm Technologies, Inc. RPMh Regulators
>> +
>> +rpmh-regulator devices support PMIC regulator management via the VRM and XOB
>> +RPMh accelerators. The APPS processor communicates with these hardware blocks
>> +via an 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.
>
> Somewhere in here can you give some context of what VRM and XOB stand
> for. From the other patch desc it's "voltage regulator manager (VRM)
> and oscillator buffer (XOB)", but nice to sprinkle that around the
> first time it's used in documents.

Sure, I'll added a more detailed description in the binding file.

> ...and, ummmmm, what's an oscillator buffer? Is this really a
> regulator? It sounds a lot more like a clock enable knob. Are you
> sure this shouldn't be exposed through the common clock framework?
> Many other PMICs expose oscillator clocks through CCF. MAX77686 comes
> to mind.

The XOB hardware naming is unfortunate as it leads to confusion. I kept
the name in DT bindings and the driver so that it matches hardware
documentation. Functionally, the XOB block in RPMh provides an interface
to enable and disable *any* PMIC peripheral, not just oscillator clock
buffers.

When this hardware feature was initially designed, it's primary use case
was PMIC clock buffer control so it was designated "XOB". However, other
use cases have come up since that point. One use case that is explicitly
described in the qcom_rpmh-regulator driver is control of PM8998
low-voltage switches (LVS) 1 and 2. These switches physically only
support enabling and disabling so it is better to control them via XOB and
VRM.

A second regulator XOB use case is management of regulators which could be
controlled via VRM (as they support voltage and mode configuration) but
which must be handled via XOB as all VRM indices in RPMh hardware are
already used by other regulators. This case is board specific (as opposed
to PMIC specific) which is why there needs to be a way to specify that a
given regulator is XOB controlled from device tree. This case comes up on
later targets which use multiple PMICs. When it does, the set of LDO/SMPS
regulators to be controlled via XOB is carefully chosen at a system level
to be sure that a single voltage and mode is acceptable for each regulator.

As a side note, there are several PMIC clock buffers which are controlled
via XOB. A separate RPMh clock driver will soon be submitted for these.
The MSM register space and hardware control logic for each XOB (or VRM)
resource is independent so there is no concern about having multiple
software entities using XOB to control different resources (regulators vs
clocks).

>> +- regulator-name
>> + Usage: optional
>> + Value type: <string>
>> + Definition: Specifies the name for this RPMh regulator. If not
>> + specified, then the regulator's name is equal to its subnode
>> + name.
>
> Probably don't need to include "regulator-name" since you say below
> "Other properties defined in regulator.txt may also be used" and this
> isn't anything special for your regulator.

Sure, I'll remove this.

>> +- regulator-min-microvolt
>> + Usage: required
>> + Value type: <u32>
>> + Definition: For VRM resources, this is the minimum supported voltage in
>> + microvolts. For XOB resources, this is the fixed output
>> + voltage.
>> +
>> +- regulator-max-microvolt
>> + Usage: required
>> + Value type: <u32>
>> + Definition: For VRM resources, this is the maximum supported voltage in
>> + microvolts. For XOB resources, this is the fixed output
>> + voltage.
>
> regulator-min-microvolt / regulator-max-microvolt are really required?
> What happens if you leave them off? In general the regulator
> framework supports this concept--it just lets you enable/disable
> without changing voltage.

I'll test with these properties not specified. Assuming that nothing bad
happens, I'll remove mention of them in this file.

>> +- qcom,regulator-initial-voltage
>> + Usage: optional; VRM regulators only
>> + Value type: <u32>
>> + Definition: Specifies the initial voltage in microvolts to request for a
>> + VRM regulator. Supported values are 0 to 8191000.
>
> The "supported values" here is a strange statement to make. Not all
> regulators will support all those voltages, right? Do you really need
> to list this here?

My intention was to list the set of values that the VRM voltage control
register will physically accept. As you said, the meaningful/useful range
for a given PMIC regulator will be a small subset of this range. I'll
remove the "supported values" sentence.

>> +- 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 4).
>
> Explicitly state whether this is allowed even if
> "regulator-allow-set-load" is not set.

Will do. It is allowed without regulator-allow-set-load. This allows a
mode to be selected for a regulator even if there isn't a meaningful way
to configure the mode dynamically at runtime based on load current.

>> +- regulator-allow-set-load
>> + Usage: optional
>> + Value type: <empty>
>> + Definition: Boolean flag indicating that the the mode of this regulator
>> + may be configured at runtime based upon consumer load needs.
>> +
>> +- qcom,allowed-modes
>
> It would be up to Mark Brown, but my guess is that he will say "please
> add this to the core". The regulator core already has the concept of
> modes and you're already using the standard core concepts in most
> places. Get rid of the special case code in your driver and add this
> to the core.

I'd prefer to keep this specific to RPMh regulators unless there is a
major objection.

>> + 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 4). Elements must be specified in order
>> + from lowest to highest value.
>
> As far as I can tell this sorting constraint should be removed.
> Really the constraints should be:
>
> * qcom,mode-threshold-currents should be sorted from lowest to highest
> * qcom,allowed-modes must match qcom,mode-threshold-currents
>
> Sure, today it happens that the numbers are always lower for lower
> currents, but it doesn't seem like something you'd have to encode into
> the dt.

I'll update the description.

>> +- qcom,headroom-voltage
>> + 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.
>
> Is this just "regulator-microvolt-offset", but supported in hardware?

No, it is equivalent to struct regulator_desc.min_dropout_uV, but handled
in hardware.

>> +- qcom,rpmh-resource-type
>> + Usage: optional
>> + Value type: <string>
>> + Definition: RPMh accelerator type for this regulator. If not specified,
>> + then the default type associated with this regulator will be
>> + used. Supported values: "vrm" or "xob".
>
> I still don't have the big picture I guess, but it seems weird that
> you can override this. You're saying that someone might stick an
> oscillator buffer the place of something that's normally a regulator?
> ...or a regulator in the place of something that's usually an
> oscillator buffer? When would you use this?

Yes, there are specific board level use cases requiring the use of XOB
instead of VRM for regulators which could be managed via VRM. I explained
this in more detail above.

Take care,
David

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

2018-03-22 22:33:02

[permalink] [raw]

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

Hello Doug,

Thank you for the very detailed review feedback.

On 03/20/2018 07:16 PM, Doug Anderson wrote:
> On Fri, Mar 16, 2018 at 6:09 PM, David Collins <[email protected]> wrote:
>> +/**
>> + * struct rpmh_regulator_mode - RPMh VRM mode attributes
>> + * @pmic_mode: Raw PMIC mode value written into VRM mode voting
>> + * register (i.e. RPMH_REGULATOR_MODE_*)
>> + * @framework_mode: Regulator framework mode value
>> + * (i.e. REGULATOR_MODE_*)
>> + * @min_load_ua: The minimum load current in microamps which
>> + * would utilize this mode
>
> Thinking of this as "min load" seems backward to me. Shouldn't we be
> keeping track of "max load". AKA:
>
> Use "idle" if the load is less than 1000 mA
> Use "normal" if the load is more than 1000 mA but less than 5000 mA
> Use "fast" if the load is more than 5000 mA.
>
> I'd think of "1000 mA as the max load that "idle" can handle". The
> reason I don't think of it as "min" is that you can't say "1000 mA is
> the min load the "normal" can handle". Normal can handle smaller
> loads just fine, it's just not ideal.
>
> Thinking of it in terms of "max" would also get rid of that weird
> "needs to be 0" entry in the device tree too. You could either leave
> the number off the top one (and set to INT_MAX in software?) or you
> could use that to put in some sort of sane maximum current. I'd bet
> there's something in the datasheet for this. If someone in software
> got mixed up and kept requesting more and more current, this could
> catch their error.

Sure, I'll change the logic to use max instead of min threshold currents
in the driver and device tree.

>> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +
>> + return vreg->enabled;
>
> You can't read hardware? The regulator framework expects you to read
> hardware. If it was common not to be able to read hardware then the
> regulator framework would just keep track of the enabled state for
> you. Right now if a regulator was left on by the BIOS (presumably
> some have to be since otherwise you're running on a computer that
> takes no power), you'll still return false at bootup? Seems
> non-ideal.

Correct, the RPMh interface provides no mechanism to poll the physical
PMIC regulator hardware state.

However, that isn't entirely a bad thing due to the nature of RPMh
regulator control. Several processors within the SoC besides the
application processor are able to vote on the state of PMIC regulators via
RPMh. The VRM and XOB RPMh hardware blocks perform max aggregation across
votes from different processors for each votable quantity for a given
regulator (enable, voltage, mode, and headroom voltage for VRM; enable for
XOB). The aggregated values are then configured in the PMIC regulator via
SPMI transactions.

If Linux could read the physical PMIC regulator state and report it back
via is_enabled(), get_voltage() and get_mode() regulator ops, then we
could get into situations where the applications processor fails to vote
on a regulator which is requested by a Linux consumer. For example, say
that the modem processor has requested a given regulator to be enabled via
RPMh. Later, a Linux consumer driver on the application processor calls
regulator_enable() for the regulator. The regulator framework will call
the is_enabled() callback which will return true. This would then
short-circuit the regulator_enable() call and skip calling the
rpmh-regulator enable() callback. This means that the application
processor would have no enable vote present within RPMh for the regulator.
If the modem processor then voted to disable the regulator, it would
physically be disabled even though a consumer on the application processor
expects it to stay enabled.

>> +}
>> +
>> +static int rpmh_regulator_enable(struct regulator_dev *rdev)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
>> + .data = RPMH_REGULATOR_ENABLE,
>> + };
>> + int ret;
>> +
>> + if (vreg->enabled)
>> + return 0;
>
> Does the "if" test above ever hit? I'd think the regulator framework
> would handle this.

I'm not sure if it has ever been hit. I can remove it.

>> +static int rpmh_regulator_disable(struct regulator_dev *rdev)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
>> + .data = RPMH_REGULATOR_DISABLE,
>> + };
>> + int ret;
>> +
>> + if (!vreg->enabled)
>> + return 0;
>
> Does the "if" test above ever hit? I'd think the regulator framework
> would handle this.

I'm not sure if it has ever been hit. I can remove it.

>> +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
>> + int min_uv, int max_uv, unsigned int *selector)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
>> + };
>> + const struct regulator_linear_range *range;
>> + int mv, uv, ret;
>> + bool wait_for_ack;
>> +
>> + mv = DIV_ROUND_UP(min_uv, 1000);
>> + uv = mv * 1000;
>> + if (uv > max_uv) {
>> + vreg_err(vreg, "no set points available in range %d-%d uV\n",
>> + min_uv, max_uv);
>> + return -EINVAL;
>> + }
>> +
>> + range = vreg->hw_data->voltage_range;
>> + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step);
>
> It seems like you should remove the existing check you have for "if
> (uv > max_uv)" and replace with a check here. Specifically, it seems
> like the DIV_ROUND_UP for the selector could also bump you over the
> max. AKA:
>
> ...
> bool wait_for_ack;
>
> range = vreg->hw_data->voltage_range;
> *selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step);
> uv = *selector * range->uV_step + range->min_uV
> if (uv > max_uv) {
> ...
> }

The VRM voltage vote register has units of mV (as does the voltage control
register for the PMIC regulators). The PMIC hardware automatically rounds
up the mV requests to the next physically available set point. This was
designed to simplify the life for software so that there isn't a need to
worry about voltage step size which varies between regulator types.

The intention of the mv check was to verify that what is being programmed
into VRM is within the requested min_uv-max_uv range. However, since this
driver is forced to be aware of the step size, I can modify this as you
suggested with the stricter step-size bounding.

> Hold up, though. Why don't you implement set_voltage_sel() instead of
> set_voltage()? That's what literally everyone else does, well except
> PWM regulators. Using that will get rid of most of this code, won't
> it? Even the check to see if perhaps the voltage isn't changing.

There are two cases that I can think of: 1. Having a set_voltage()
callback allows for delaying for an RPMh request ACK during certain
voltage set point decreasing scenarios (to be elaborated below). 2.
Having a get_voltage() as opposed to get_voltage_sel() callback allows an
uninitialized voltage of 0 to be returned in the case that no initial
voltage is specified in device tree. This eliminates the possibility of
the regulator framework short-circuiting a regulator_set_voltage() call
that happens to match the voltage corresponding to selector 0.

>> +
>> + if (uv == vreg->voltage)
>> + return 0;
>> +
>> + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage;
>
> Do you often see "wait_for_ack = false" in reality? Most regulator
> usage I've seen requests a fairly tight range. AKA: I don't often
> see:
>
> set_voltage(min=3000mV, max=3300mV)
> set_voltage(min=1800mV, max=3300mV)

I can't think of a good example off the top of my head. However, this
situation is certainly valid and can arise any time that there is a shared
rail and the hardware connected to it has minimum voltage requirements per
use case but large maximum voltage allowed.

> Instead, I see:
>
> set_voltage(min=3000mV, max=3300mV)
> set_voltage(min=1800mV, max=1900mV)
>
> So you'll always have wait_for_ack = true in the cases I've seen.
>
> ...but are you certain it's useful to wait for an ack anyway when the
> voltage is falling? Most regulators won't guarantee that the voltage
> has actually fallen even after they ack you. Specifically if a
> regulator is under light load and it doesn't have an active discharge
> circuit then the regulator might fall very slowly over time. As a
> specific example, see commit 7c5209c315ea ("mmc: core: Increase delay
> for voltage to stabilize from 3.3V to 1.8V").

I'm aware of one important instance in which decreasing voltage needs a
delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case
that I had in mind with the 'max_uv < vreg->voltage' check. However, you
are correct that hardware will report completion of the voltage slewing
early in this case since the comparator is checking for output >= set
point. I can remove this special case check.

> That was a lot of words, so to sum it all up:
>
> * If you have no actual examples where you see "wait_for_ack = false"
> then remove this code and always wait.
>
> * If you have evidence that the time spent waiting for the ack is
> slowing you down, consider always setting wait_for_ack to false when
> you're lowering the voltage. Anyone who truly cares could just set
> something like the device tree property
> regulator-settling-time-down-us. ...or, assuming Mark doesn't hate
> it, they could set the always-wait-for-ack property in the device
> tree.

I'm ok with changing the logic so that it waits for an ACK when increasing
voltage (required for correctness) and does not wait for an ACK when
decreasing voltage.

> NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy
> ARC things for this?), we're probably talking about a small handful of
> voltage transitions per boot, right?

VRM isn't used for CPU DVFS. I haven't profiled the quantity of RPMh
regulator requests made during different use cases.

>> +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +
>> + return vreg->voltage;
>
> I guess there's no way to read the voltage at bootup? So this will
> return 0 until someone sets it? ...maybe less of a big deal due to
> the "qcom,regulator-initial-voltage" property?

Correct. As mentioned above, there is no way to read the physical PMIC
regulator voltage from RPMh. Yes, this will return 0 unless
qcom,regulator-initial-voltage is specified or the set_voltage() regulator
op is called. Returning an invalid voltage of 0 uV here is convenient as
it ensures that there is no way to accidentally skip setting the voltage
due to returning the voltage of selector 0.

>> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
>> + unsigned int mode)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>> + };
>> + int i, ret;
>> +
>> + if (mode == vreg->mode)
>> + return 0;
>> +
>> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
>> + if (vreg->hw_data->mode_map[i].framework_mode == mode)
>> + break;
>> + if (i >= RPMH_REGULATOR_MODE_COUNT) {
>> + vreg_err(vreg, "invalid mode=%u\n", mode);
>> + return -EINVAL;
>> + }
>> +
>> + cmd.data = vreg->hw_data->mode_map[i].pmic_mode;
>> +
>> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
>> + mode < vreg->mode || !vreg->mode);
>
> Please explain the "mode < vreg->mode || !vreg->mode" test in words.

This waits for an ACK from RPMh of successfully mode transition when
switching to a higher power mode (regulator framework modes are ordered
with highest power == lowest numerical value) or when setting the first mode.

>> +static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +
>> + return vreg->mode;
>
> You'll probably guess that I'd expect you to read this from hardware.

As mentioned above, there is no way to read the physical PMIC regulator
mode from RPMh.

>> +/**
>> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
>> + * for a VRM RPMh resource from device tree
>> + * vreg: Pointer to the rpmh regulator resource
>> + *
>> + * This function initializes the mode[] array of vreg based upon the values
>> + * of optional device tree properties.
>> + *
>> + * Return: 0 on success, errno on failure
>> + */
>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
>> +{
>> + struct device_node *node = vreg->of_node;
>> + const struct rpmh_regulator_mode *map;
>> + const char *prop;
>> + int i, len, ret;
>> + u32 *buf;
>> +
>> + map = vreg->hw_data->mode_map;
>> + if (!map)
>> + return 0;
>> +
>> + /* qcom,allowed-modes is optional */
>> + prop = "qcom,allowed-modes";
>> + len = of_property_count_elems_of_size(node, prop, sizeof(u32));
>> + if (len < 0)
>> + return 0;
>
> Seems like it might be worth it to count
> "qcom,mode-threshold-currents" too and confirm the count is the same?
> If someone left in an extra threshold current you won't notice
> otherwise (right?)

Sure, I'll add a check for that.

>> +
>> + vreg->mode_map = devm_kcalloc(vreg->pmic->dev, len,
>> + sizeof(*vreg->mode_map), GFP_KERNEL);
>
> I keep getting myself confused because you have two things called
> "mode_map" and they work differently:
>
> vreg->mode_map - contains 1 element per allowed mode. Need to iterate
> through this to map "framework mode" to "pmic mode". Note: because of
> the need to iterate this isn't really a "map" in my mind.
>
> vreg->hw_data->mode_map - contains 1 element for each possible "device
> tree mode". Index into this using "device tree mode" and you get both
> a "framework mode" and "pmic mode".

I agree that this is confusing. I reused struct rpmh_regulator_mode for
both tables out of convenience. I'll change this.

> IMHO it would be better to have a table like "dt_to_linux_mode" that
> was just a simple list:
>
> static const int dt_to_linux_mode_bob[] = [
> [RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY,
> [RPMH_REGULATOR_MODE_RET] = -EINVAL,
> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
> ];
>
> static const int dt_to_linux_mode_ldo_smps[] =
> [RPMH_REGULATOR_MODE_PASS] = -EINVAL,
> [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
> ];
>
> You'd only use that in the "map_mode" functions and when parsing
> qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could
> actually just call the map_mode function. This would be especially a
> good way to do this if you moved "allowed-modes" into the regulator
> core, which seems like a good idea.
>
> The nice thing about this is that only place you need to conceptually
> keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing
> code. Otherwise you just think of the Linux version.

I would be ok with defining arrays like these to handle the mapping of
PMIC regulator hardware agnostic "device tree mode" to "framework mode".
However, a second set of arrays would then be needed to map from
"framework mode" to hardware specific "pmic mode". I suppose that having
the second array indexed by framework mode would make the code a little
simpler since iteration would not be needed.

Here is an explanation for why the "device tree mode" abstraction is
present in the first place. Between different Qualcomm Technologies, Inc.
PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO,
LPM/PFM, Retention, and pass-through). However, the register values for
the same modes vary between different regulator types and different PMIC
families. This patch is adding support for several PMIC4 family PMICs.
The values needed for to-be-released PMIC5 PMIC regulators are different.
As an example, here are the different values for LPM/PFM across PMIC
families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5
LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode"
ensures that it is not possible to inadvertently specify a PMIC specific
mode in device tree which corresponds to the wrong type or family but
which aliases a value that would be accepted as correct.

> Once you do the above, then your other list could just be named
> "allowed_modes". This would make it obvious that this isn't a map
> itself but that you could iterate over it to accomplish a mapping.

I'm concerned with using the name "allowed_modes" as it tends to imply the
wrong idea. Perhaps something along the lines of "drms_modes" would be
better (to go along with REGULATOR_CHANGE_DRMS). These would be the modes
utilized by the set_load() callback along with corresponding threshold
load currents.

>> +/**
>> + * rpmh_regulator_allocate_vreg() - allocate space for the regulators associated
>> + * with the PMIC and initialize important pointers for each
>> + * regulator
>> + * @pmic: Pointer to the RPMh regulator PMIC
>> + *
>> + * Return: 0 on success, errno on failure
>> + */
>> +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic)
>> +{
>> + struct device_node *node;
>> + int i;
>> +
>> + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node);
>> + if (pmic->vreg_count == 0) {
>> + dev_err(pmic->dev, "could not find any regulator subnodes\n");
>> + return -ENODEV;
>> + }
>> +
>> + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count,
>> + sizeof(*pmic->vreg), GFP_KERNEL);
>> + if (!pmic->vreg)
>> + return -ENOMEM;
>> +
>> + i = 0;
>> + for_each_available_child_of_node(pmic->dev->of_node, node) {
>> + pmic->vreg[i].of_node = node;
>> + pmic->vreg[i].pmic = pmic;
>> +
>> + i++;
>> + }
>
> While I can believe that things don't crash, you're not quite using
> for_each_available_child_of_node() correctly. You need to reorganize
> your code structure to fix. Specifically the "node" that's provided
> to each iteration only has its refcount held for each iteration. By
> the end of your function the refcount of all the "of_node"s that you
> stored wil be 0. Doh.
>
> You could try to fix this by adding a of_node_get() before storing the
> node and that would work, but that would complicate your error
> handling. You'll need to do this in a "cleanup" error path of probe
> and in remove. :(
>
> A better solution is to get rid of the rpmh_regulator_allocate_vreg()
> function and move the functionality into probe. There, instead of
> iterating over pmic->vreg_count, just use the
> for_each_available_child_of_node() function. If
> rpmh_regulator_init_vreg() you'll have to manually of_node_put() but
> that should be OK.
>
> Why will that work? You'll call rpmh_regulator_init_vreg() while the
> reference count is still held. In that function you'll call
> devm_regulator_register(), which will grab the refcount if it needs
> it. Since that's a devm_ function you can be sure that it will
> properly drop the refcount at the right times.
>
> NOTE: with this you presumably should remove the "of_node" from the
> "struct rpmh_vreg". It seems like it shouldn't be needed anymore and
> it's good not to keep the pointer around if you didn't call
> of_node_get() yourself.

I'll make this change.

>> +/**
>> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
>> + * request for this regulator based on optional device tree
>> + * properties
>> + * @vreg: Pointer to the RPMh regulator
>> + *
>> + * Return: 0 on success, errno on failure
>> + */
>> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
>> +{
>> + struct tcs_cmd cmd[2] = { };
>> + const char *prop;
>> + int cmd_count = 0;
>> + int ret;
>> + u32 temp;
>> +
>> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
>> + prop = "qcom,headroom-voltage";
>> + ret = of_property_read_u32(vreg->of_node, prop, &temp);
>> + if (!ret) {
>> + if (temp < RPMH_VRM_HEADROOM_MIN_UV ||
>> + temp > RPMH_VRM_HEADROOM_MAX_UV) {
>> + vreg_err(vreg, "%s=%u is invalid\n",
>> + prop, temp);
>> + return -EINVAL;
>> + }
>> + vreg->headroom_voltage = temp;
>> +
>> + cmd[cmd_count].addr
>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_HEADROOM;
>> + cmd[cmd_count++].data
>> + = DIV_ROUND_UP(vreg->headroom_voltage, 1000);
>> + }
>> +
>> + prop = "qcom,regulator-initial-voltage";
>> + ret = of_property_read_u32(vreg->of_node, prop, &temp);
>> + if (!ret) {
>> + if (temp < RPMH_VRM_MIN_UV || temp > RPMH_VRM_MAX_UV) {
>> + vreg_err(vreg, "%s=%u is invalid\n",
>> + prop, temp);
>> + return -EINVAL;
>> + }
>> + vreg->voltage = temp;
>> +
>> + cmd[cmd_count].addr
>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE;
>> + cmd[cmd_count++].data
>> + = DIV_ROUND_UP(vreg->voltage, 1000);
>
> It seems like you should set vreg->voltage to the actual result of the
> division * 1000. AKA: if the user said the initial voltage was
> 123,456 then vreg->voltage should become 124,000.

Sure, I'll change this.

> Actually, shouldn't you somehow resolve this with "struct
> rpmh_vreg_hw_data". It seems like some regulators have 128 steps,
> some have 256 steps, etc. You should have a function that reconciles
> the requested voltage with the one that hardware will actually
> provide.
>
> ...and, actually, you should share code for this reconciliation with
> rpmh_regulator_vrm_set_voltage().

I'll see what I can do about this.

>> +/**
>> + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator
>> + * @vreg: Pointer to the RPMh regulator
>> + *
>> + * Return: 0 on success, errno on failure
>> + */
>> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg)
>> +{
>> + struct device *dev = vreg->pmic->dev;
>> + struct regulator_config reg_config = {};
>> + const struct rpmh_vreg_init_data *rpmh_data = NULL;
>> + const char *type_name = NULL;
>> + enum rpmh_regulator_type type;
>> + struct regulator_init_data *init_data;
>> + int ret, i;
>> +
>> + for (i = 0; i < vreg->pmic->init_data->count; i++) {
>> + if (!strcmp(vreg->pmic->init_data->vreg_data[i].name,
>> + vreg->of_node->name)) {
>> + rpmh_data = &vreg->pmic->init_data->vreg_data[i];
>> + break;
>> + }
>> + }
>> +
>> + if (!rpmh_data) {
>> + dev_err(dev, "Unknown regulator %s for %s RPMh regulator PMIC\n",
>> + vreg->of_node->name, vreg->pmic->init_data->name);
>> + return -EINVAL;
>> + }
>> +
>> + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d",
>> + rpmh_data->resource_name_base, vreg->pmic->pmic_id,
>> + rpmh_data->id);
>> + if (!vreg->resource_name)
>> + return -ENOMEM;
>> +
>> + vreg->addr = cmd_db_read_addr(vreg->resource_name);
>> + if (!vreg->addr) {
>> + vreg_err(vreg, "could not find RPMh address for resource %s\n",
>> + vreg->resource_name);
>> + return -ENODEV;
>> + }
>> +
>> + vreg->rdesc.name = rpmh_data->name;
>> + vreg->rdesc.supply_name = rpmh_data->supply_name;
>> + vreg->regulator_type = rpmh_data->regulator_type;
>> + vreg->hw_data = rpmh_data->hw_data;
>> +
>> + if (rpmh_data->hw_data->voltage_range) {
>> + vreg->rdesc.linear_ranges = rpmh_data->hw_data->voltage_range;
>> + vreg->rdesc.n_linear_ranges = 1;
>> + vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages;
>> + }
>> +
>> + /* Optional override for the default RPMh accelerator type */
>> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",
>> + &type_name);
>> + if (!ret) {
>> + if (!strcmp("vrm", type_name)) {
>> + vreg->regulator_type = RPMH_REGULATOR_TYPE_VRM;
>> + } else if (!strcmp("xob", type_name)) {
>> + vreg->regulator_type = RPMH_REGULATOR_TYPE_XOB;
>> + } else {
>> + vreg_err(vreg, "Unknown RPMh accelerator type %s\n",
>> + type_name);
>> + return -EINVAL;
>> + }
>
> As per comment in device tree patch, it seems really weird that you
> can override this. Are you sure?

Yes, we definitely need to have a board-specific mechanism to specify that
a regulator which could be handled via VRM must be handled via XOB on a
given board. I explained this in more detail in [1].

>> +static const struct rpmh_regulator_mode
>> +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = {
>> + [RPMH_REGULATOR_MODE_PASS] = {
>> + .pmic_mode = 0,
>> + .framework_mode = REGULATOR_MODE_STANDBY,
>
> Is "PASS" truly the same concept as the Linux concept of STANDBY. If
> so, why do you need a separate define for it?
>
> If it truly is the same, it seems like you can simplify everything by
> just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and
> "RPMH_REGULATOR_MODE_PASS" and just call it
> "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that
> "standby" maps to "retention" for some regulators and maps to "pass"
> for other regulators if you want to map PMIC documentation. ...but
> getting rid of this distinction simply means less error checking and
> fewer tables in Linux.
>
> If "pass" really shouldn't map to "standby" then this seems like a
> hack and you should add the concept of "pass" to the core regulator
> framework.

For Qualcomm Technologies, Inc. PMIC regulators, retention mode (RET) is a
very low power mode which offers better power effeciency (i.e. lower
ground current) than low power mode (LPM) for low load scenarios with the
trade off of worse load regulator. It is typically applied when the
system is asleep for regulators that must stay on but which have very low
and stable load. Retention maps very well to REGULATOR_MODE_STANDBY.

Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm
Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected,
the BOB output is directly connected to the BOB input (typically Vbat).
This does not map exactly to REGULATOR_MODE_STANDBY. I agree that it is
somewhat hacky to use it in this way. However, doing so makes
qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode
could be handled via get_bypass() and set_bypass() regulator ops. Doing
this would require more complicated ops selections in the driver since it
could no longer be determined simply by VRM vs XOB, it would instead need
to be BOB VRM, other VRM, and XOB. The BOB set_mode() and set_bypass()
callbacks would be complicated because both would be writing to the same
VRM address (mode control) with bypass==true taking precedence.
Additionally, there is currently no way to specify default bypass from DT.
A BOB-specific property would be needed to get this information.

>> +static const struct rpmh_vreg_hw_data pmic4_pldo_hw_data = {
>> + .voltage_range = &(const struct regulator_linear_range)
>> + REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),
>
> This seems pretty iffy to me. You're relying on the compiler to make
> an anonymous chunk of memory with a "struct regulator_linear_range" in
> it and then storing a pointer to said anonymous chunk of memory?
> Nobody else using REGULATOR_LINEAR_RANGE() does this.

A similiar approach is used in qcom_smd-regulator.c [2]:

static const struct regulator_desc pm8941_nldo = {
.linear_ranges = (struct regulator_linear_range[]) {
REGULATOR_LINEAR_RANGE(750000, 0, 63, 12500),
},
.n_linear_ranges = 1,
.n_voltages = 64,
.ops = &rpm_smps_ldo_ops,
};

> Why not just get rid of the pointer and put the structure right inside
> "struct rpmh_vreg_hw_data"? It'll get rid of the weird cast / strange
> anonymous chunk, save an indirection, and save 4 bytes for a pointer.

I'll make this change.

>> +/**
>> + * rpmh_regulator_probe() - probe an RPMh PMIC and register regulators for each
>> + * of the regulator nodes associated with it
>> + * @pdev: Pointer to the platform device of the RPMh PMIC
>> + *
>> + * Return: 0 on success, errno on failure
>> + */
>> +static int rpmh_regulator_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + const struct of_device_id *match;
>> + struct rpmh_pmic *pmic;
>> + struct device_node *node;
>> + int ret, i;
>> +
>> + node = dev->of_node;
>> +
>> + if (!node) {
>> + dev_err(dev, "Device tree node is missing\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = cmd_db_ready();
>> + if (ret < 0) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
>> + return ret;
>> + }
>> +
>> + pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
>> + if (!pmic)
>> + return -ENOMEM;
>> +
>> + pmic->dev = dev;
>> + platform_set_drvdata(pdev, pmic);
>> +
>> + pmic->rpmh_client = rpmh_get_client(pdev);
>
> It seems like you'd do yourself (and the other clients of rpmh) a
> favor if you added a devm_rpmh_get_client() in a patch before this
> one. Adding a devm version of a calls is pretty easy and you'll be
> able to completely get rid of your "remove" function. ...and get rid
> of the "cleanup" exit here.

Lina, could you please add this to your rpmh patch series since it is
still undergoing review? [3]

>> +static struct platform_driver rpmh_regulator_driver = {
>> + .driver = {
>> + .name = "qcom-rpmh-regulator",
>> + .of_match_table = rpmh_regulator_match_table,
>> + .owner = THIS_MODULE,
>
> As per the robot, no need to set .owner here. The core will do it.

I'll remove this.

>> + },
>> + .probe = rpmh_regulator_probe,
>> + .remove = rpmh_regulator_remove,
>> +};
>> +
>> +static int rpmh_regulator_init(void)
>> +{
>> + return platform_driver_register(&rpmh_regulator_driver);
>> +}
>> +
>> +static void rpmh_regulator_exit(void)
>> +{
>> + platform_driver_unregister(&rpmh_regulator_driver);
>> +}
>> +
>> +arch_initcall(rpmh_regulator_init);
>
> I always get yelled at when I try to use arch_initcall() for stuff
> like this. You should do what everyone else does and use
> module_platform_driver() to declare your driver. Yeah, regulators are
> important and (as I remember) they get probed slightly early anyway,
> but everything else in the system just gotta deal with the fact that
> they'll sometimes get deferred probes.

I agree that consumers should handle probe deferral. Unfortunately,
reality isn't always so nice. Also, probe deferrals increase boot-up time
which is particularly problematic in the mobile space. I suppose that I
can change this to module_platform_driver() for now. If any major issues
arise it could be changed back to arch_initcall().

Note that both qcom_rpm-regulator.c and qcom_smd-regulator.c are using
subsys_initcall() right now in place of module_platform_driver().

>> +module_exit(rpmh_regulator_exit);
>> +
>> +MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
>> +MODULE_LICENSE("GPL v2");
>> 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..f854e0e
>> --- /dev/null
>> +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
>
> Device tree guys will yell at you here. The "include/dt-bindings"
> bits are supposed to be together with the bindings. Different
> maintainers have different beliefs here, but I think the way that's
> least likely to get you yelled at by the most people is:
>
> Patch #1: bindings (.txt and include file)
> Patch #2: the driver
>
> ...with the idea being that if another operating system wanted just
> the bindings they could get it all in one patch.

I'll move the header into the DT patch of my series.

>> @@ -0,0 +1,40 @@
>> +/* 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 qcom,allowed-modes and qcom,init-mode
>
> Not "qcom,init-mode". This is actually "regulator-initial-mode" now.

I'll correct this.

Take care,
David

[1]: https://lkml.org/lkml/2018/3/21/877
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/qcom_smd-regulator.c?h=v4.16-rc6#n252
[3]: https://lkml.org/lkml/2018/3/9/979

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

2018-03-23 01:31:33

[permalink] [raw]

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

Hello Stephen,

Thank you for the very detailed review feedback.

On 03/21/2018 12:07 PM, Stephen Boyd wrote:
> Quoting David Collins (2018-03-16 18:09:10)
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index 097f617..e0ecd0a 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
>> Qualcomm RPM as a module. The module will be named
>> "qcom_rpm-regulator".
>>
>> +config REGULATOR_QCOM_RPMH
>> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
>> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
>
> What's the build dependency on OF?

The qcom_rpmh-regulator driver cannot function without device tree support
enabled. I suppose that it might be able to compile, but it wouldn't be
useful.

>> diff --git a/drivers/regulator/qcom_rpmh-regulator.c b/drivers/regulator/qcom_rpmh-regulator.c
...
>> +#include <linux/string.h>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>
> Including machine is usually a red flag in regulator drivers.

Many of the regulator drivers include machine.h. It is done here in order
to allow manipulation of constraints.valid_ops_mask (to set
REGULATOR_CHANGE_MODE) and constraints.valid_mode_mask. There is no DT
method to configure these fields.

>> +/* Register offsets: */
>
> Why the colon? ^
> Why even have the comment?

I can remove such comments if they are not preferred in upstream code.

>> +/* Enable register values: */
>> +#define RPMH_REGULATOR_DISABLE 0x0
>> +#define RPMH_REGULATOR_ENABLE 0x1
>> +
>> +/* Number of unique hardware modes supported: */
>
> Both above also look useless.

I'll remove them too.

>> + * @rdev: Regulator device pointer returned by
>> + * devm_regulator_register()
>
> Is this used? Why save it around?

I'll remove it.

>> + * @voltage: RPMh VRM regulator voltage in microvolts
>
> So call it uV?

Ok.

>> + * @mode: RPMh VRM regulator current framework mode
>> + * @headroom_voltage: RPMh VRM regulator minimum headroom voltage
>> + * required
>
> headroom_uV?

Ok.

>> +struct rpmh_vreg {
>> + struct device_node *of_node;
>> + struct rpmh_pmic *pmic;
>> + const char *resource_name;
>> + u32 addr;
>> + struct regulator_desc rdesc;
>> + struct regulator_dev *rdev;
>> + const struct rpmh_vreg_hw_data *hw_data;
>> + enum rpmh_regulator_type regulator_type;
>> + bool always_wait_for_ack;
>> + struct rpmh_regulator_mode *mode_map;
>> + int mode_count;
>
> size_t?

Ok.

>> +
>> + bool enabled;
>> + int voltage;
>> + unsigned int mode;
>> + int headroom_voltage;
>
> Please try to limit the things that are assigned into these structs and
> then never used outside of init. It adds complexity to the code when a
> local variable in the function would work just as well.

I'll remove headroom_voltage from this struct.

>> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
>> + * @name: Name for the regulator which also corresponds
>> + * to the device tree subnode name of the regulator
>> + * @resource_name_base: RPMh regulator resource name prefix. E.g.
>> + * "ldo" for RPMh resource "ldoa1".
>
> Maybe it should be "ldo%c1"? Then we could kasprintf the name with the
> pmic_id and drop the 'id' member entirely.

I can make this modification (though with %s instead of %c for simplicity
with DT string parsing). Hopefully having a variable format string
doesn't trigger any static analysis tools.

>> +struct rpmh_pmic {
>> + struct device *dev;
>> + struct rpmh_client *rpmh_client;
>> + struct rpmh_vreg *vreg;
>
> It's a circle! Life is a circle! It's a circle!

I can get rid of the vreg array.

>> + int vreg_count;
>> + const char *pmic_id;
>> + const struct rpmh_pmic_init_data *init_data;
>
> Hopefully we don't really need this entire struct and we can just use
> local variables instead.

Outside of probe-time, this is used by struct rpmh_vreg in order to access
rpmh_client (for RPMh transactions) and pmic->init_data->name (for debug
and error messages). I suppose that rpmh_client could be specified in
struct rpmh_vreg directly.

>> +static int rpmh_regulator_enable(struct regulator_dev *rdev)
>> +{
>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> + struct tcs_cmd cmd = {
>> + .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
>> + .data = RPMH_REGULATOR_ENABLE,
>> + };
>> + int ret;
>> +
>> + if (vreg->enabled)
>> + return 0;
>> +
>> + ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
>> + if (ret < 0) {
>> + vreg_err(vreg, "enable failed, ret=%d\n", ret);
>
> Do we really need all these error messages deep down in the regulator
> drivers? If consumers care, they'll print some error message themselves
> anyway.

I'd prefer to keep error messages in place unless you have a strong
objection. They make debugging simpler.

>> +/**
>> + * rpmh_regulator_vrm_set_load() - set the PMIC mode based upon the maximum load
>> + * required from the VRM rpmh-regulator
>> + * @rdev: Regulator device pointer for the rpmh-regulator
>> + * @load_ua: Maximum current required from all consumers in microamps
>> + *
>> + * This function is passed as a callback function into the regulator ops that
>> + * are registered for each VRM rpmh-regulator device.
>> + *
>> + * This function sets the mode of the regulator to that which has the highest
>> + * min support load less than or equal to load_ua. Example:
>
> s/support/supported/

ACK

>> +/**
>> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
>> + * for a VRM RPMh resource from device tree
>> + * vreg: Pointer to the rpmh regulator resource
>> + *
>> + * This function initializes the mode[] array of vreg based upon the values
>> + * of optional device tree properties.
>> + *
>> + * Return: 0 on success, errno on failure
>> + */
>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
>> +{
>
> I have a feeling this should all come from the driver data, not DT.
> Doubtful this really changes for each board.

This needs to be determined at a board level instead of hard-coded per
regulator type. For LDOs switching between LPM and HPM typically happens
at 10 mA or 30 mA per hardware documentation. Unfortunately, sharing
control of regulators with other processors adds some subtlety.

Consider the case of a regulator with 10 mA LPM/HPM threshold physically.
Say that modem and application processors each have a load on the
regulator that draws 9 mA. If they each respect the 10 mA threshold, then
they'd each vote for LPM. VRM will aggregate these requests together
which will result in the regulator being set to LPM even though the total
load is 18 mA (which would require HPM). To get around this corner case,
a threshold of 1 uA can be used for all supplies that have non-application
processor consumers. Thus, any non-zero current request will result in
setting the regulator to HPM (which is always safe).

Another example is SMPS regulators which should theoretically always be
able to operate in AUTO mode. However, there may be board/system issues
that require switching to PWM mode (HPM) for certain use cases as it has
better load regulation (even though it can source the same amount of
current as AUTO mode). If there is more than one consumer that requires
this ability for a given regulator, then regulator_set_load() is the only
option since it provides aggregation, where as regulator_set_mode() does not.

>> +static int rpmh_regulator_allocate_vreg(struct rpmh_pmic *pmic)
>> +{
>> + struct device_node *node;
>> + int i;
>> +
>> + pmic->vreg_count = of_get_available_child_count(pmic->dev->of_node);
>> + if (pmic->vreg_count == 0) {
>> + dev_err(pmic->dev, "could not find any regulator subnodes\n");
>> + return -ENODEV;
>> + }
>> +
>> + pmic->vreg = devm_kcalloc(pmic->dev, pmic->vreg_count,
>> + sizeof(*pmic->vreg), GFP_KERNEL);
>> + if (!pmic->vreg)
>> + return -ENOMEM;
>
> Please just allocate one at a time. It's not like we have thousands of
> these things to worry about.

Sure, I'll change this.

>> +/**
>> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
>> + * request for this regulator based on optional device tree
>> + * properties
>> + * @vreg: Pointer to the RPMh regulator
>> + *
>> + * Return: 0 on success, errno on failure
>> + */
>> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
>> +{
>> + struct tcs_cmd cmd[2] = { };
>> + const char *prop;
>> + int cmd_count = 0;
>> + int ret;
>> + u32 temp;
>> +
>> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
>> + prop = "qcom,headroom-voltage";
>
> Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh
> registers. This probably needs to be pushed into the framework and come
> down through a 'set_headroom' op in the regulator ops via a
> regulator-headroom-microvolt property that's parsed in of_regulator.c.

The qcom,headroom-voltage property is equivalent to struct
regulator_desc.min_dropout_uV, but handled in hardware. I don't see the
need to make a new regulator op to configure this value dynamically.
Headroom typically does not need to change. Also, we don't really want
this particular value plumbed into min_dropout_uV since we need to pass it
directly to hardware and not have the regulator framework attempt to use
it for a parent.

>> + prop = "qcom,regulator-initial-voltage";
>
> DT constraints should take care of this by setting voltages on all
> regulators that need them?

Unfortunately not. At regulator registration time, the regulator
framework machine_constraints_voltage() function will call get_voltage()
op and check if the voltage is in the min_uV to max_uV constraint range.
If it is, then nothing happens. If it's not, then it will call the
set_voltage() call back to set the voltage to min_uV if current_uV <
min_uV or max_uV if current_uV > max_uV. Since the rpmh regulator driver
doesn't know the initial voltage, get_voltage() will return 0 initially.
As a result, machine_constraints_voltage() will always set the regulator
to the minimum constraint voltage specified in DT.

That behavior may not be acceptable for some regulators depending upon the
hardware state at kernel initialization. Therefore, we need a DT
mechanism to specify a single voltage to configure by default.

>> + * rpmh_regulator_init_vreg() - initialize all abbributes of an rpmh-regulator
>
> Heh, abbributes.

I'll fix this.

>> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg)
...
>> + vreg->resource_name = devm_kasprintf(dev, GFP_KERNEL, "%s%s%d",
>> + rpmh_data->resource_name_base, vreg->pmic->pmic_id,
>> + rpmh_data->id);
>> + if (!vreg->resource_name)
>> + return -ENOMEM;
>
> This isn't used outside of this function, so remove the
> vreg::resource_name member and use a local variable that gets freed
> on exit please.

Ok.

>> + /* Optional override for the default RPMh accelerator type */
>> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",
>
> Can this property have override in the name? And then because it is
> called override, perhaps it should come from the driver instead of DT
> because DT may need an override itself.

Yes, I can add 'override' to the name of the property. I'm not following
your second sentence. We require the ability to specify that a given
regulator is using XOB instead of VRM at a board level. This means that
the override needs to happen in DT instead of the driver. See [1] for
more details.

> Also, is this currently being used? If not I'd prefer we drop this until we
> need it.

It will be needed on a to-be-released Qualcomm Technologies, Inc. SoC that
is currently under development. I suppose that I can leave this property
out for the initial patch. However, we would still need to ensure that
the driver architecture allows for this property to be read in a later
patch and determine the regulator ops and whether or not to perform VRM
specific initialization.

>> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) {
>> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
>> + init_data->constraints.apply_uV = 0;
>> + vreg->rdesc.n_voltages = 1;
>> + }
>
> What is this doing? Usually constraints aren't touched by the driver.

For XOB managed regulators, we need to set fixed_uV to match the DT
constraint voltage and n_voltages = 1. This allows consumers
regulator_set_voltage() calls to succeed for such regulators. It works
the same as a fixed regulator. I think that apply_uV = 0 could be left out.

>> +
>> + if (vreg->hw_data->mode_map) {
>> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
>
> Huh, I thought this was assigned by the framework.

No, this is not set anywhere in the regulator framework. There isn't a DT
method to configure it. It seems that it could only be handled before
with board files. Other regulator drivers also configure it.

>> +static const struct rpmh_pmic_init_data pm8005_pmic_data = {
>> + .name = "PM8005",
>> + .vreg_data = pm8005_vreg_data,
>> + .count = ARRAY_SIZE(pm8005_vreg_data),
>> +};
>
> Kill 'name' please, and then get rid of 'count' and NUL terminate the
> array instead. This follows previous rpm regulator driver styles. Also,
> drop the macro that does stringification. We should end up with the
> match_table pointing to static arrays of structs that look like:
>
> { "s1", VRM, "smp", 1, &pmic4_ftsmps42, "vdd_s1", },
>
> And yes, drop the RPMH_REGULATOR_TYPE_ prefix and _hw_data postfix.

Ok.

>> +static const struct of_device_id rpmh_regulator_match_table[] = {
>> + {
>> + .compatible = "qcom,pm8998-rpmh-regulators",
>> + .data = &pm8998_pmic_data,
>> + },
>> + {
>> + .compatible = "qcom,pmi8998-rpmh-regulators",
>> + .data = &pmi8998_pmic_data,
>> + },
>> + {
>> + .compatible = "qcom,pm8005-rpmh-regulators",
>> + .data = &pm8005_pmic_data,
>> + },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table);
>
> Please move this array next to the driver structure.

Ok.

>> +static int rpmh_regulator_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + const struct of_device_id *match;
>> + struct rpmh_pmic *pmic;
>> + struct device_node *node;
>> + int ret, i;
>> +
>> + node = dev->of_node;
>> +
>> + if (!node) {
>> + dev_err(dev, "Device tree node is missing\n");
>> + return -EINVAL;
>> + }
>
> This should never happen. Please remove.

Ok.

>> +
>> + ret = cmd_db_ready();
>> + if (ret < 0) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
>> + return ret;
>> + }
>
> We should just make rpmh parent device call cmd_db_ready() so that these
> devices aren't even populated until then and so that cmd_db_ready() is
> only in one place. Lina?

Let's see if Lina has qualms about this plan.

>> + pmic->rpmh_client = rpmh_get_client(pdev);
>> + if (IS_ERR(pmic->rpmh_client)) {
>> + ret = PTR_ERR(pmic->rpmh_client);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "failed to request RPMh client, ret=%d\n",
>> + ret);
>
> I fail to see how this happens given that rpmh creates this device.

I'll remove the EPROBE_DEFER check.

>> + match = of_match_node(rpmh_regulator_match_table, node);
>> + if (match) {
>> + pmic->init_data = match->data;
>> + } else {
>> + dev_err(dev, "could not find compatible string match\n");
>> + ret = -ENODEV;
>> + goto cleanup;
>> + }
>
> Use of_device_get_match_data() instead and print nothing on error.

Ok.

>> + ret = rpmh_regulator_allocate_vreg(pmic);
>
> _allocate_vregs (plural)?

This function is going to be removed per Doug's suggestions.

>> + if (ret < 0) {
>> + dev_err(dev, "failed to allocate regulator subnode array, ret=%d\n",
>
> Please no allocation error messages, kmalloc already prints a bunch of
> info.

Ok.

>> + dev_dbg(dev, "successfully probed %d %s regulators\n",
>> + pmic->vreg_count, pmic->init_data->name);
>
> Doesn't the regulator framework already print a bunch of constraint
> stuff when regulators are registered? Seems sort of spammy even for
> debug mode. Plus it's the only reason for pmic::name right now.

I think that the regulator framework will only print something if it has
to set the voltage in order to bring the regulator voltage into the
constraint range. I suppose that I can remove this message.

>> +static int rpmh_regulator_remove(struct platform_device *pdev)
>> +{
>> + struct rpmh_pmic *pmic = platform_get_drvdata(pdev);
>> +
>> + rpmh_release(pmic->rpmh_client);
>
> I'm still lost on what rpmh_client is giving us besides more code we
> don't need. I'll ping the rpmh thread again.

Let's see if Lina is willing to add some devm_* calls so that no cleanup
is required.

Take care,
David

[1]: https://lkml.org/lkml/2018/3/21/877

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

2018-03-23 20:02:08

[permalink] [raw]

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

Hi,

On Thu, Mar 22, 2018 at 3:31 PM, David Collins <[email protected]> wrote:
>>> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
>>> +{
>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>> +
>>> + return vreg->enabled;
>>
>> You can't read hardware? The regulator framework expects you to read
>> hardware. If it was common not to be able to read hardware then the
>> regulator framework would just keep track of the enabled state for
>> you. Right now if a regulator was left on by the BIOS (presumably
>> some have to be since otherwise you're running on a computer that
>> takes no power), you'll still return false at bootup? Seems
>> non-ideal.
>
> Correct, the RPMh interface provides no mechanism to poll the physical
> PMIC regulator hardware state.
>
> However, that isn't entirely a bad thing due to the nature of RPMh
> regulator control. Several processors within the SoC besides the
> application processor are able to vote on the state of PMIC regulators via
> RPMh. The VRM and XOB RPMh hardware blocks perform max aggregation across
> votes from different processors for each votable quantity for a given
> regulator (enable, voltage, mode, and headroom voltage for VRM; enable for
> XOB). The aggregated values are then configured in the PMIC regulator via
> SPMI transactions.
>
> If Linux could read the physical PMIC regulator state and report it back
> via is_enabled(), get_voltage() and get_mode() regulator ops, then we
> could get into situations where the applications processor fails to vote
> on a regulator which is requested by a Linux consumer. For example, say
> that the modem processor has requested a given regulator to be enabled via
> RPMh. Later, a Linux consumer driver on the application processor calls
> regulator_enable() for the regulator. The regulator framework will call
> the is_enabled() callback which will return true. This would then
> short-circuit the regulator_enable() call and skip calling the
> rpmh-regulator enable() callback. This means that the application
> processor would have no enable vote present within RPMh for the regulator.
> If the modem processor then voted to disable the regulator, it would
> physically be disabled even though a consumer on the application processor
> expects it to stay enabled.

How about if I say it another way: can you read the vote that the boot
code (pre-Linux) might have left for this regulator? ...if the boot
code didn't init it, then I guess you could return 0, but if it did it
would be nice to report it.

AKA: I understand your concerns about other (non-AP) users. I just
want to read the state that the BIOS left us in.

>>> +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
>>> + int min_uv, int max_uv, unsigned int *selector)
>>> +{
>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>> + struct tcs_cmd cmd = {
>>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
>>> + };
>>> + const struct regulator_linear_range *range;
>>> + int mv, uv, ret;
>>> + bool wait_for_ack;
>>> +
>>> + mv = DIV_ROUND_UP(min_uv, 1000);
>>> + uv = mv * 1000;
>>> + if (uv > max_uv) {
>>> + vreg_err(vreg, "no set points available in range %d-%d uV\n",
>>> + min_uv, max_uv);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + range = vreg->hw_data->voltage_range;
>>> + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step);
>>
>> It seems like you should remove the existing check you have for "if
>> (uv > max_uv)" and replace with a check here. Specifically, it seems
>> like the DIV_ROUND_UP for the selector could also bump you over the
>> max. AKA:
>>
>> ...
>> bool wait_for_ack;
>>
>> range = vreg->hw_data->voltage_range;
>> *selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step);
>> uv = *selector * range->uV_step + range->min_uV
>> if (uv > max_uv) {
>> ...
>> }
>
> The VRM voltage vote register has units of mV (as does the voltage control
> register for the PMIC regulators). The PMIC hardware automatically rounds
> up the mV requests to the next physically available set point. This was
> designed to simplify the life for software so that there isn't a need to
> worry about voltage step size which varies between regulator types.
>
> The intention of the mv check was to verify that what is being programmed
> into VRM is within the requested min_uv-max_uv range. However, since this
> driver is forced to be aware of the step size, I can modify this as you
> suggested with the stricter step-size bounding.
>
>
>> Hold up, though. Why don't you implement set_voltage_sel() instead of
>> set_voltage()? That's what literally everyone else does, well except
>> PWM regulators. Using that will get rid of most of this code, won't
>> it? Even the check to see if perhaps the voltage isn't changing.
>
> There are two cases that I can think of: 1. Having a set_voltage()
> callback allows for delaying for an RPMh request ACK during certain
> voltage set point decreasing scenarios (to be elaborated below).

Can't you still have a delay in set_voltage_sel()?

> 2.
> Having a get_voltage() as opposed to get_voltage_sel() callback allows an
> uninitialized voltage of 0 to be returned in the case that no initial
> voltage is specified in device tree. This eliminates the possibility of
> the regulator framework short-circuiting a regulator_set_voltage() call
> that happens to match the voltage corresponding to selector 0.

Interesting. I suppose you could mix them (have set_voltage_sel() and
get_voltage()) as long as you documented why you were doing it. Then
we'd have to see if Mark was happy with that...

>>> +
>>> + if (uv == vreg->voltage)
>>> + return 0;
>>> +
>>> + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage;
>>
>> Do you often see "wait_for_ack = false" in reality? Most regulator
>> usage I've seen requests a fairly tight range. AKA: I don't often
>> see:
>>
>> set_voltage(min=3000mV, max=3300mV)
>> set_voltage(min=1800mV, max=3300mV)
>
> I can't think of a good example off the top of my head. However, this
> situation is certainly valid and can arise any time that there is a shared
> rail and the hardware connected to it has minimum voltage requirements per
> use case but large maximum voltage allowed.

Yup. I'm not saying it's not allowed, it's just not something I've
come across so far. ...but (assuming I read the code correctly) this
is the only case your "wait_for_ack" was optimizing for. I don't mind
having the optimization if it's needed, I just don't want it if there
are no known users.

>> Instead, I see:
>>
>> set_voltage(min=3000mV, max=3300mV)
>> set_voltage(min=1800mV, max=1900mV)
>>
>> So you'll always have wait_for_ack = true in the cases I've seen.
>>
>> ...but are you certain it's useful to wait for an ack anyway when the
>> voltage is falling? Most regulators won't guarantee that the voltage
>> has actually fallen even after they ack you. Specifically if a
>> regulator is under light load and it doesn't have an active discharge
>> circuit then the regulator might fall very slowly over time. As a
>> specific example, see commit 7c5209c315ea ("mmc: core: Increase delay
>> for voltage to stabilize from 3.3V to 1.8V").
>
> I'm aware of one important instance in which decreasing voltage needs a
> delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case
> that I had in mind with the 'max_uv < vreg->voltage' check. However, you
> are correct that hardware will report completion of the voltage slewing
> early in this case since the comparator is checking for output >= set
> point. I can remove this special case check.
>
>
>> That was a lot of words, so to sum it all up:
>>
>> * If you have no actual examples where you see "wait_for_ack = false"
>> then remove this code and always wait.
>>
>> * If you have evidence that the time spent waiting for the ack is
>> slowing you down, consider always setting wait_for_ack to false when
>> you're lowering the voltage. Anyone who truly cares could just set
>> something like the device tree property
>> regulator-settling-time-down-us. ...or, assuming Mark doesn't hate
>> it, they could set the always-wait-for-ack property in the device
>> tree.
>
> I'm ok with changing the logic so that it waits for an ACK when increasing
> voltage (required for correctness) and does not wait for an ACK when
> decreasing voltage.

Sounds fine with me. You could even add a comment saying that waiting
for an Ack when falling isn't so useful unless the line has some sort
of active discharge. You've already got a device tree feature for
this, so it would be a good to document it there too.

We'll also have to see if Mark wants this to be more generic in some
way. It seems that other regulators could benefit from this type of
knowledge and it would be nice if drivers could specify this and have
it work on all PMICs.

>> NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy
>> ARC things for this?), we're probably talking about a small handful of
>> voltage transitions per boot, right?
>
> VRM isn't used for CPU DVFS. I haven't profiled the quantity of RPMh
> regulator requests made during different use cases.
>
>
>>> +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev)
>>> +{
>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>> +
>>> + return vreg->voltage;
>>
>> I guess there's no way to read the voltage at bootup? So this will
>> return 0 until someone sets it? ...maybe less of a big deal due to
>> the "qcom,regulator-initial-voltage" property?
>
> Correct. As mentioned above, there is no way to read the physical PMIC
> regulator voltage from RPMh. Yes, this will return 0 unless
> qcom,regulator-initial-voltage is specified or the set_voltage() regulator
> op is called. Returning an invalid voltage of 0 uV here is convenient as
> it ensures that there is no way to accidentally skip setting the voltage
> due to returning the voltage of selector 0.

I actually wonder if it would be wiser to return an error code if you
try to read a regulator that has never been set.

>>> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
>>> + unsigned int mode)
>>> +{
>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>> + struct tcs_cmd cmd = {
>>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>>> + };
>>> + int i, ret;
>>> +
>>> + if (mode == vreg->mode)
>>> + return 0;
>>> +
>>> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
>>> + if (vreg->hw_data->mode_map[i].framework_mode == mode)
>>> + break;
>>> + if (i >= RPMH_REGULATOR_MODE_COUNT) {
>>> + vreg_err(vreg, "invalid mode=%u\n", mode);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + cmd.data = vreg->hw_data->mode_map[i].pmic_mode;
>>> +
>>> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
>>> + mode < vreg->mode || !vreg->mode);
>>
>> Please explain the "mode < vreg->mode || !vreg->mode" test in words.
>
> This waits for an ACK from RPMh of successfully mode transition when
> switching to a higher power mode (regulator framework modes are ordered
> with highest power == lowest numerical value) or when setting the first mode.

Sorry, I meant: please add this explanation to comments in the function.

>> IMHO it would be better to have a table like "dt_to_linux_mode" that
>> was just a simple list:
>>
>> static const int dt_to_linux_mode_bob[] = [
>> [RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY,
>> [RPMH_REGULATOR_MODE_RET] = -EINVAL,
>> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
>> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
>> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
>> ];
>>
>> static const int dt_to_linux_mode_ldo_smps[] =
>> [RPMH_REGULATOR_MODE_PASS] = -EINVAL,
>> [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
>> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
>> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
>> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
>> ];
>>
>> You'd only use that in the "map_mode" functions and when parsing
>> qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could
>> actually just call the map_mode function. This would be especially a
>> good way to do this if you moved "allowed-modes" into the regulator
>> core, which seems like a good idea.
>>
>> The nice thing about this is that only place you need to conceptually
>> keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing
>> code. Otherwise you just think of the Linux version.
>
> I would be ok with defining arrays like these to handle the mapping of
> PMIC regulator hardware agnostic "device tree mode" to "framework mode".
> However, a second set of arrays would then be needed to map from
> "framework mode" to hardware specific "pmic mode". I suppose that having
> the second array indexed by framework mode would make the code a little
> simpler since iteration would not be needed.

Yeah, I was thinking that with 2 arrays the code would be more obvious.

> Here is an explanation for why the "device tree mode" abstraction is
> present in the first place. Between different Qualcomm Technologies, Inc.
> PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO,
> LPM/PFM, Retention, and pass-through). However, the register values for
> the same modes vary between different regulator types and different PMIC
> families. This patch is adding support for several PMIC4 family PMICs.
> The values needed for to-be-released PMIC5 PMIC regulators are different.
> As an example, here are the different values for LPM/PFM across PMIC
> families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5
> LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode"
> ensures that it is not possible to inadvertently specify a PMIC specific
> mode in device tree which corresponds to the wrong type or family but
> which aliases a value that would be accepted as correct.

I'm OK with having the "device tree mode" abstraction, and in fact the
current regulator framework seems to want you to have this anyway. If
I read the code correctly, you're required to have the conversion
function and there's no default.

>> Once you do the above, then your other list could just be named
>> "allowed_modes". This would make it obvious that this isn't a map
>> itself but that you could iterate over it to accomplish a mapping.
>
> I'm concerned with using the name "allowed_modes" as it tends to imply the
> wrong idea. Perhaps something along the lines of "drms_modes" would be
> better (to go along with REGULATOR_CHANGE_DRMS). These would be the modes
> utilized by the set_load() callback along with corresponding threshold
> load currents.

Yeah, that name is OK by me. At least I can lookup in the header file
to find out that DRMS means "Dynamic Regulator Mode Switching". :)

>>> +static const struct rpmh_regulator_mode
>>> +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = {
>>> + [RPMH_REGULATOR_MODE_PASS] = {
>>> + .pmic_mode = 0,
>>> + .framework_mode = REGULATOR_MODE_STANDBY,
>>
>> Is "PASS" truly the same concept as the Linux concept of STANDBY. If
>> so, why do you need a separate define for it?
>>
>> If it truly is the same, it seems like you can simplify everything by
>> just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and
>> "RPMH_REGULATOR_MODE_PASS" and just call it
>> "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that
>> "standby" maps to "retention" for some regulators and maps to "pass"
>> for other regulators if you want to map PMIC documentation. ...but
>> getting rid of this distinction simply means less error checking and
>> fewer tables in Linux.
>>
>> If "pass" really shouldn't map to "standby" then this seems like a
>> hack and you should add the concept of "pass" to the core regulator
>> framework.
>
> For Qualcomm Technologies, Inc. PMIC regulators, retention mode (RET) is a
> very low power mode which offers better power effeciency (i.e. lower
> ground current) than low power mode (LPM) for low load scenarios with the
> trade off of worse load regulator. It is typically applied when the
> system is asleep for regulators that must stay on but which have very low
> and stable load. Retention maps very well to REGULATOR_MODE_STANDBY.

Yup, this seems like a perfect mapping between the Qualcomm mode and
the regulator framework.

> Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm
> Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected,
> the BOB output is directly connected to the BOB input (typically Vbat).
> This does not map exactly to REGULATOR_MODE_STANDBY. I agree that it is
> somewhat hacky to use it in this way. However, doing so makes
> qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode
> could be handled via get_bypass() and set_bypass() regulator ops. Doing
> this would require more complicated ops selections in the driver since it
> could no longer be determined simply by VRM vs XOB, it would instead need
> to be BOB VRM, other VRM, and XOB. The BOB set_mode() and set_bypass()
> callbacks would be complicated because both would be writing to the same
> VRM address (mode control) with bypass==true taking precedence.
> Additionally, there is currently no way to specify default bypass from DT.
> A BOB-specific property would be needed to get this information.

I've never poked at the get_bypass() / set_bypass(), but it sounds
better to me to use them. I'm not a fan of the current hack. Even
aside from the bit of hackiness, I'm slightly concerned that some of
your logic that generally assumes lower integers = lower power modes
would break.

For instance in rpmh_regulator_vrm_set_mode() switching to "PASS"
would look like you're switching to a low power mode so you'd skip the
"wait for ack", right? I could sorta imagine things getting confused
when trying to specify the mA load and having it switch modes
automatically.

I'd also notice that "regulator/qcom_spmi-regulator.c" seems to be
using get_bypass() / set_bypass(), so maybe qcom-related clients will
expect this?

>>> + },
>>> + .probe = rpmh_regulator_probe,
>>> + .remove = rpmh_regulator_remove,
>>> +};
>>> +
>>> +static int rpmh_regulator_init(void)
>>> +{
>>> + return platform_driver_register(&rpmh_regulator_driver);
>>> +}
>>> +
>>> +static void rpmh_regulator_exit(void)
>>> +{
>>> + platform_driver_unregister(&rpmh_regulator_driver);
>>> +}
>>> +
>>> +arch_initcall(rpmh_regulator_init);
>>
>> I always get yelled at when I try to use arch_initcall() for stuff
>> like this. You should do what everyone else does and use
>> module_platform_driver() to declare your driver. Yeah, regulators are
>> important and (as I remember) they get probed slightly early anyway,
>> but everything else in the system just gotta deal with the fact that
>> they'll sometimes get deferred probes.
>
> I agree that consumers should handle probe deferral. Unfortunately,
> reality isn't always so nice. Also, probe deferrals increase boot-up time
> which is particularly problematic in the mobile space.

Sigh, yeah. I'm not a fan either. If you can convince Mark that you
should use arch_initcall() or subsys_initcall() I won't yell. ...but
in the past I've seen others get yelled at.

Note: in actuality it doesn't always increase boot time a whole lot.
In theory as long as the CPU is running full bore and you're not
killing parallelism too much then the only "waste" is the bit of time
to run the start of the probe. It's not much. Every time someone
claims "but my boot is slow because of deferrals" others say "where is
the evidence?" and I don't remember a whole lot of evidence being
presented. Maybe you have evidence?

...but deferrals _do_ for sure increase the time for certain
peripherals to come up, and if those peripherals are things like the
LCD displays then it sucks.

> I suppose that I
> can change this to module_platform_driver() for now. If any major issues
> arise it could be changed back to arch_initcall().

Probably wise.

> Note that both qcom_rpm-regulator.c and qcom_smd-regulator.c are using
> subsys_initcall() right now in place of module_platform_driver().

Yeah, I've made the argument before and been told "they are grandfathered in."

-Doug

2018-03-26 15:36:58

by Lina Iyer

[permalink] [raw]

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

On Thu, Mar 22 2018 at 19:30 -0600, David Collins wrote:
>Hello Stephen,
>
>Thank you for the very detailed review feedback.
>
>On 03/21/2018 12:07 PM, Stephen Boyd wrote:
>> Quoting David Collins (2018-03-16 18:09:10)

>>> +static int rpmh_regulator_remove(struct platform_device *pdev)
>>> +{
>>> + struct rpmh_pmic *pmic = platform_get_drvdata(pdev);
>>> +
>>> + rpmh_release(pmic->rpmh_client);
>>
>> I'm still lost on what rpmh_client is giving us besides more code we
>> don't need. I'll ping the rpmh thread again.
>
>Let's see if Lina is willing to add some devm_* calls so that no cleanup
>is required.
>
I will look into this.

-- Lina

2018-03-27 11:57:58

[permalink] [raw]

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

On Fri, Mar 23, 2018 at 01:00:47PM -0700, Doug Anderson wrote:
> On Thu, Mar 22, 2018 at 3:31 PM, David Collins <[email protected]> wrote:

> > There are two cases that I can think of: 1. Having a set_voltage()
> > callback allows for delaying for an RPMh request ACK during certain
> > voltage set point decreasing scenarios (to be elaborated below).

> Can't you still have a delay in set_voltage_sel()?

We have specific support for adding ramp delays, no need to open code it
in operations.

> > 2.
> > Having a get_voltage() as opposed to get_voltage_sel() callback allows an
> > uninitialized voltage of 0 to be returned in the case that no initial
> > voltage is specified in device tree. This eliminates the possibility of
> > the regulator framework short-circuiting a regulator_set_voltage() call
> > that happens to match the voltage corresponding to selector 0.

> Interesting. I suppose you could mix them (have set_voltage_sel() and
> get_voltage()) as long as you documented why you were doing it. Then
> we'd have to see if Mark was happy with that...

This is a *terrible* idea which will almost certainly break. If the
driver can't read values it should return an appropriate error code.

> > I'm aware of one important instance in which decreasing voltage needs a
> > delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case
> > that I had in mind with the 'max_uv < vreg->voltage' check. However, you
> > are correct that hardware will report completion of the voltage slewing
> > early in this case since the comparator is checking for output >= set
> > point. I can remove this special case check.

You can't usefully wait for voltages to fall, you can never guarantee
what the loading on the device is. It's something the user has to
manage if they care.

> > Here is an explanation for why the "device tree mode" abstraction is
> > present in the first place. Between different Qualcomm Technologies, Inc.
> > PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO,
> > LPM/PFM, Retention, and pass-through). However, the register values for
> > the same modes vary between different regulator types and different PMIC
> > families. This patch is adding support for several PMIC4 family PMICs.
> > The values needed for to-be-released PMIC5 PMIC regulators are different.
> > As an example, here are the different values for LPM/PFM across PMIC
> > families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5
> > LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode"
> > ensures that it is not possible to inadvertently specify a PMIC specific
> > mode in device tree which corresponds to the wrong type or family but
> > which aliases a value that would be accepted as correct.

> I'm OK with having the "device tree mode" abstraction, and in fact the
> current regulator framework seems to want you to have this anyway. If
> I read the code correctly, you're required to have the conversion
> function and there's no default.

I didn't spot this in the code but something called "device tree mode"
sounds like it's going to be awfully confusing...

> > Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm
> > Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected,
> > the BOB output is directly connected to the BOB input (typically Vbat).

...

> > qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode
> > could be handled via get_bypass() and set_bypass() regulator ops. Doing
> > this would require more complicated ops selections in the driver since it

This is exactly the functionality supported by the bypass operations.
Any complexity due to the hardware design is unfortunate but honestly
the way the QC regulator stuff is designed they seem like a bit of a
lost cause on that front - they look very different to any other
hardware we've seen.

> I've never poked at the get_bypass() / set_bypass(), but it sounds
> better to me to use them. I'm not a fan of the current hack. Even
> aside from the bit of hackiness, I'm slightly concerned that some of
> your logic that generally assumes lower integers = lower power modes
> would break.

Yes, abusing the framework is just going to make things even worse.

> >>> +arch_initcall(rpmh_regulator_init);

> >> I always get yelled at when I try to use arch_initcall() for stuff
> >> like this. You should do what everyone else does and use

> > I agree that consumers should handle probe deferral. Unfortunately,
> > reality isn't always so nice. Also, probe deferrals increase boot-up time
> > which is particularly problematic in the mobile space.

> Sigh, yeah. I'm not a fan either. If you can convince Mark that you
> should use arch_initcall() or subsys_initcall() I won't yell. ...but
> in the past I've seen others get yelled at.

Do you have concrete consumers that have a good reason for doing this?

> Note: in actuality it doesn't always increase boot time a whole lot.

Note also that we now have the device dependency mechanism that Raphael
implemented with the explicit idea that that it'd be used to avoid
unneeded deferrals.

> ...but deferrals _do_ for sure increase the time for certain
> peripherals to come up, and if those peripherals are things like the
> LCD displays then it sucks.

There's been some discussion of allowing the user to specific certain
devices to target as priorities for probing which should deal with that.

Attachments:

(No filename) (5.45 kB)
signature.asc (499.00 B)
Download all attachments

2018-03-27 20:53:44

[permalink] [raw]

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

Hi,

On Tue, Mar 27, 2018 at 4:56 AM, Mark Brown <[email protected]> wrote:
>> > Here is an explanation for why the "device tree mode" abstraction is
>> > present in the first place. Between different Qualcomm Technologies, Inc.
>> > PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO,
>> > LPM/PFM, Retention, and pass-through). However, the register values for
>> > the same modes vary between different regulator types and different PMIC
>> > families. This patch is adding support for several PMIC4 family PMICs.
>> > The values needed for to-be-released PMIC5 PMIC regulators are different.
>> > As an example, here are the different values for LPM/PFM across PMIC
>> > families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5
>> > LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode"
>> > ensures that it is not possible to inadvertently specify a PMIC specific
>> > mode in device tree which corresponds to the wrong type or family but
>> > which aliases a value that would be accepted as correct.
>
>> I'm OK with having the "device tree mode" abstraction, and in fact the
>> current regulator framework seems to want you to have this anyway. If
>> I read the code correctly, you're required to have the conversion
>> function and there's no default.
>
> I didn't spot this in the code but something called "device tree mode"
> sounds like it's going to be awfully confusing...

Just to clarify this bit: The regulator framework allows for a
callback function of_map_mode() in regulator drivers. My reading of
the code shows that if you wish to use the property
"regulator-initial-mode" (like this regulator does) that it is
_required_ to provide an of_map_mode() function.

Assuming I didn't mess up my analysis, the entire job of of_map_mode()
is to convert from one integer to another. It should take the number
that was specified in the device tree and convert it to a
REGULATOR_MODE_XXX. That means that the regulator framework is
enforcing a distinct and per-regulator numbering system for the mode
(I called this "device tree mode").

Just for kicks, right now these are the REGULATOR_MODEs:

#define REGULATOR_MODE_FAST 0x1
#define REGULATOR_MODE_NORMAL 0x2
#define REGULATOR_MODE_IDLE 0x4
#define REGULATOR_MODE_STANDBY 0x8

--

In cpcap_map_mode():

0 (CPCAP_BIT_AUDIO_NORMAL_MODE) => 0x2 (normal)
0x40 (CPCAP_BIT_AUDIO_LOW_PWR) ==> 0x8 (standby)
else => error

--

In max77802_map_mode():

3 (MAX77802_OPMODE_NORMAL) ==> 0x2 (normal)
else (intends 1 or MAX77802_OPMODE_LP) ==> 0x8 (standby)

--

In spmi_regulator_of_map_mode():

1 => 0x2 (normal)
2 => 0x1 (fast)
else => 0x4 (idle)

--

In twl4030reg_map_mode():

0xe (RES_STATE_ACTIVE) => 0x2 (normal)
0x8 (RES_STATE_SLEEP) => 0x8 (standby)
else => error

--

So basically it sounds like everyone makes up some arbitrary numbering
system that is only used in their device tree files and needs to be
mapped into the standard numbering system...

Perhaps the right answer is to improve the regulator core to make
of_map_mode() optional? We'd have to figure out where to place the
#defines for the default of_map_mode() so they could be used in .dts
files, but that should be possible. Presumably it would be wise to
add a new property that was a bitmask of valid modes...

-Doug

2018-03-27 23:23:46

[permalink] [raw]

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

On 03/23/2018 01:00 PM, Doug Anderson wrote:
> On Thu, Mar 22, 2018 at 3:31 PM, David Collins <[email protected]> wrote:
>>>> +static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
>>>> +{
>>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>>> +
>>>> + return vreg->enabled;
>>>
>>> You can't read hardware? The regulator framework expects you to read
>>> hardware. If it was common not to be able to read hardware then the
>>> regulator framework would just keep track of the enabled state for
>>> you. Right now if a regulator was left on by the BIOS (presumably
>>> some have to be since otherwise you're running on a computer that
>>> takes no power), you'll still return false at bootup? Seems
>>> non-ideal.
>>
>> Correct, the RPMh interface provides no mechanism to poll the physical
>> PMIC regulator hardware state.
>>
>> However, that isn't entirely a bad thing due to the nature of RPMh
>> regulator control. Several processors within the SoC besides the
>> application processor are able to vote on the state of PMIC regulators via
>> RPMh. The VRM and XOB RPMh hardware blocks perform max aggregation across
>> votes from different processors for each votable quantity for a given
>> regulator (enable, voltage, mode, and headroom voltage for VRM; enable for
>> XOB). The aggregated values are then configured in the PMIC regulator via
>> SPMI transactions.
>>
>> If Linux could read the physical PMIC regulator state and report it back
>> via is_enabled(), get_voltage() and get_mode() regulator ops, then we
>> could get into situations where the applications processor fails to vote
>> on a regulator which is requested by a Linux consumer. For example, say
>> that the modem processor has requested a given regulator to be enabled via
>> RPMh. Later, a Linux consumer driver on the application processor calls
>> regulator_enable() for the regulator. The regulator framework will call
>> the is_enabled() callback which will return true. This would then
>> short-circuit the regulator_enable() call and skip calling the
>> rpmh-regulator enable() callback. This means that the application
>> processor would have no enable vote present within RPMh for the regulator.
>> If the modem processor then voted to disable the regulator, it would
>> physically be disabled even though a consumer on the application processor
>> expects it to stay enabled.
>
> How about if I say it another way: can you read the vote that the boot
> code (pre-Linux) might have left for this regulator? ...if the boot
> code didn't init it, then I guess you could return 0, but if it did it
> would be nice to report it.
>
> AKA: I understand your concerns about other (non-AP) users. I just
> want to read the state that the BIOS left us in.

Unfortunately, this is not possible either. The RPMh interface is write-only.

>>>> +static int rpmh_regulator_vrm_set_voltage(struct regulator_dev *rdev,
>>>> + int min_uv, int max_uv, unsigned int *selector)
>>>> +{
>>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>>> + struct tcs_cmd cmd = {
>>>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
>>>> + };
>>>> + const struct regulator_linear_range *range;
>>>> + int mv, uv, ret;
>>>> + bool wait_for_ack;
>>>> +
>>>> + mv = DIV_ROUND_UP(min_uv, 1000);
>>>> + uv = mv * 1000;
>>>> + if (uv > max_uv) {
>>>> + vreg_err(vreg, "no set points available in range %d-%d uV\n",
>>>> + min_uv, max_uv);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + range = vreg->hw_data->voltage_range;
>>>> + *selector = DIV_ROUND_UP(uv - range->min_uV, range->uV_step);
>>>
>>> It seems like you should remove the existing check you have for "if
>>> (uv > max_uv)" and replace with a check here. Specifically, it seems
>>> like the DIV_ROUND_UP for the selector could also bump you over the
>>> max. AKA:
>>>
>>> ...
>>> bool wait_for_ack;
>>>
>>> range = vreg->hw_data->voltage_range;
>>> *selector = DIV_ROUND_UP(min_uv - range->min_uV, range->uV_step);
>>> uv = *selector * range->uV_step + range->min_uV
>>> if (uv > max_uv) {
>>> ...
>>> }
>>
>> The VRM voltage vote register has units of mV (as does the voltage control
>> register for the PMIC regulators). The PMIC hardware automatically rounds
>> up the mV requests to the next physically available set point. This was
>> designed to simplify the life for software so that there isn't a need to
>> worry about voltage step size which varies between regulator types.
>>
>> The intention of the mv check was to verify that what is being programmed
>> into VRM is within the requested min_uv-max_uv range. However, since this
>> driver is forced to be aware of the step size, I can modify this as you
>> suggested with the stricter step-size bounding.
>>
>>
>>> Hold up, though. Why don't you implement set_voltage_sel() instead of
>>> set_voltage()? That's what literally everyone else does, well except
>>> PWM regulators. Using that will get rid of most of this code, won't
>>> it? Even the check to see if perhaps the voltage isn't changing.
>>
>> There are two cases that I can think of: 1. Having a set_voltage()
>> callback allows for delaying for an RPMh request ACK during certain
>> voltage set point decreasing scenarios (to be elaborated below).
>
> Can't you still have a delay in set_voltage_sel()?

The same behavior cannot be achieved with set_voltage_sel as it is only
aware of a single selector value, not a min to max allowed consumer range.
However, since the driver can't guarantee that the voltage will have
actually slewed down due to potentially high output capacitance and low
load, this case can be safely ignored. I'll switch to a set_voltage_sel()
callback.

>> 2.
>> Having a get_voltage() as opposed to get_voltage_sel() callback allows an
>> uninitialized voltage of 0 to be returned in the case that no initial
>> voltage is specified in device tree. This eliminates the possibility of
>> the regulator framework short-circuiting a regulator_set_voltage() call
>> that happens to match the voltage corresponding to selector 0.
>
> Interesting. I suppose you could mix them (have set_voltage_sel() and
> get_voltage()) as long as you documented why you were doing it. Then
> we'd have to see if Mark was happy with that...

I'll switch to using get_voltage_sel().

>>>> +
>>>> + if (uv == vreg->voltage)
>>>> + return 0;
>>>> +
>>>> + wait_for_ack = uv > vreg->voltage || max_uv < vreg->voltage;
>>>
>>> Do you often see "wait_for_ack = false" in reality? Most regulator
>>> usage I've seen requests a fairly tight range. AKA: I don't often
>>> see:
>>>
>>> set_voltage(min=3000mV, max=3300mV)
>>> set_voltage(min=1800mV, max=3300mV)
>>
>> I can't think of a good example off the top of my head. However, this
>> situation is certainly valid and can arise any time that there is a shared
>> rail and the hardware connected to it has minimum voltage requirements per
>> use case but large maximum voltage allowed.
>
> Yup. I'm not saying it's not allowed, it's just not something I've
> come across so far. ...but (assuming I read the code correctly) this
> is the only case your "wait_for_ack" was optimizing for. I don't mind
> having the optimization if it's needed, I just don't want it if there
> are no known users.

I'll change it to be simply: increasing voltage: wait for ACK (required),
decreasing voltage: don't wait for ACK (potential performance optimization).

>>> Instead, I see:
>>>
>>> set_voltage(min=3000mV, max=3300mV)
>>> set_voltage(min=1800mV, max=1900mV)
>>>
>>> So you'll always have wait_for_ack = true in the cases I've seen.
>>>
>>> ...but are you certain it's useful to wait for an ack anyway when the
>>> voltage is falling? Most regulators won't guarantee that the voltage
>>> has actually fallen even after they ack you. Specifically if a
>>> regulator is under light load and it doesn't have an active discharge
>>> circuit then the regulator might fall very slowly over time. As a
>>> specific example, see commit 7c5209c315ea ("mmc: core: Increase delay
>>> for voltage to stabilize from 3.3V to 1.8V").
>>
>> I'm aware of one important instance in which decreasing voltage needs a
>> delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case
>> that I had in mind with the 'max_uv < vreg->voltage' check. However, you
>> are correct that hardware will report completion of the voltage slewing
>> early in this case since the comparator is checking for output >= set
>> point. I can remove this special case check.
>>
>>
>>> That was a lot of words, so to sum it all up:
>>>
>>> * If you have no actual examples where you see "wait_for_ack = false"
>>> then remove this code and always wait.
>>>
>>> * If you have evidence that the time spent waiting for the ack is
>>> slowing you down, consider always setting wait_for_ack to false when
>>> you're lowering the voltage. Anyone who truly cares could just set
>>> something like the device tree property
>>> regulator-settling-time-down-us. ...or, assuming Mark doesn't hate
>>> it, they could set the always-wait-for-ack property in the device
>>> tree.
>>
>> I'm ok with changing the logic so that it waits for an ACK when increasing
>> voltage (required for correctness) and does not wait for an ACK when
>> decreasing voltage.
>
> Sounds fine with me. You could even add a comment saying that waiting
> for an Ack when falling isn't so useful unless the line has some sort
> of active discharge. You've already got a device tree feature for
> this, so it would be a good to document it there too.
>
> We'll also have to see if Mark wants this to be more generic in some
> way. It seems that other regulators could benefit from this type of
> knowledge and it would be nice if drivers could specify this and have
> it work on all PMICs.

As Mark pointed out, this case can't really be handled by the driver since
the regulator could be lightly loaded. Therefore, it is safe to ignore
and simply use set_voltage_sel().

>>> NOTE: I think you don't use VRMs for DVFS anyway (you use the fancy
>>> ARC things for this?), we're probably talking about a small handful of
>>> voltage transitions per boot, right?
>>
>> VRM isn't used for CPU DVFS. I haven't profiled the quantity of RPMh
>> regulator requests made during different use cases.
>>
>>
>>>> +static int rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev)
>>>> +{
>>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>>> +
>>>> + return vreg->voltage;
>>>
>>> I guess there's no way to read the voltage at bootup? So this will
>>> return 0 until someone sets it? ...maybe less of a big deal due to
>>> the "qcom,regulator-initial-voltage" property?
>>
>> Correct. As mentioned above, there is no way to read the physical PMIC
>> regulator voltage from RPMh. Yes, this will return 0 unless
>> qcom,regulator-initial-voltage is specified or the set_voltage() regulator
>> op is called. Returning an invalid voltage of 0 uV here is convenient as
>> it ensures that there is no way to accidentally skip setting the voltage
>> due to returning the voltage of selector 0.
>
> I actually wonder if it would be wiser to return an error code if you
> try to read a regulator that has never been set.

I'll try this out and see if the regulator framework complains during
regulator registration.

>>>> +static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
>>>> + unsigned int mode)
>>>> +{
>>>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>>> + struct tcs_cmd cmd = {
>>>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>>>> + };
>>>> + int i, ret;
>>>> +
>>>> + if (mode == vreg->mode)
>>>> + return 0;
>>>> +
>>>> + for (i = 0; i < RPMH_REGULATOR_MODE_COUNT; i++)
>>>> + if (vreg->hw_data->mode_map[i].framework_mode == mode)
>>>> + break;
>>>> + if (i >= RPMH_REGULATOR_MODE_COUNT) {
>>>> + vreg_err(vreg, "invalid mode=%u\n", mode);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + cmd.data = vreg->hw_data->mode_map[i].pmic_mode;
>>>> +
>>>> + ret = rpmh_regulator_send_request(vreg, &cmd, 1,
>>>> + mode < vreg->mode || !vreg->mode);
>>>
>>> Please explain the "mode < vreg->mode || !vreg->mode" test in words.
>>
>> This waits for an ACK from RPMh of successfully mode transition when
>> switching to a higher power mode (regulator framework modes are ordered
>> with highest power == lowest numerical value) or when setting the first mode.
>
> Sorry, I meant: please add this explanation to comments in the function.

Sure, I'll add a comment for it.

>>> IMHO it would be better to have a table like "dt_to_linux_mode" that
>>> was just a simple list:
>>>
>>> static const int dt_to_linux_mode_bob[] = [
>>> [RPMH_REGULATOR_MODE_PASS] = REGULATOR_MODE_STANDBY,
>>> [RPMH_REGULATOR_MODE_RET] = -EINVAL,
>>> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
>>> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
>>> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
>>> ];
>>>
>>> static const int dt_to_linux_mode_ldo_smps[] =
>>> [RPMH_REGULATOR_MODE_PASS] = -EINVAL,
>>> [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
>>> [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
>>> [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
>>> [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST
>>> ];
>>>
>>> You'd only use that in the "map_mode" functions and when parsing
>>> qcom,allowed-modes. ...and, in fact, parsing qcom,allowed-modes could
>>> actually just call the map_mode function. This would be especially a
>>> good way to do this if you moved "allowed-modes" into the regulator
>>> core, which seems like a good idea.
>>>
>>> The nice thing about this is that only place you need to conceptually
>>> keep track of RPMH_REGULATOR_MODE_XYZ is in the device tree parsing
>>> code. Otherwise you just think of the Linux version.
>>
>> I would be ok with defining arrays like these to handle the mapping of
>> PMIC regulator hardware agnostic "device tree mode" to "framework mode".
>> However, a second set of arrays would then be needed to map from
>> "framework mode" to hardware specific "pmic mode". I suppose that having
>> the second array indexed by framework mode would make the code a little
>> simpler since iteration would not be needed.
>
> Yeah, I was thinking that with 2 arrays the code would be more obvious.

I'll make this modification.

>> Here is an explanation for why the "device tree mode" abstraction is
>> present in the first place. Between different Qualcomm Technologies, Inc.
>> PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO,
>> LPM/PFM, Retention, and pass-through). However, the register values for
>> the same modes vary between different regulator types and different PMIC
>> families. This patch is adding support for several PMIC4 family PMICs.
>> The values needed for to-be-released PMIC5 PMIC regulators are different.
>> As an example, here are the different values for LPM/PFM across PMIC
>> families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5
>> LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode"
>> ensures that it is not possible to inadvertently specify a PMIC specific
>> mode in device tree which corresponds to the wrong type or family but
>> which aliases a value that would be accepted as correct.
>
> I'm OK with having the "device tree mode" abstraction, and in fact the
> current regulator framework seems to want you to have this anyway. If
> I read the code correctly, you're required to have the conversion
> function and there's no default.

I will continue to use this abstraction.

>>> Once you do the above, then your other list could just be named
>>> "allowed_modes". This would make it obvious that this isn't a map
>>> itself but that you could iterate over it to accomplish a mapping.
>>
>> I'm concerned with using the name "allowed_modes" as it tends to imply the
>> wrong idea. Perhaps something along the lines of "drms_modes" would be
>> better (to go along with REGULATOR_CHANGE_DRMS). These would be the modes
>> utilized by the set_load() callback along with corresponding threshold
>> load currents.
>
> Yeah, that name is OK by me. At least I can lookup in the header file
> to find out that DRMS means "Dynamic Regulator Mode Switching". :)

I'll make this modification.

>>>> +static const struct rpmh_regulator_mode
>>>> +rpmh_regulator_mode_map_pmic4_bob[RPMH_REGULATOR_MODE_COUNT] = {
>>>> + [RPMH_REGULATOR_MODE_PASS] = {
>>>> + .pmic_mode = 0,
>>>> + .framework_mode = REGULATOR_MODE_STANDBY,
>>>
>>> Is "PASS" truly the same concept as the Linux concept of STANDBY. If
>>> so, why do you need a separate define for it?
>>>
>>> If it truly is the same, it seems like you can simplify everything by
>>> just changing your defines. Get rid of "RPMH_REGULATOR_MODE_RET" and
>>> "RPMH_REGULATOR_MODE_PASS" and just call it
>>> "RPMH_REGULATOR_MODE_STANDBY". You can add comments saying that
>>> "standby" maps to "retention" for some regulators and maps to "pass"
>>> for other regulators if you want to map PMIC documentation. ...but
>>> getting rid of this distinction simply means less error checking and
>>> fewer tables in Linux.
>>>
>>> If "pass" really shouldn't map to "standby" then this seems like a
>>> hack and you should add the concept of "pass" to the core regulator
>>> framework.
>>
>> For Qualcomm Technologies, Inc. PMIC regulators, retention mode (RET) is a
>> very low power mode which offers better power effeciency (i.e. lower
>> ground current) than low power mode (LPM) for low load scenarios with the
>> trade off of worse load regulator. It is typically applied when the
>> system is asleep for regulators that must stay on but which have very low
>> and stable load. Retention maps very well to REGULATOR_MODE_STANDBY.
>
> Yup, this seems like a perfect mapping between the Qualcomm mode and
> the regulator framework.
>
>
>> Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm
>> Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected,
>> the BOB output is directly connected to the BOB input (typically Vbat).
>> This does not map exactly to REGULATOR_MODE_STANDBY. I agree that it is
>> somewhat hacky to use it in this way. However, doing so makes
>> qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode
>> could be handled via get_bypass() and set_bypass() regulator ops. Doing
>> this would require more complicated ops selections in the driver since it
>> could no longer be determined simply by VRM vs XOB, it would instead need
>> to be BOB VRM, other VRM, and XOB. The BOB set_mode() and set_bypass()
>> callbacks would be complicated because both would be writing to the same
>> VRM address (mode control) with bypass==true taking precedence.
>> Additionally, there is currently no way to specify default bypass from DT.
>> A BOB-specific property would be needed to get this information.
>
> I've never poked at the get_bypass() / set_bypass(), but it sounds
> better to me to use them. I'm not a fan of the current hack. Even
> aside from the bit of hackiness, I'm slightly concerned that some of
> your logic that generally assumes lower integers = lower power modes
> would break.
>
> For instance in rpmh_regulator_vrm_set_mode() switching to "PASS"
> would look like you're switching to a low power mode so you'd skip the
> "wait for ack", right? I could sorta imagine things getting confused
> when trying to specify the mA load and having it switch modes
> automatically.
>
> I'd also notice that "regulator/qcom_spmi-regulator.c" seems to be
> using get_bypass() / set_bypass(), so maybe qcom-related clients will
> expect this?

I'll implement BOB PASS mode via get_bypass() and set_bypass() callbacks.

>>>> + },
>>>> + .probe = rpmh_regulator_probe,
>>>> + .remove = rpmh_regulator_remove,
>>>> +};
>>>> +
>>>> +static int rpmh_regulator_init(void)
>>>> +{
>>>> + return platform_driver_register(&rpmh_regulator_driver);
>>>> +}
>>>> +
>>>> +static void rpmh_regulator_exit(void)
>>>> +{
>>>> + platform_driver_unregister(&rpmh_regulator_driver);
>>>> +}
>>>> +
>>>> +arch_initcall(rpmh_regulator_init);
>>>
>>> I always get yelled at when I try to use arch_initcall() for stuff
>>> like this. You should do what everyone else does and use
>>> module_platform_driver() to declare your driver. Yeah, regulators are
>>> important and (as I remember) they get probed slightly early anyway,
>>> but everything else in the system just gotta deal with the fact that
>>> they'll sometimes get deferred probes.
>>
>> I agree that consumers should handle probe deferral. Unfortunately,
>> reality isn't always so nice. Also, probe deferrals increase boot-up time
>> which is particularly problematic in the mobile space.
>
> Sigh, yeah. I'm not a fan either. If you can convince Mark that you
> should use arch_initcall() or subsys_initcall() I won't yell. ...but
> in the past I've seen others get yelled at.
>
> Note: in actuality it doesn't always increase boot time a whole lot.
> In theory as long as the CPU is running full bore and you're not
> killing parallelism too much then the only "waste" is the bit of time
> to run the start of the probe. It's not much. Every time someone
> claims "but my boot is slow because of deferrals" others say "where is
> the evidence?" and I don't remember a whole lot of evidence being
> presented. Maybe you have evidence?
>
> ...but deferrals _do_ for sure increase the time for certain
> peripherals to come up, and if those peripherals are things like the
> LCD displays then it sucks.
>
>
>> I suppose that I
>> can change this to module_platform_driver() for now. If any major issues
>> arise it could be changed back to arch_initcall().
>
> Probably wise.

I'll switch to using module_platform_driver().

>> Note that both qcom_rpm-regulator.c and qcom_smd-regulator.c are using
>> subsys_initcall() right now in place of module_platform_driver().
>
> Yeah, I've made the argument before and been told "they are grandfathered in."

Ah, I see.

Take care,
David

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

2018-03-27 23:40:06

[permalink] [raw]

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

On 03/27/2018 04:56 AM, Mark Brown wrote:
> On Fri, Mar 23, 2018 at 01:00:47PM -0700, Doug Anderson wrote:
>> On Thu, Mar 22, 2018 at 3:31 PM, David Collins <[email protected]> wrote:
>
>>> There are two cases that I can think of: 1. Having a set_voltage()
>>> callback allows for delaying for an RPMh request ACK during certain
>>> voltage set point decreasing scenarios (to be elaborated below).
>
>> Can't you still have a delay in set_voltage_sel()?
>
> We have specific support for adding ramp delays, no need to open code it
> in operations.

I'll switch to using set_voltage_sel(). No ramp delay is need in Linux.
RPMh hardware sends an ACK back to Linux after the voltage reaches the
desired set point (i.e. is greater than or equal to the set point). The
qcom_rpmh-regulator driver just needs to decide if it is going to wait for
the ACK from RPMh or not.

>>> 2.
>>> Having a get_voltage() as opposed to get_voltage_sel() callback allows an
>>> uninitialized voltage of 0 to be returned in the case that no initial
>>> voltage is specified in device tree. This eliminates the possibility of
>>> the regulator framework short-circuiting a regulator_set_voltage() call
>>> that happens to match the voltage corresponding to selector 0.
>
>> Interesting. I suppose you could mix them (have set_voltage_sel() and
>> get_voltage()) as long as you documented why you were doing it. Then
>> we'd have to see if Mark was happy with that...
>
> This is a *terrible* idea which will almost certainly break. If the
> driver can't read values it should return an appropriate error code.

I'll switch to using get_voltage_sel(). I'll test out returning an error
value in the case of an uninitialized voltage. Hopefully the regulator
framework won't halt regulator registration because of it.

>>> I'm aware of one important instance in which decreasing voltage needs a
>>> delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case
>>> that I had in mind with the 'max_uv < vreg->voltage' check. However, you
>>> are correct that hardware will report completion of the voltage slewing
>>> early in this case since the comparator is checking for output >= set
>>> point. I can remove this special case check.
>
> You can't usefully wait for voltages to fall, you can never guarantee
> what the loading on the device is. It's something the user has to
> manage if they care.

I agree. This case can't be handled in the regulator driver.

>>> Here is an explanation for why the "device tree mode" abstraction is
>>> present in the first place. Between different Qualcomm Technologies, Inc.
>>> PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO,
>>> LPM/PFM, Retention, and pass-through). However, the register values for
>>> the same modes vary between different regulator types and different PMIC
>>> families. This patch is adding support for several PMIC4 family PMICs.
>>> The values needed for to-be-released PMIC5 PMIC regulators are different.
>>> As an example, here are the different values for LPM/PFM across PMIC
>>> families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5
>>> LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode"
>>> ensures that it is not possible to inadvertently specify a PMIC specific
>>> mode in device tree which corresponds to the wrong type or family but
>>> which aliases a value that would be accepted as correct.
>
>> I'm OK with having the "device tree mode" abstraction, and in fact the
>> current regulator framework seems to want you to have this anyway. If
>> I read the code correctly, you're required to have the conversion
>> function and there's no default.
>
> I didn't spot this in the code but something called "device tree mode"
> sounds like it's going to be awfully confusing...

As I explained in the earlier email, it makes the device tree
configurations much simpler and less confusing/error prone. I'd like to
keep this concept around unless their are strong objections.

>>> Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm
>>> Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected,
>>> the BOB output is directly connected to the BOB input (typically Vbat).
>
> ...
>
>>> qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode
>>> could be handled via get_bypass() and set_bypass() regulator ops. Doing
>>> this would require more complicated ops selections in the driver since it
>
> This is exactly the functionality supported by the bypass operations.
> Any complexity due to the hardware design is unfortunate but honestly
> the way the QC regulator stuff is designed they seem like a bit of a
> lost cause on that front - they look very different to any other
> hardware we've seen.
>
>> I've never poked at the get_bypass() / set_bypass(), but it sounds
>> better to me to use them. I'm not a fan of the current hack. Even
>> aside from the bit of hackiness, I'm slightly concerned that some of
>> your logic that generally assumes lower integers = lower power modes
>> would break.
>
> Yes, abusing the framework is just going to make things even worse.

I'll implement get_bypass() and set_bypass() callbacks for BOB.

>>>>> +arch_initcall(rpmh_regulator_init);
>
>>>> I always get yelled at when I try to use arch_initcall() for stuff
>>>> like this. You should do what everyone else does and use
>
>>> I agree that consumers should handle probe deferral. Unfortunately,
>>> reality isn't always so nice. Also, probe deferrals increase boot-up time
>>> which is particularly problematic in the mobile space.
>
>> Sigh, yeah. I'm not a fan either. If you can convince Mark that you
>> should use arch_initcall() or subsys_initcall() I won't yell. ...but
>> in the past I've seen others get yelled at.
>
> Do you have concrete consumers that have a good reason for doing this?

I don't have any examples off the top of my head.

>> Note: in actuality it doesn't always increase boot time a whole lot.
>
> Note also that we now have the device dependency mechanism that Raphael
> implemented with the explicit idea that that it'd be used to avoid
> unneeded deferrals.
>
>> ...but deferrals _do_ for sure increase the time for certain
>> peripherals to come up, and if those peripherals are things like the
>> LCD displays then it sucks.
>
> There's been some discussion of allowing the user to specific certain
> devices to target as priorities for probing which should deal with that.

I'll look into these features.

Take care,
David

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

2018-03-27 23:49:31

[permalink] [raw]

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

Hello Mark,

On 03/20/2018 07:43 PM, Mark Brown wrote:
> On Fri, Mar 16, 2018 at 06:09:11PM -0700, David Collins wrote:
>> Introduce bindings for RPMh regulator devices found on some
>> Qualcomm Technlogies, Inc. SoCs. These devices allow a given
>
> Please use subject lines matching the style for the subsystem. This
> makes it easier for people to identify relevant patches.

Looking through the DT binding patches sent to [email protected],
it seemed like the preferred style is:
[PATCH] dt-bindings: subsystem: ...

I can change the subject line for this patch to be:
"[PATCH 2/2] regulator: dt-bindings: add QCOM RPMh regulator bindings"
if you prefer.

>> +=========================================
>> +Second Level Nodes - Regulators
>> +=========================================
>> +
>> +- regulator-name
>
> This document appears to be reproducing the standard regulator bindings;
> there is no need to do that, just reference them.

Sure, I'll remove this.

Thanks,
David

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

2018-03-28 02:10:33

[permalink] [raw]

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

On Tue, Mar 27, 2018 at 04:38:07PM -0700, David Collins wrote:
> On 03/27/2018 04:56 AM, Mark Brown wrote:

> > I didn't spot this in the code but something called "device tree mode"
> > sounds like it's going to be awfully confusing...

> As I explained in the earlier email, it makes the device tree
> configurations much simpler and less confusing/error prone. I'd like to
> keep this concept around unless their are strong objections.

Like I say I didn't spot this in the code, I did give it a brief once
over but as Doug had spotted such extensive problems I was expecting a
resend. It *is* setting off big alarm bells though, in general if your
individual driver is doing something weird to make life easier that's a
sign that it's doing things at the wrong level.

Attachments:

(No filename) (789.00 B)
signature.asc (499.00 B)
Download all attachments

2018-03-28 02:37:48

[permalink] [raw]

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

On Tue, Mar 27, 2018 at 01:51:56PM -0700, Doug Anderson wrote:

> Assuming I didn't mess up my analysis, the entire job of of_map_mode()
> is to convert from one integer to another. It should take the number
> that was specified in the device tree and convert it to a
> REGULATOR_MODE_XXX. That means that the regulator framework is
> enforcing a distinct and per-regulator numbering system for the mode
> (I called this "device tree mode").

OK, the confusion comes from your terminology invention rather than the
driver then :(

> So basically it sounds like everyone makes up some arbitrary numbering
> system that is only used in their device tree files and needs to be
> mapped into the standard numbering system...

It's not just that. The very modes themselves are not defined at all
consistently between regulators - the set of options varies wildly as
does the range of applications they are suitable for. We want people to
not only define numbers here but also names that make sense in the
context of the regulator documentation that can then be mapped into the
concept of modes we've inherited in the regulator subsystem.

Attachments:

(No filename) (1.13 kB)
signature.asc (499.00 B)
Download all attachments

2018-03-29 22:38:04

[permalink] [raw]

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

Hi,

On Wed, Mar 21, 2018 at 12:07 PM, Stephen Boyd <[email protected]> wrote:
>> +static int rpmh_regulator_remove(struct platform_device *pdev)
>> +{
>> + struct rpmh_pmic *pmic = platform_get_drvdata(pdev);
>> +
>> + rpmh_release(pmic->rpmh_client);
>
> I'm still lost on what rpmh_client is giving us besides more code we
> don't need. I'll ping the rpmh thread again.

It's not completely obvious to me what you are asking here and I think
you didn't actually get back to pinging the RPMH thread, did you? In
his response, David seems to have taken your comment as "I agree with
Doug, please add devm_rpmh_get_client", but I'm not sure that's what
you meant. It sounds a lot like you're suggesting totally gutting the
concept of the "rpmh_client".

I certainly haven't reviewed RPMH a whole lot, but perhaps you can
make it more obvious how this would work.

In any case, I guess this is a bit off topic for the regulator series.
Perhaps you can respond to the RPMH series with more details about
what you're looking for?

Thanks!

-Doug

2018-04-19 05:57:18

by Stephen Boyd

[permalink] [raw]

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

Quoting David Collins (2018-03-22 18:30:06)
> On 03/21/2018 12:07 PM, Stephen Boyd wrote:
> > Quoting David Collins (2018-03-16 18:09:10)
> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> >> index 097f617..e0ecd0a 100644
> >> --- a/drivers/regulator/Kconfig
> >> +++ b/drivers/regulator/Kconfig
> >> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
> >> Qualcomm RPM as a module. The module will be named
> >> "qcom_rpm-regulator".
> >>
> >> +config REGULATOR_QCOM_RPMH
> >> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
> >> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
> >
> > What's the build dependency on OF?
>
> The qcom_rpmh-regulator driver cannot function without device tree support
> enabled. I suppose that it might be able to compile, but it wouldn't be
> useful.

If it isn't a build dependency then we usually leave it out because it's
not always useful to express runtime constraints in Kconfig. Probably
all we need is depends on QCOM_RPMH || COMPILE_TEST and then stubs take
care of it from there.

>
> >> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
> >> + * @name: Name for the regulator which also corresponds
> >> + * to the device tree subnode name of the regulator
> >> + * @resource_name_base: RPMh regulator resource name prefix. E.g.
> >> + * "ldo" for RPMh resource "ldoa1".
> >
> > Maybe it should be "ldo%c1"? Then we could kasprintf the name with the
> > pmic_id and drop the 'id' member entirely.
>
> I can make this modification (though with %s instead of %c for simplicity
> with DT string parsing). Hopefully having a variable format string
> doesn't trigger any static analysis tools.

Oh it's variable how many digits in the 'id' number there are? I'll look
at v2.

>
>
> >> + int vreg_count;
> >> + const char *pmic_id;
> >> + const struct rpmh_pmic_init_data *init_data;
> >
> > Hopefully we don't really need this entire struct and we can just use
> > local variables instead.
>
> Outside of probe-time, this is used by struct rpmh_vreg in order to access
> rpmh_client (for RPMh transactions) and pmic->init_data->name (for debug
> and error messages). I suppose that rpmh_client could be specified in
> struct rpmh_vreg directly.
>

Hopefully rpm_client goes away. Maybe it would just be dev pointer to
hold onto then and possibly rip the name of each regulator out of the
framework name for it?

>
>
> >> +/**
> >> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
> >> + * for a VRM RPMh resource from device tree
> >> + * vreg: Pointer to the rpmh regulator resource
> >> + *
> >> + * This function initializes the mode[] array of vreg based upon the values
> >> + * of optional device tree properties.
> >> + *
> >> + * Return: 0 on success, errno on failure
> >> + */
> >> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
> >> +{
> >
> > I have a feeling this should all come from the driver data, not DT.
> > Doubtful this really changes for each board.
>
> This needs to be determined at a board level instead of hard-coded per
> regulator type. For LDOs switching between LPM and HPM typically happens
> at 10 mA or 30 mA per hardware documentation. Unfortunately, sharing
> control of regulators with other processors adds some subtlety.
>
> Consider the case of a regulator with 10 mA LPM/HPM threshold physically.
> Say that modem and application processors each have a load on the
> regulator that draws 9 mA. If they each respect the 10 mA threshold, then
> they'd each vote for LPM. VRM will aggregate these requests together
> which will result in the regulator being set to LPM even though the total
> load is 18 mA (which would require HPM). To get around this corner case,
> a threshold of 1 uA can be used for all supplies that have non-application
> processor consumers. Thus, any non-zero current request will result in
> setting the regulator to HPM (which is always safe).

Sad. Sounds like the rpmh interface is broken and should be expressing
the load instead of the mode so that aggregation on the rpm side can
pick the right mode. So now we have a workaround by specifying some
default minimum value that we add in?

>
> Another example is SMPS regulators which should theoretically always be
> able to operate in AUTO mode. However, there may be board/system issues
> that require switching to PWM mode (HPM) for certain use cases as it has
> better load regulation (even though it can source the same amount of
> current as AUTO mode). If there is more than one consumer that requires
> this ability for a given regulator, then regulator_set_load() is the only
> option since it provides aggregation, where as regulator_set_mode() does not.

I lost the code context, but my general comment was that the modes that
the hardware supports should come from the driver. If there's some sort
of constraint on what those modes end up being because some board has an
issue with some mode then we would want a binding that indicates such a
mode needs to be avoided, instead of listing the ones that are
supported. So default assume what registers support is there and then
knock out things when board designs constrain it. That's all I'm saying.
Maybe it's just "supported" that threw me off.

>
> >> +/**
> >> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
> >> + * request for this regulator based on optional device tree
> >> + * properties
> >> + * @vreg: Pointer to the RPMh regulator
> >> + *
> >> + * Return: 0 on success, errno on failure
> >> + */
> >> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
> >> +{
> >> + struct tcs_cmd cmd[2] = { };
> >> + const char *prop;
> >> + int cmd_count = 0;
> >> + int ret;
> >> + u32 temp;
> >> +
> >> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
> >> + prop = "qcom,headroom-voltage";
> >
> > Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh
> > registers. This probably needs to be pushed into the framework and come
> > down through a 'set_headroom' op in the regulator ops via a
> > regulator-headroom-microvolt property that's parsed in of_regulator.c.
>
> The qcom,headroom-voltage property is equivalent to struct
> regulator_desc.min_dropout_uV, but handled in hardware. I don't see the
> need to make a new regulator op to configure this value dynamically.
> Headroom typically does not need to change. Also, we don't really want
> this particular value plumbed into min_dropout_uV since we need to pass it
> directly to hardware and not have the regulator framework attempt to use
> it for a parent.

Ok? We have other regulator ops just to configure boot time things. The
goal is to come up with generic regulator properties that can be applied
from the framework perspective and be used by other regulator drivers in
the future.

>
>
> >> + prop = "qcom,regulator-initial-voltage";
> >
> > DT constraints should take care of this by setting voltages on all
> > regulators that need them?
>
> Unfortunately not. At regulator registration time, the regulator
> framework machine_constraints_voltage() function will call get_voltage()
> op and check if the voltage is in the min_uV to max_uV constraint range.
> If it is, then nothing happens. If it's not, then it will call the
> set_voltage() call back to set the voltage to min_uV if current_uV <
> min_uV or max_uV if current_uV > max_uV. Since the rpmh regulator driver
> doesn't know the initial voltage, get_voltage() will return 0 initially.
> As a result, machine_constraints_voltage() will always set the regulator
> to the minimum constraint voltage specified in DT.
>
> That behavior may not be acceptable for some regulators depending upon the
> hardware state at kernel initialization. Therefore, we need a DT
> mechanism to specify a single voltage to configure by default.

Cool. This should be a generic regulator DT property that all regulators
can use. We have the same problem on other RPM based regulator drivers
where boot sends a bunch of voltages because we don't know what it is by
default out of boot and we can't read it.

>
>
> >> + /* Optional override for the default RPMh accelerator type */
> >> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",
> >
> > Can this property have override in the name? And then because it is
> > called override, perhaps it should come from the driver instead of DT
> > because DT may need an override itself.
>
> Yes, I can add 'override' to the name of the property. I'm not following
> your second sentence. We require the ability to specify that a given
> regulator is using XOB instead of VRM at a board level. This means that
> the override needs to happen in DT instead of the driver. See [1] for
> more details.

Ah ok. Can this be "discovered" through cmd-db by probing for VRM and
then for XOB if there isn't a VRM one? Just wondering why we need to
describe it in DT at all if cmd-db will have one or the other.

>
> > Also, is this currently being used? If not I'd prefer we drop this until we
> > need it.
>
> It will be needed on a to-be-released Qualcomm Technologies, Inc. SoC that
> is currently under development. I suppose that I can leave this property
> out for the initial patch. However, we would still need to ensure that
> the driver architecture allows for this property to be read in a later
> patch and determine the regulator ops and whether or not to perform VRM
> specific initialization.

OK.

>
>
> >> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) {
> >> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
> >> + init_data->constraints.apply_uV = 0;
> >> + vreg->rdesc.n_voltages = 1;
> >> + }
> >
> > What is this doing? Usually constraints aren't touched by the driver.
>
> For XOB managed regulators, we need to set fixed_uV to match the DT
> constraint voltage and n_voltages = 1. This allows consumers
> regulator_set_voltage() calls to succeed for such regulators. It works
> the same as a fixed regulator. I think that apply_uV = 0 could be left out.
>

Wouldn't XOB regulators only have one voltage specified for min/max in
DT already though? I seem to recall that's how we make set_voltage() in
those cases work. Or it could be that drivers aren't supposed to call
set_voltage() on single or fixed voltage regulators anyway, because
constraints take care of it for them.

>
> >> +
> >> + if (vreg->hw_data->mode_map) {
> >> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
> >
> > Huh, I thought this was assigned by the framework.
>
> No, this is not set anywhere in the regulator framework. There isn't a DT
> method to configure it. It seems that it could only be handled before
> with board files. Other regulator drivers also configure it.

Hmm ok. Would something be bad if we did support it through DT? I can't
seem to recall the history here.

> >> +
> >> + ret = cmd_db_ready();
> >> + if (ret < 0) {
> >> + if (ret != -EPROBE_DEFER)
> >> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
> >> + return ret;
> >> + }
> >
> > We should just make rpmh parent device call cmd_db_ready() so that these
> > devices aren't even populated until then and so that cmd_db_ready() is
> > only in one place. Lina?
>
> Let's see if Lina has qualms about this plan.

Sounds like you're ok with it.

>
> >> + dev_dbg(dev, "successfully probed %d %s regulators\n",
> >> + pmic->vreg_count, pmic->init_data->name);
> >
> > Doesn't the regulator framework already print a bunch of constraint
> > stuff when regulators are registered? Seems sort of spammy even for
> > debug mode. Plus it's the only reason for pmic::name right now.
>
> I think that the regulator framework will only print something if it has
> to set the voltage in order to bring the regulator voltage into the
> constraint range. I suppose that I can remove this message.

Ah you're right. Still seems like it would be better to put some sort of
debug print into the core on regulator registration if anything.

2018-04-19 12:09:41

[permalink] [raw]

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

On Wed, Apr 18, 2018 at 10:55:49PM -0700, Stephen Boyd wrote:

> I lost the code context, but my general comment was that the modes that
> the hardware supports should come from the driver. If there's some sort
> of constraint on what those modes end up being because some board has an
> issue with some mode then we would want a binding that indicates such a

No, that's *never* how regulator constraints work. We only change the
hardware configuration if we have explicit permission from constraints
to do so, otherwise everything is left as we found it. That way if your
board catches fire or whatever it wasn't something that Linux initiated
just from default behaviour.

> > > Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh
> > > registers. This probably needs to be pushed into the framework and come
> > > down through a 'set_headroom' op in the regulator ops via a
> > > regulator-headroom-microvolt property that's parsed in of_regulator.c.

> > The qcom,headroom-voltage property is equivalent to struct
> > regulator_desc.min_dropout_uV, but handled in hardware. I don't see the
> > need to make a new regulator op to configure this value dynamically.

> Ok? We have other regulator ops just to configure boot time things. The
> goal is to come up with generic regulator properties that can be applied
> from the framework perspective and be used by other regulator drivers in
> the future.

This doesn't sound like what the min_dropout_uV constraint is intended
to handle - that's there for the regulator driver (not constraints) to
indicate how much headroom the regulator needs in the supply voltage in
order to provide regulation. It's not something the regulator uses,
it's something that gets fed into voltage requests made on the supply of
the regulator which I can't see that the hardware is going to be able to
handle unaided.

> Cool. This should be a generic regulator DT property that all regulators
> can use. We have the same problem on other RPM based regulator drivers
> where boot sends a bunch of voltages because we don't know what it is by
> default out of boot and we can't read it.

Ideally future versions of the RPM will have improved interfaces,
there's a bunch of problems like this :(

> >
> > >> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) {
> > >> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
> > >> + init_data->constraints.apply_uV = 0;
> > >> + vreg->rdesc.n_voltages = 1;
> > >> + }

> > > What is this doing? Usually constraints aren't touched by the driver.

> > For XOB managed regulators, we need to set fixed_uV to match the DT
> > constraint voltage and n_voltages = 1. This allows consumers
> > regulator_set_voltage() calls to succeed for such regulators. It works
> > the same as a fixed regulator. I think that apply_uV = 0 could be left out.

> Wouldn't XOB regulators only have one voltage specified for min/max in
> DT already though? I seem to recall that's how we make set_voltage() in
> those cases work. Or it could be that drivers aren't supposed to call
> set_voltage() on single or fixed voltage regulators anyway, because
> constraints take care of it for them.

Yes, constraints that specify a single voltage are done by setting min
and max to the same value. fixed_uV is *only* for regulators that have
a physically fixed voltage.

> > >> + if (vreg->hw_data->mode_map) {
> > >> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;

A driver must *NEVER* modify any constraints.

> > > Huh, I thought this was assigned by the framework.

> > No, this is not set anywhere in the regulator framework. There isn't a DT
> > method to configure it. It seems that it could only be handled before
> > with board files. Other regulator drivers also configure it.

> Hmm ok. Would something be bad if we did support it through DT? I can't
> seem to recall the history here.

Yes, if someone wants to configure this through DT they should add
support for configuring it using DT. The mode support in most
regulators is not practically useful so nobody did that yet. Mostly the
hardware does a much better job of adjusting modes on the fly for
anything that's going on at runtime than software is ever likely to do
so it's not worth it.

Attachments:

(No filename) (4.34 kB)
signature.asc (499.00 B)
Download all attachments

2018-04-20 19:10:44

[permalink] [raw]

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

On 04/18/2018 10:55 PM, Stephen Boyd wrote:
> Quoting David Collins (2018-03-22 18:30:06)
>> On 03/21/2018 12:07 PM, Stephen Boyd wrote:
>>> Quoting David Collins (2018-03-16 18:09:10)
>>>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>>>> index 097f617..e0ecd0a 100644
>>>> --- a/drivers/regulator/Kconfig
>>>> +++ b/drivers/regulator/Kconfig
>>>> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
>>>> Qualcomm RPM as a module. The module will be named
>>>> "qcom_rpm-regulator".
>>>>
>>>> +config REGULATOR_QCOM_RPMH
>>>> + tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
>>>> + depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
>>>
>>> What's the build dependency on OF?
>>
>> The qcom_rpmh-regulator driver cannot function without device tree support
>> enabled. I suppose that it might be able to compile, but it wouldn't be
>> useful.
>
> If it isn't a build dependency then we usually leave it out because it's
> not always useful to express runtime constraints in Kconfig. Probably
> all we need is depends on QCOM_RPMH || COMPILE_TEST and then stubs take
> care of it from there.

Ok. I'll change this to:

depends on QCOM_RPMH || COMPILE_TEST

>>>> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
>>>> + * @name: Name for the regulator which also corresponds
>>>> + * to the device tree subnode name of the regulator
>>>> + * @resource_name_base: RPMh regulator resource name prefix. E.g.
>>>> + * "ldo" for RPMh resource "ldoa1".
>>>
>>> Maybe it should be "ldo%c1"? Then we could kasprintf the name with the
>>> pmic_id and drop the 'id' member entirely.
>>
>> I can make this modification (though with %s instead of %c for simplicity
>> with DT string parsing). Hopefully having a variable format string
>> doesn't trigger any static analysis tools.
>
> Oh it's variable how many digits in the 'id' number there are? I'll look
> at v2.

At the moment, the id field has only ever been a single character.
However, that isn't set in stone. I was mainly interested in using %s
instead of %c since it doesn't require any special handling of the string
read directly from DT.

>>>> + int vreg_count;
>>>> + const char *pmic_id;
>>>> + const struct rpmh_pmic_init_data *init_data;
>>>
>>> Hopefully we don't really need this entire struct and we can just use
>>> local variables instead.
>>
>> Outside of probe-time, this is used by struct rpmh_vreg in order to access
>> rpmh_client (for RPMh transactions) and pmic->init_data->name (for debug
>> and error messages). I suppose that rpmh_client could be specified in
>> struct rpmh_vreg directly.
>>
>
> Hopefully rpm_client goes away. Maybe it would just be dev pointer to
> hold onto then and possibly rip the name of each regulator out of the
> framework name for it?

In the v2 patch set, I removed this top-level struct and moved rpmh_client
into struct rpmh_vreg. Now that v6 of the rpmh series is out with
rpmh_client removed and dev pointer in its place, I'll add the dev pointer
to struct rpmh_vreg in my v3 patch set.

>>>> +/**
>>>> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
>>>> + * for a VRM RPMh resource from device tree
>>>> + * vreg: Pointer to the rpmh regulator resource
>>>> + *
>>>> + * This function initializes the mode[] array of vreg based upon the values
>>>> + * of optional device tree properties.
>>>> + *
>>>> + * Return: 0 on success, errno on failure
>>>> + */
>>>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
>>>> +{
>>>
>>> I have a feeling this should all come from the driver data, not DT.
>>> Doubtful this really changes for each board.
>>
>> This needs to be determined at a board level instead of hard-coded per
>> regulator type. For LDOs switching between LPM and HPM typically happens
>> at 10 mA or 30 mA per hardware documentation. Unfortunately, sharing
>> control of regulators with other processors adds some subtlety.
>>
>> Consider the case of a regulator with 10 mA LPM/HPM threshold physically.
>> Say that modem and application processors each have a load on the
>> regulator that draws 9 mA. If they each respect the 10 mA threshold, then
>> they'd each vote for LPM. VRM will aggregate these requests together
>> which will result in the regulator being set to LPM even though the total
>> load is 18 mA (which would require HPM). To get around this corner case,
>> a threshold of 1 uA can be used for all supplies that have non-application
>> processor consumers. Thus, any non-zero current request will result in
>> setting the regulator to HPM (which is always safe).
>
> Sad. Sounds like the rpmh interface is broken and should be expressing
> the load instead of the mode so that aggregation on the rpm side can
> pick the right mode. So now we have a workaround by specifying some
> default minimum value that we add in?

RPMh hardware is not going to support load current aggregation. The
aggregation method in hardware is to pick the highest mode control
register value requested by all processors and send that to the PMIC.

We are avoiding the LPM + LPM request issue on downstream targets by
configuring the LPM vs HPM threshold current such that any load_uA total
greater than 0 from Linux consumers results in a HPM request.

>> Another example is SMPS regulators which should theoretically always be
>> able to operate in AUTO mode. However, there may be board/system issues
>> that require switching to PWM mode (HPM) for certain use cases as it has
>> better load regulation (even though it can source the same amount of
>> current as AUTO mode). If there is more than one consumer that requires
>> this ability for a given regulator, then regulator_set_load() is the only
>> option since it provides aggregation, where as regulator_set_mode() does not.
>
> I lost the code context, but my general comment was that the modes that
> the hardware supports should come from the driver. If there's some sort
> of constraint on what those modes end up being because some board has an
> issue with some mode then we would want a binding that indicates such a
> mode needs to be avoided, instead of listing the ones that are
> supported. So default assume what registers support is there and then
> knock out things when board designs constrain it. That's all I'm saying.
> Maybe it's just "supported" that threw me off.

As Mark explained, modes need to be explicitly allowed per board-specific
configurations.

>>>> +/**
>>>> + * rpmh_regulator_load_default_parameters() - initialize the RPMh resource
>>>> + * request for this regulator based on optional device tree
>>>> + * properties
>>>> + * @vreg: Pointer to the RPMh regulator
>>>> + *
>>>> + * Return: 0 on success, errno on failure
>>>> + */
>>>> +static int rpmh_regulator_load_default_parameters(struct rpmh_vreg *vreg)
>>>> +{
>>>> + struct tcs_cmd cmd[2] = { };
>>>> + const char *prop;
>>>> + int cmd_count = 0;
>>>> + int ret;
>>>> + u32 temp;
>>>> +
>>>> + if (vreg->regulator_type == RPMH_REGULATOR_TYPE_VRM) {
>>>> + prop = "qcom,headroom-voltage";
>>>
>>> Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh
>>> registers. This probably needs to be pushed into the framework and come
>>> down through a 'set_headroom' op in the regulator ops via a
>>> regulator-headroom-microvolt property that's parsed in of_regulator.c.
>>
>> The qcom,headroom-voltage property is equivalent to struct
>> regulator_desc.min_dropout_uV, but handled in hardware. I don't see the
>> need to make a new regulator op to configure this value dynamically.
>> Headroom typically does not need to change. Also, we don't really want
>> this particular value plumbed into min_dropout_uV since we need to pass it
>> directly to hardware and not have the regulator framework attempt to use
>> it for a parent.
>
> Ok? We have other regulator ops just to configure boot time things. The
> goal is to come up with generic regulator properties that can be applied
> from the framework perspective and be used by other regulator drivers in
> the future.

Hardware enforced headroom voltage is a feature very specific to RPMh. It
is not generic. I don't see any need to add general support for this in
the regulator framework. Software managed headroom voltage is already
present in the framework.

>>>> + prop = "qcom,regulator-initial-voltage";
>>>
>>> DT constraints should take care of this by setting voltages on all
>>> regulators that need them?
>>
>> Unfortunately not. At regulator registration time, the regulator
>> framework machine_constraints_voltage() function will call get_voltage()
>> op and check if the voltage is in the min_uV to max_uV constraint range.
>> If it is, then nothing happens. If it's not, then it will call the
>> set_voltage() call back to set the voltage to min_uV if current_uV <
>> min_uV or max_uV if current_uV > max_uV. Since the rpmh regulator driver
>> doesn't know the initial voltage, get_voltage() will return 0 initially.
>> As a result, machine_constraints_voltage() will always set the regulator
>> to the minimum constraint voltage specified in DT.
>>
>> That behavior may not be acceptable for some regulators depending upon the
>> hardware state at kernel initialization. Therefore, we need a DT
>> mechanism to specify a single voltage to configure by default.
>
> Cool. This should be a generic regulator DT property that all regulators
> can use. We have the same problem on other RPM based regulator drivers
> where boot sends a bunch of voltages because we don't know what it is by
> default out of boot and we can't read it.

I suppose that I can look into adding a generic
regulator-initial-microvolt property if that is strongly preferred over an
rpmh-regulator specific one.

>>>> + /* Optional override for the default RPMh accelerator type */
>>>> + ret = of_property_read_string(vreg->of_node, "qcom,rpmh-resource-type",
>>>
>>> Can this property have override in the name? And then because it is
>>> called override, perhaps it should come from the driver instead of DT
>>> because DT may need an override itself.
>>
>> Yes, I can add 'override' to the name of the property. I'm not following
>> your second sentence. We require the ability to specify that a given
>> regulator is using XOB instead of VRM at a board level. This means that
>> the override needs to happen in DT instead of the driver. See [1] for
>> more details.
>
> Ah ok. Can this be "discovered" through cmd-db by probing for VRM and
> then for XOB if there isn't a VRM one? Just wondering why we need to
> describe it in DT at all if cmd-db will have one or the other.

Unfortunately, this cannot be determined via command DB. Command DB will
only show us the mapping from resource name string to RPMh address. The
address does not distinguish VRM vs XOB. There are plans for adding
command DB AUX data to specify VRM vs XOB in future chips, but this is not
present for SDM845.

>>>> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) {
>>>> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
>>>> + init_data->constraints.apply_uV = 0;
>>>> + vreg->rdesc.n_voltages = 1;
>>>> + }
>>>
>>> What is this doing? Usually constraints aren't touched by the driver.
>>
>> For XOB managed regulators, we need to set fixed_uV to match the DT
>> constraint voltage and n_voltages = 1. This allows consumers
>> regulator_set_voltage() calls to succeed for such regulators. It works
>> the same as a fixed regulator. I think that apply_uV = 0 could be left out.
>>
>
> Wouldn't XOB regulators only have one voltage specified for min/max in
> DT already though? I seem to recall that's how we make set_voltage() in
> those cases work. Or it could be that drivers aren't supposed to call
> set_voltage() on single or fixed voltage regulators anyway, because
> constraints take care of it for them.

The XOB regulators will indeed have regulator-min-microvolt ==
regulator-max-microvolt in DT. However, this is not sufficient to ensure
that consumer regulator_set_voltage() calls intersecting the constraint
voltage return successfully. I have not specified any set_voltage() or
get_voltage() callbacks for XOB regulators because it is physically not
possible to configure their voltage. As a result, consumer
regulator_set_voltage calls can only succeed if desc.fixed_uV is set and
desc.n_voltages == 1. See [1] and [2]. Note that I removed
init_data->constraints.apply_uV manipulation in the qcom_rpmh-regulator v2
patch set.

Using the existing fixed_uV feature of the regulator framework seemed like
a better option than open-coding a get_voltage() callback for XOB which
returns a new struct rpmh_vreg field that stores the fixed XOB voltage.
Do you agree with this approach? I'll add init_data->constraints.min_uV
== init_data->constraints.max_uV to the above check as well.

>>>> +
>>>> + if (vreg->hw_data->mode_map) {
>>>> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
>>>
>>> Huh, I thought this was assigned by the framework.
>>
>> No, this is not set anywhere in the regulator framework. There isn't a DT
>> method to configure it. It seems that it could only be handled before
>> with board files. Other regulator drivers also configure it.
>
> Hmm ok. Would something be bad if we did support it through DT? I can't
> seem to recall the history here.

I'll send out a patch to support this in of_get_regulation_constraints().

>>>> + ret = cmd_db_ready();
>>>> + if (ret < 0) {
>>>> + if (ret != -EPROBE_DEFER)
>>>> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
>>>> + return ret;
>>>> + }
>>>
>>> We should just make rpmh parent device call cmd_db_ready() so that these
>>> devices aren't even populated until then and so that cmd_db_ready() is
>>> only in one place. Lina?
>>
>> Let's see if Lina has qualms about this plan.
>
> Sounds like you're ok with it.

Sure, I'll remove this check if Lina agrees to add it in the rpmh driver.

Thanks,
David

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c?h=v4.17-rc1#n2940
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c?h=v4.17-rc1#n3319

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

2018-04-20 19:29:57

[permalink] [raw]

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

On 04/19/2018 05:08 AM, Mark Brown wrote:
> On Wed, Apr 18, 2018 at 10:55:49PM -0700, Stephen Boyd wrote:
>>>> Is this regulator-microvolt-offset? Ah I guess it's a thing in the RPMh
>>>> registers. This probably needs to be pushed into the framework and come
>>>> down through a 'set_headroom' op in the regulator ops via a
>>>> regulator-headroom-microvolt property that's parsed in of_regulator.c.
>
>>> The qcom,headroom-voltage property is equivalent to struct
>>> regulator_desc.min_dropout_uV, but handled in hardware. I don't see the
>>> need to make a new regulator op to configure this value dynamically.
>
>> Ok? We have other regulator ops just to configure boot time things. The
>> goal is to come up with generic regulator properties that can be applied
>> from the framework perspective and be used by other regulator drivers in
>> the future.
>
> This doesn't sound like what the min_dropout_uV constraint is intended
> to handle - that's there for the regulator driver (not constraints) to
> indicate how much headroom the regulator needs in the supply voltage in
> order to provide regulation. It's not something the regulator uses,
> it's something that gets fed into voltage requests made on the supply of
> the regulator which I can't see that the hardware is going to be able to
> handle unaided.

RPMh hardware enforces the requested minimum headroom voltage for all
regulators with a parent. It has full knowledge of the parent-child
connections of regulators on the board (as programmed by the bootloader).
It automatically reconfigures the parent voltage when needed as a result
of requests changing the voltage of any of its child regulators.

>> Cool. This should be a generic regulator DT property that all regulators
>> can use. We have the same problem on other RPM based regulator drivers
>> where boot sends a bunch of voltages because we don't know what it is by
>> default out of boot and we can't read it.
>
> Ideally future versions of the RPM will have improved interfaces,
> there's a bunch of problems like this :(

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.

>>>>> + if (type == RPMH_REGULATOR_TYPE_XOB && init_data->constraints.min_uV) {
>>>>> + vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
>>>>> + init_data->constraints.apply_uV = 0;
>>>>> + vreg->rdesc.n_voltages = 1;
>>>>> + }
>
>>>> What is this doing? Usually constraints aren't touched by the driver.
>
>>> For XOB managed regulators, we need to set fixed_uV to match the DT
>>> constraint voltage and n_voltages = 1. This allows consumers
>>> regulator_set_voltage() calls to succeed for such regulators. It works
>>> the same as a fixed regulator. I think that apply_uV = 0 could be left out.
>
>> Wouldn't XOB regulators only have one voltage specified for min/max in
>> DT already though? I seem to recall that's how we make set_voltage() in
>> those cases work. Or it could be that drivers aren't supposed to call
>> set_voltage() on single or fixed voltage regulators anyway, because
>> constraints take care of it for them.
>
> Yes, constraints that specify a single voltage are done by setting min
> and max to the same value. fixed_uV is *only* for regulators that have
> a physically fixed voltage.

XOB managed regulators physically cannot change voltage. Therefore, do
you agree that it is reasonable to use fixed_uV for them? Note that I
removed init_data->constraints.apply_uV manipulation in version 2 of this
patch.

>>>>> + if (vreg->hw_data->mode_map) {
>>>>> + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
>
> A driver must *NEVER* modify any constraints.
>
>>>> Huh, I thought this was assigned by the framework.
>
>>> No, this is not set anywhere in the regulator framework. There isn't a DT
>>> method to configure it. It seems that it could only be handled before
>>> with board files. Other regulator drivers also configure it.
>
>> Hmm ok. Would something be bad if we did support it through DT? I can't
>> seem to recall the history here.
>
> Yes, if someone wants to configure this through DT they should add
> support for configuring it using DT. The mode support in most
> regulators is not practically useful so nobody did that yet. Mostly the
> hardware does a much better job of adjusting modes on the fly for
> anything that's going on at runtime than software is ever likely to do
> so it's not worth it.

I'll send out a patch to add generic support for this configuration via
device tree in the of_get_regulation_constraints() function.

Thanks,
David

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

2018-04-20 22:52:11

by Lina Iyer

[permalink] [raw]

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

On Fri, Apr 20 2018 at 13:07 -0600, David Collins wrote:
>On 04/18/2018 10:55 PM, Stephen Boyd wrote:
>> Quoting David Collins (2018-03-22 18:30:06)
>>> On 03/21/2018 12:07 PM, Stephen Boyd wrote:
>>>> Quoting David Collins (2018-03-16 18:09:10)
>>>>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>>>>> index 097f617..e0ecd0a 100644

>>>>> + ret = cmd_db_ready();
>>>>> + if (ret < 0) {
>>>>> + if (ret != -EPROBE_DEFER)
>>>>> + dev_err(dev, "Command DB not available, ret=%d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>
>>>> We should just make rpmh parent device call cmd_db_ready() so that these
>>>> devices aren't even populated until then and so that cmd_db_ready() is
>>>> only in one place. Lina?
>>>
>>> Let's see if Lina has qualms about this plan.
>>
>> Sounds like you're ok with it.
>
>Sure, I'll remove this check if Lina agrees to add it in the rpmh driver.
>
We want to make the RSC nodes child of Command DB? That way we probe the
controllers only if the command DB is ready?

I could do that. Just so you know, there is are no strict directives to
use Command DB. If a driver knows the information it needs to pass to
the accelerator, it may choose to skip command DB completely.

Thanks,
Lina

2018-04-24 17:43:52

[permalink] [raw]

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

On Fri, Apr 20, 2018 at 12:28:21PM -0700, David Collins wrote:
> On 04/19/2018 05:08 AM, Mark Brown wrote:

> > This doesn't sound like what the min_dropout_uV constraint is intended
> > to handle - that's there for the regulator driver (not constraints) to
> > indicate how much headroom the regulator needs in the supply voltage in
> > order to provide regulation. It's not something the regulator uses,
> > it's something that gets fed into voltage requests made on the supply of
> > the regulator which I can't see that the hardware is going to be able to
> > handle unaided.

> RPMh hardware enforces the requested minimum headroom voltage for all
> regulators with a parent. It has full knowledge of the parent-child
> connections of regulators on the board (as programmed by the bootloader).
> It automatically reconfigures the parent voltage when needed as a result
> of requests changing the voltage of any of its child regulators.

If the hardware has full knowledge of all these constraints and enforces
them transparently then why does the kernel care that it's doing that?
Doesn't it defeat the point of it doing all this stuff if we have to
know about it?

> > Ideally future versions of the RPM will have improved interfaces,
> > there's a bunch of problems like this :(

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

> > Yes, constraints that specify a single voltage are done by setting min
> > and max to the same value. fixed_uV is *only* for regulators that have
> > a physically fixed voltage.

> XOB managed regulators physically cannot change voltage. Therefore, do
> you agree that it is reasonable to use fixed_uV for them? Note that I
> removed init_data->constraints.apply_uV manipulation in version 2 of this
> patch.

If these regulators can't change voltage then surely we know what
voltage they have without needing it to be specified in DT?

Attachments:

(No filename) (2.24 kB)
signature.asc (499.00 B)
Download all attachments

2018-04-24 20:39:01

[permalink] [raw]

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

>>>>>> + ret = cmd_db_ready();
>>>>>> + if (ret < 0) {
>>>>>> + if (ret != -EPROBE_DEFER)
>>>>>> + dev_err(dev, "Command DB not available,
>>>>>> ret=%d\n", ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>
>>>>> We should just make rpmh parent device call cmd_db_ready() so that these
>>>>> devices aren't even populated until then and so that cmd_db_ready() is
>>>>> only in one place. Lina?
>>>>
>>>> Let's see if Lina has qualms about this plan.
>>>
>>> Sounds like you're ok with it.
>>
>> Sure, I'll remove this check if Lina agrees to add it in the rpmh driver.
>>
> We want to make the RSC nodes child of Command DB? That way we probe the
> controllers only if the command DB is ready?
>
> I could do that. Just so you know, there is are no strict directives to
> use Command DB. If a driver knows the information it needs to pass to
> the accelerator, it may choose to skip command DB completely.

The ask here is for the rpmh driver to call cmd_db_ready() in its probe
function and return any error encountered (e.g. -EPROBE_DEFER).

I suppose that specifying the rpmh rsc device tree node as a child inside
of the command DB node could achieve the same result. However, that seems
a little hacky as the rsc device is describing a hardware block physically
found on the SoC as opposed to a subcomponent of the command DB SMEM data
structure. I defer to device tree maintainers to comment on the
acceptability of this approach.

While technically it is possible for an rpmh consumer to intrinsically
know the addresses for its resources without polling command DB, do you
have any examples where this is the case?

Thanks,
David

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

2018-04-24 20:48:02

[permalink] [raw]

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

On 04/24/2018 10:41 AM, Mark Brown wrote:
> On Fri, Apr 20, 2018 at 12:28:21PM -0700, David Collins wrote:

>> RPMh hardware enforces the requested minimum headroom voltage for all
>> regulators with a parent. It has full knowledge of the parent-child
>> connections of regulators on the board (as programmed by the bootloader).
>> It automatically reconfigures the parent voltage when needed as a result
>> of requests changing the voltage of any of its child regulators.
>
> If the hardware has full knowledge of all these constraints and enforces
> them transparently then why does the kernel care that it's doing that?
> Doesn't it defeat the point of it doing all this stuff if we have to
> know about it?

The RPMh hardware is aware of the parent-child connections between
regulators as well as minimum headroom to ensure stable LDO voltage output
for subregulated LDOs. The intention of having the headroom be a
configurable property for processors is to support usecases in which
subregulated LDO loads are particularly sensitive to noise and require
additional headroom. Such usecases are board dependent and beyond the
baseline configurations set in RPMh hardware.

>>> Ideally future versions of the RPM will have improved interfaces,
>>> there's a bunch of problems like this :(
>
>> 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.

I will go with qcom,regulator-initial-microvolt then.

>>> Yes, constraints that specify a single voltage are done by setting min
>>> and max to the same value. fixed_uV is *only* for regulators that have
>>> a physically fixed voltage.
>
>> XOB managed regulators physically cannot change voltage. Therefore, do
>> you agree that it is reasonable to use fixed_uV for them? Note that I
>> removed init_data->constraints.apply_uV manipulation in version 2 of this
>> patch.
>
> If these regulators can't change voltage then surely we know what
> voltage they have without needing it to be specified in DT?

In the case of XOB managed LDO regulators, the LDOs physically can be
configured to different voltages by the bootloader. However, the RPMh
interface provides no mechanism for the application processor to read or
change that voltage. Therefore, we need a way to specify such voltages in
a board specific (as opposed to driver specific) manner (i.e. device tree).

Take care,
David

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

2018-05-17 16:47:50

[permalink] [raw]

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

On Tue, Apr 24, 2018 at 01:46:21PM -0700, David Collins wrote:
> On 04/24/2018 10:41 AM, Mark Brown wrote:

> > If the hardware has full knowledge of all these constraints and enforces
> > them transparently then why does the kernel care that it's doing that?
> > Doesn't it defeat the point of it doing all this stuff if we have to
> > know about it?

> The RPMh hardware is aware of the parent-child connections between
> regulators as well as minimum headroom to ensure stable LDO voltage output
> for subregulated LDOs. The intention of having the headroom be a
> configurable property for processors is to support usecases in which
> subregulated LDO loads are particularly sensitive to noise and require
> additional headroom. Such usecases are board dependent and beyond the
> baseline configurations set in RPMh hardware.

So the hardware implementation is some hard coding stuff that doesn't
really adequately reflect reality? This seems unfortunate. However do
we really need to tell the hardware about the fact that we're adding
extra headroom - are there actual interactions with non-Linux things
here?

> >> XOB managed regulators physically cannot change voltage. Therefore, do
> >> you agree that it is reasonable to use fixed_uV for them? Note that I
> >> removed init_data->constraints.apply_uV manipulation in version 2 of this
> >> patch.

> > If these regulators can't change voltage then surely we know what
> > voltage they have without needing it to be specified in DT?

> In the case of XOB managed LDO regulators, the LDOs physically can be
> configured to different voltages by the bootloader. However, the RPMh
> interface provides no mechanism for the application processor to read or
> change that voltage. Therefore, we need a way to specify such voltages in
> a board specific (as opposed to driver specific) manner (i.e. device tree).

Is the kernel somehow prevented from varying these voltages?

Attachments:

(No filename) (1.93 kB)
signature.asc (499.00 B)
Download all attachments

2018-05-17 20:49:30

[permalink] [raw]

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

On 05/16/2018 11:09 PM, Mark Brown wrote:
> On Tue, Apr 24, 2018 at 01:46:21PM -0700, David Collins wrote:
>> The RPMh hardware is aware of the parent-child connections between
>> regulators as well as minimum headroom to ensure stable LDO voltage output
>> for subregulated LDOs. The intention of having the headroom be a
>> configurable property for processors is to support usecases in which
>> subregulated LDO loads are particularly sensitive to noise and require
>> additional headroom. Such usecases are board dependent and beyond the
>> baseline configurations set in RPMh hardware.
>
> So the hardware implementation is some hard coding stuff that doesn't
> really adequately reflect reality? This seems unfortunate. However do
> we really need to tell the hardware about the fact that we're adding
> extra headroom - are there actual interactions with non-Linux things
> here?

The RPMh hardware is configured by the boot loader. The configuration
does reflect reality; however, it cannot handle all configurations at
initialization time. Specific headroom management typically comes up in
modem usecases for RF supplies that are sensitive to noise. This feature
allows RPMh masters (application processor, modem processor, etc) to make
requests only for the regulators that they directly care about without
having to worry about power grid parent-child details and setting the
voltage of parent regulators in order to ensure sufficient headroom.

If you really don't like having this feature present in the Linux RPMh
regulator driver, then I'd be ok removing it. It is not required for
SDM845 which the driver is initially targeting.

>>>> XOB managed regulators physically cannot change voltage. Therefore, do
>>>> you agree that it is reasonable to use fixed_uV for them? Note that I
>>>> removed init_data->constraints.apply_uV manipulation in version 2 of this
>>>> patch.
>
>>> If these regulators can't change voltage then surely we know what
>>> voltage they have without needing it to be specified in DT?
>
>> In the case of XOB managed LDO regulators, the LDOs physically can be
>> configured to different voltages by the bootloader. However, the RPMh
>> interface provides no mechanism for the application processor to read or
>> change that voltage. Therefore, we need a way to specify such voltages in
>> a board specific (as opposed to driver specific) manner (i.e. device tree).
>
> Is the kernel somehow prevented from varying these voltages?

Yes. Physically, there exists no RPMh register to read or write the
voltage of LDOs managed via XOB. Additionally, the kernel running on the
application processor is blocked from configuring the voltage via a direct
SPMI writes by access permissions that crash the system when violated.

Take care,
David

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

2018-05-22 16:37:21