[PATCH v2 0/4] Add support to read RPMH regulator settings

[PATCH v2 0/4] Add support to read RPMH regulator settings

[Kamal Wadhwa]

This patch series adds a new `rpmh_read()` API to allow reading RPMH
addresses. Also, using this new API, enhances the RPMH regulator driver
get_regulator* APIs like `get_regulator_sel()`, `get_mode()` and
`get_status()` to allow reading voltage/mode/enable setting from H/W.

This is needed because current design has a limitation - regulator
framework can only get the cached values from the last voltage set
operation. Because of this right after bootup a `get_voltage_sel()`
from regulator framework will return -ENOTRECOVERABLE error, causing
regulator framework to trigger an unnecessary `set_voltage_sel()` call
with the `min_uV` value specified in the regulator's device tree
settings, which can cause issues for consumers like the display and
UFS that require a consistent voltage setting from the bootloader
state until their drivers are probed.

With this change regulator framework will get the regulator voltage
and other settings, as configured during bootloader stage, avoiding
unnecessary voltage adjustments and maintaining consistent power
settings across the transition from bootloader to kernel.

Besides this feature this series also fixes:-
- An existing issue with the BOB5 pass code value. This is needed
  by `get_status()`.
- Code format/style related errors reported by checkpatch.pl.

Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
Changes in v2:
- Fixed the BOB bypass mode handling (existing issue in current driver).
  This was needed for `get_status()` implementation.
- Implemented `get_status()` callback.
- Callbacks for `is_enabled()` & `get_mode()` will now be used as-is
  ie. v1 changes reverted.
- Bootstapped the read values for `mode` and `status` in probe, based on
  comments recieved from reviewer. 
- Callback for `get_voltage_sel()` has been modified to handle cases
  where read voltage is out-of-range defined in the regulator DT settings,
  this is needed to ensure backward compatibilty. Regulator probes may
  fail otherwise for some older targets.
- This patch is rebased & tested on:
   https://lore.kernel.org/all/176070318151.57631.15443673679580823321.b4-ty@kernel.org/
  to avoid any merge issues.
- Fixed code style issues reported by checkpatch.pl script. 
- Link to v1: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-0-ae583d260195@oss.qualcomm.com

---
Kamal Wadhwa (3):
      regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling
      regulator: qcom-rpmh: Add support to read regulator settings
      regulators: qcom-rpmh-regulator: Fix coding style issues

Maulik Shah (1):
      soc: qcom: rpmh: Add support to read back resource settings

 drivers/regulator/qcom-rpmh-regulator.c | 184 +++++++++++++++++++++++++++++++-
 drivers/soc/qcom/rpmh-rsc.c             |  13 ++-
 drivers/soc/qcom/rpmh.c                 |  47 +++++++-
 include/soc/qcom/rpmh.h                 |   5 +
 include/soc/qcom/tcs.h                  |   2 +
 5 files changed, 241 insertions(+), 10 deletions(-)
---
base-commit: fe45352cd106ae41b5ad3f0066c2e54dbb2dfd70
change-id: 20250623-add-rpmh-read-support-3288f83cc20a

Best regards,
-- 
Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>

[PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Kamal Wadhwa]

Currently, when `rpmh_regulator_set_mode_bypass()` helper function
is called to set bypass mode, it sends PMIC4's BOB bypass mode
value for even if its a PMIC5 BOB.

To fix this, introduce new hw_data parameters:
 - `bypass_supported` to check if bypass is supported.
 - `pmic_bypass_mode` to store bypass mode value.

Use these 2 parameters to send correct PMIC bypass mode that
corresponds to PMIC4/5 BOB regulators from the helper function.

Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
 drivers/regulator/qcom-rpmh-regulator.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 0a561f1d94523bf479f48e8c2062f79cf64f5b5f..947fb5241233c92eaeda974b1b64d227d5946a59 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -111,6 +111,9 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
  * @hpm_min_load_uA:		Minimum load current in microamps that requires
  *				high power mode (HPM) operation.  This is used
  *				for LDO hardware type regulators only.
+ * @pmic_bypass_mode:		The PMIC bypass mode value. This is only
+ *				used if bypass_supported == true.
+ * @bypass_supported:		Indicates if bypass mode is supported
  * @pmic_mode_map:		Array indexed by regulator framework mode
  *				containing PMIC hardware modes.  Must be large
  *				enough to index all framework modes supported
@@ -125,6 +128,8 @@ struct rpmh_vreg_hw_data {
 	int					n_linear_ranges;
 	int					n_voltages;
 	int					hpm_min_load_uA;
+	int					pmic_bypass_mode;
+	bool					bypass_supported;
 	const int				*pmic_mode_map;
 	unsigned int			      (*of_map_mode)(unsigned int mode);
 };
@@ -310,10 +315,13 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
 	if (pmic_mode < 0)
 		return pmic_mode;
 
-	if (bypassed)
-		cmd.data = PMIC4_BOB_MODE_PASS;
-	else
+	if (bypassed) {
+		if(!vreg->hw_data->bypass_supported)
+			return -EINVAL;
+		cmd.data = vreg->hw_data->pmic_bypass_mode;
+	} else {
 		cmd.data = pmic_mode;
+	}
 
 	return rpmh_regulator_send_request(vreg, &cmd, true);
 }
@@ -767,6 +775,8 @@ static const struct rpmh_vreg_hw_data pmic4_bob = {
 	},
 	.n_linear_ranges = 1,
 	.n_voltages = 84,
+	.bypass_supported = true,
+	.pmic_bypass_mode = PMIC4_BOB_MODE_PASS,
 	.pmic_mode_map = pmic_mode_map_pmic4_bob,
 	.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
 };
@@ -975,6 +985,8 @@ static const struct rpmh_vreg_hw_data pmic5_bob = {
 	},
 	.n_linear_ranges = 1,
 	.n_voltages = 32,
+	.bypass_supported = true,
+	.pmic_bypass_mode = PMIC5_BOB_MODE_PASS,
 	.pmic_mode_map = pmic_mode_map_pmic5_bob,
 	.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
 };

-- 
2.25.1

[PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Kamal Wadhwa]

From: Maulik Shah <maulik.shah@oss.qualcomm.com>

All rpmh_*() APIs so far have supported placing votes for various
resource settings but the H/W also have option to read resource
settings.

This change adds a new rpmh_read() API to allow clients
to read back resource setting from H/W. This will be useful for
clients like regulators, which currently don't have a way to know
the settings applied during bootloader stage.

Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com
Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
 drivers/soc/qcom/rpmh-rsc.c | 13 +++++++++++--
 drivers/soc/qcom/rpmh.c     | 47 +++++++++++++++++++++++++++++++++++++++++----
 include/soc/qcom/rpmh.h     |  5 +++++
 include/soc/qcom/tcs.h      |  2 ++
 4 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index c6f7d5c9c493d9e06c048930b8a14a38660df4b1..ec85c457ea4527f94339c2033bfcc12346e3c443 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -443,6 +443,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 	int i;
 	unsigned long irq_status;
 	const struct tcs_request *req;
+	u32 reg;
 
 	irq_status = readl_relaxed(drv->tcs_base + drv->regs[RSC_DRV_IRQ_STATUS]);
 
@@ -453,6 +454,11 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 
 		trace_rpmh_tx_done(drv, i, req);
 
+		if (req->is_read) {
+			reg = drv->regs[RSC_DRV_CMD_RESP_DATA];
+			req->cmds[0].data = read_tcs_reg(drv, reg, i);
+		}
+
 		/* Clear AMC trigger & enable modes and
 		 * disable interrupt for this TCS
 		 */
@@ -493,13 +499,15 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 			       const struct tcs_request *msg)
 {
 	u32 msgid;
-	u32 cmd_msgid = CMD_MSGID_LEN | CMD_MSGID_WRITE;
+	u32 cmd_msgid = CMD_MSGID_LEN;
 	u32 cmd_enable = 0;
 	struct tcs_cmd *cmd;
 	int i, j;
 
 	/* Convert all commands to RR when the request has wait_for_compl set */
 	cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
+	if (!msg->is_read)
+		cmd_msgid |= CMD_MSGID_WRITE;
 
 	for (i = 0, j = cmd_id; i < msg->num_cmds; i++, j++) {
 		cmd = &msg->cmds[i];
@@ -513,7 +521,8 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 
 		write_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_MSGID], tcs_id, j, msgid);
 		write_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_ADDR], tcs_id, j, cmd->addr);
-		write_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_DATA], tcs_id, j, cmd->data);
+		if (!msg->is_read)
+			write_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_DATA], tcs_id, j, cmd->data);
 		trace_rpmh_send_msg(drv, tcs_id, msg->state, j, msgid, cmd);
 	}
 
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 8903ed956312d0a2ac7f673d86ef504947e9b119..4a611dac437ef28bac124583073c87a79e9e5cad 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -175,6 +175,9 @@ static int __rpmh_write(const struct device *dev, enum rpmh_state state,
 	struct cache_req *req;
 	int i;
 
+	if (rpm_msg->msg.is_read)
+		goto send_data;
+
 	/* Cache the request in our store and link the payload */
 	for (i = 0; i < rpm_msg->msg.num_cmds; i++) {
 		req = cache_rpm_request(ctrlr, state, &rpm_msg->msg.cmds[i]);
@@ -182,6 +185,7 @@ static int __rpmh_write(const struct device *dev, enum rpmh_state state,
 			return PTR_ERR(req);
 	}
 
+send_data:
 	if (state == RPMH_ACTIVE_ONLY_STATE) {
 		ret = rpmh_rsc_send_data(ctrlr_to_drv(ctrlr), &rpm_msg->msg);
 	} else {
@@ -194,7 +198,7 @@ static int __rpmh_write(const struct device *dev, enum rpmh_state state,
 }
 
 static int __fill_rpmh_msg(struct rpmh_request *req, enum rpmh_state state,
-		const struct tcs_cmd *cmd, u32 n)
+		const struct tcs_cmd *cmd, u32 n, bool is_read)
 {
 	if (!cmd || !n || n > MAX_RPMH_PAYLOAD)
 		return -EINVAL;
@@ -204,10 +208,45 @@ static int __fill_rpmh_msg(struct rpmh_request *req, enum rpmh_state state,
 	req->msg.state = state;
 	req->msg.cmds = req->cmd;
 	req->msg.num_cmds = n;
+	req->msg.is_read = is_read;
 
 	return 0;
 }
 
+/**
+ * rpmh_read: Read a resource value
+ *
+ * @dev: The device making the request
+ * @cmd: The payload having address of resource to read
+ *
+ * Reads the value for the resource address given in tcs_cmd->addr
+ * and returns the tcs_cmd->data filled with same.
+ *
+ * May sleep. Do not call from atomic contexts.
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)
+{
+	DECLARE_COMPLETION_ONSTACK(compl);
+	DEFINE_RPMH_MSG_ONSTACK(dev, RPMH_ACTIVE_ONLY_STATE, &compl, rpm_msg);
+	int ret;
+
+	ret = __fill_rpmh_msg(&rpm_msg, RPMH_ACTIVE_ONLY_STATE, cmd, 1, true);
+	if (ret)
+		return ret;
+
+	ret = __rpmh_write(dev, RPMH_ACTIVE_ONLY_STATE, &rpm_msg);
+	if (ret)
+		return ret;
+
+	ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);
+	cmd[0].data = rpm_msg.cmd[0].data;
+
+	return (ret > 0) ? 0 : -ETIMEDOUT;
+}
+EXPORT_SYMBOL_GPL(rpmh_read);
+
 /**
  * rpmh_write_async: Write a set of RPMH commands
  *
@@ -230,7 +269,7 @@ int rpmh_write_async(const struct device *dev, enum rpmh_state state,
 		return -ENOMEM;
 	rpm_msg->needs_free = true;
 
-	ret = __fill_rpmh_msg(rpm_msg, state, cmd, n);
+	ret = __fill_rpmh_msg(rpm_msg, state, cmd, n, false);
 	if (ret) {
 		kfree(rpm_msg);
 		return ret;
@@ -257,7 +296,7 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
 	DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
 	int ret;
 
-	ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n);
+	ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n, false);
 	if (ret)
 		return ret;
 
@@ -352,7 +391,7 @@ int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
 	rpm_msgs = req->rpm_msgs;
 
 	for (i = 0; i < count; i++) {
-		__fill_rpmh_msg(rpm_msgs + i, state, cmd, n[i]);
+		__fill_rpmh_msg(rpm_msgs + i, state, cmd, n[i], false);
 		cmd += n[i];
 	}
 
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index bdbee1a97d3685d8a6153d586ddf650bd3bd3dde..14ecbf242b6bd67c8167c176ed0970f42432f4f4 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -11,6 +11,8 @@
 
 
 #if IS_ENABLED(CONFIG_QCOM_RPMH)
+int rpmh_read(const struct device *dev, struct tcs_cmd *cmd);
+
 int rpmh_write(const struct device *dev, enum rpmh_state state,
 	       const struct tcs_cmd *cmd, u32 n);
 
@@ -24,6 +26,9 @@ void rpmh_invalidate(const struct device *dev);
 
 #else
 
+static inline int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)
+{ return -ENODEV; }
+
 static inline int rpmh_write(const struct device *dev, enum rpmh_state state,
 			     const struct tcs_cmd *cmd, u32 n)
 { return -ENODEV; }
diff --git a/include/soc/qcom/tcs.h b/include/soc/qcom/tcs.h
index cff67ce25488e2d3603a7707af2ca77f8266a713..45b8513be2f9bb0957796476f6031146ee34e931 100644
--- a/include/soc/qcom/tcs.h
+++ b/include/soc/qcom/tcs.h
@@ -51,6 +51,7 @@ struct tcs_cmd {
  * struct tcs_request: A set of tcs_cmds sent together in a TCS
  *
  * @state:          state for the request.
+ * @is_read:        set for read only requests
  * @wait_for_compl: wait until we get a response from the h/w accelerator
  *                  (same as setting cmd->wait for all commands in the request)
  * @num_cmds:       the number of @cmds in this request
@@ -58,6 +59,7 @@ struct tcs_cmd {
  */
 struct tcs_request {
 	enum rpmh_state state;
+	bool is_read;
 	u32 wait_for_compl;
 	u32 num_cmds;
 	struct tcs_cmd *cmds;

-- 
2.25.1

[PATCH v2 3/4] regulator: qcom-rpmh: Add support to read regulator settings

[Kamal Wadhwa]

Currently, the RPMH regulator's `get_voltage_sel()` function only
returns cached values from the last `set_voltage_sel()` operation.
This limitation prevents the regulator framework from accurately
reflecting the regulator configurations set during the bootloader
stage. As a result, the regulator framework may trigger an
unnecessary `set_voltage_sel()` call with the `min_uV` value
specified in the regulator's device tree settings, which can
cause issues for consumers like the display and UFS that require
a consistent voltage setting from the bootloader state until
their drivers are probed.

To address this issue, enhance the `get_voltage_sel()`, and also
add new `get_status()` callbacks to read the regulator settings
directly from the RPMH hardware using the `rpmh_read()`function.
This change ensures that the regulator framework accurately
reflects the actual state of the regulators, avoiding unnecessary
voltage adjustments and maintaining consistent power settings
across the transition from bootloader to kernel.

Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-2-ae583d260195@oss.qualcomm.com
---
 drivers/regulator/qcom-rpmh-regulator.c | 164 ++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)

diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 947fb5241233c92eaeda974b1b64d227d5946a59..9f693043cb87aa77a7a529b5b973323450db80be 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -61,8 +61,13 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
 };
 
 #define RPMH_REGULATOR_REG_VRM_VOLTAGE		0x0
+#define RPMH_REGULATOR_VOLTAGE_MASK		0x1FFF
+
 #define RPMH_REGULATOR_REG_ENABLE		0x4
+#define RPMH_REGULATOR_ENABLE_MASK		0x1
+
 #define RPMH_REGULATOR_REG_VRM_MODE		0x8
+#define RPMH_REGULATOR_MODE_MASK		0x7
 
 #define PMIC4_LDO_MODE_RETENTION		4
 #define PMIC4_LDO_MODE_LPM			5
@@ -169,6 +174,7 @@ struct rpmh_vreg {
 	bool				bypassed;
 	int				voltage_selector;
 	unsigned int			mode;
+	unsigned int			status;
 };
 
 /**
@@ -213,6 +219,36 @@ static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
 	return ret;
 }
 
+static int rpmh_regulator_read_data(struct rpmh_vreg *vreg, struct tcs_cmd *cmd)
+{
+	return rpmh_read(vreg->dev, cmd);
+}
+
+static int _rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev, int *uV)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+	};
+	int min_uV = rdev->constraints->min_uV;
+	int max_uV = rdev->constraints->max_uV;
+	int ret, _uV = 0;
+
+	ret = rpmh_regulator_read_data(vreg, &cmd);
+	if (!ret)
+		_uV = (cmd.data & RPMH_REGULATOR_VOLTAGE_MASK) * 1000;
+	else
+		dev_err(vreg->dev, "failed to read VOLTAGE ret = %d\n", ret);
+
+	if (!_uV || (_uV >= min_uV && _uV <= max_uV))
+		*uV = _uV;
+	else
+		dev_dbg(vreg->dev, "read voltage %d is out-of-range[%d:%d]\n",
+						_uV, min_uV, max_uV);
+
+	return ret;
+}
+
 static int _rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
 				unsigned int selector, bool wait_for_ack)
 {
@@ -254,10 +290,36 @@ static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
 static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
 {
 	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	int ret, uV = 0;
+
+	if (vreg->voltage_selector < 0) {
+		ret = _rpmh_regulator_vrm_get_voltage(rdev, &uV);
+		if (!ret && uV != 0)
+			vreg->voltage_selector = regulator_map_voltage_linear_range(rdev,
+							uV, INT_MAX);
+	}
 
 	return vreg->voltage_selector;
 }
 
+static enum regulator_status convert_mode_to_status(int mode)
+{
+	switch (mode) {
+	case REGULATOR_MODE_FAST:
+		return REGULATOR_STATUS_FAST;
+	case REGULATOR_MODE_NORMAL:
+		return REGULATOR_STATUS_NORMAL;
+	case REGULATOR_MODE_IDLE:
+		return REGULATOR_STATUS_IDLE;
+	case REGULATOR_MODE_STANDBY:
+		return REGULATOR_STATUS_STANDBY;
+	case REGULATOR_MODE_INVALID:
+		return REGULATOR_STATUS_ERROR;
+	default:
+		return REGULATOR_STATUS_UNDEFINED;
+	};
+}
+
 static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
 {
 	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
@@ -287,6 +349,15 @@ static int rpmh_regulator_set_enable_state(struct regulator_dev *rdev,
 	if (!ret)
 		vreg->enabled = enable;
 
+	if (vreg->enabled) {
+		if (vreg->bypassed)
+			vreg->status = REGULATOR_STATUS_BYPASS;
+		else
+			vreg->status = convert_mode_to_status(vreg->mode);
+	} else {
+		vreg->status = REGULATOR_STATUS_OFF;
+	}
+
 	return ret;
 }
 
@@ -323,6 +394,15 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
 		cmd.data = pmic_mode;
 	}
 
+	if (vreg->enabled) {
+		if (bypassed)
+			vreg->status = REGULATOR_STATUS_BYPASS;
+		else
+			vreg->status = convert_mode_to_status(mode);
+	} else {
+		vreg->status = REGULATOR_STATUS_OFF;
+	}
+
 	return rpmh_regulator_send_request(vreg, &cmd, true);
 }
 
@@ -342,6 +422,22 @@ static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
 	return ret;
 }
 
+static int rpmh_regulator_vrm_get_pmic_mode(struct rpmh_vreg *vreg, int *pmic_mode)
+{
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+	};
+	int ret;
+
+	ret = rpmh_regulator_read_data(vreg, &cmd);
+	if (!ret)
+		*pmic_mode = cmd.data & RPMH_REGULATOR_MODE_MASK;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+
 static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
 {
 	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
@@ -349,6 +445,13 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
 	return vreg->mode;
 }
 
+static int rpmh_regulator_vrm_get_status(struct regulator_dev *rdev)
+{
+	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+	return vreg->status;
+}
+
 /**
  * rpmh_regulator_vrm_get_optimum_mode() - get the mode based on the  load
  * @rdev:		Regulator device pointer for the rpmh-regulator
@@ -407,6 +510,7 @@ static const struct regulator_ops rpmh_regulator_vrm_ops = {
 	.list_voltage		= regulator_list_voltage_linear_range,
 	.set_mode		= rpmh_regulator_vrm_set_mode,
 	.get_mode		= rpmh_regulator_vrm_get_mode,
+	.get_status		= rpmh_regulator_vrm_get_status,
 };
 
 static const struct regulator_ops rpmh_regulator_vrm_drms_ops = {
@@ -418,6 +522,7 @@ static const struct regulator_ops rpmh_regulator_vrm_drms_ops = {
 	.list_voltage		= regulator_list_voltage_linear_range,
 	.set_mode		= rpmh_regulator_vrm_set_mode,
 	.get_mode		= rpmh_regulator_vrm_get_mode,
+	.get_status		= rpmh_regulator_vrm_get_status,
 	.get_optimum_mode	= rpmh_regulator_vrm_get_optimum_mode,
 };
 
@@ -430,6 +535,7 @@ static const struct regulator_ops rpmh_regulator_vrm_bypass_ops = {
 	.list_voltage		= regulator_list_voltage_linear_range,
 	.set_mode		= rpmh_regulator_vrm_set_mode,
 	.get_mode		= rpmh_regulator_vrm_get_mode,
+	.get_status		= rpmh_regulator_vrm_get_status,
 	.set_bypass		= rpmh_regulator_vrm_set_bypass,
 	.get_bypass		= rpmh_regulator_vrm_get_bypass,
 };
@@ -438,6 +544,7 @@ static const struct regulator_ops rpmh_regulator_xob_ops = {
 	.enable			= rpmh_regulator_enable,
 	.disable		= rpmh_regulator_disable,
 	.is_enabled		= rpmh_regulator_is_enabled,
+	.get_status		= rpmh_regulator_vrm_get_status,
 };
 
 /**
@@ -546,6 +653,58 @@ static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
 	return 0;
 }
 
+static int rpmh_regulator_determine_initial_status(struct rpmh_vreg *vreg)
+{
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+	};
+	int ret, pmic_mode, mode;
+	int sts = 0;
+
+	ret = rpmh_regulator_read_data(vreg, &cmd);
+	if (ret) {
+		dev_dbg(vreg->dev, "failed to read ENABLE status ret = %d\n", ret);
+		vreg->status = REGULATOR_STATUS_UNDEFINED;
+		return ret;
+	}
+
+	sts = cmd.data & RPMH_REGULATOR_ENABLE_MASK;
+	if (!sts) {
+		vreg->status = REGULATOR_STATUS_OFF;
+		return 0;
+	}
+
+	if (vreg->hw_data->regulator_type == XOB) {
+		vreg->status = sts ? REGULATOR_STATUS_ON : REGULATOR_STATUS_OFF;
+		return 0;
+	}
+
+	ret = rpmh_regulator_vrm_get_pmic_mode(vreg, &pmic_mode);
+	if (ret < 0) {
+		dev_dbg(vreg->dev, "failed to read pmic_mode ret = %d\n", ret);
+		vreg->mode = REGULATOR_MODE_INVALID;
+		vreg->status = REGULATOR_STATUS_UNDEFINED;
+		return ret;
+	}
+
+	if (vreg->hw_data->bypass_supported &&
+			vreg->hw_data->pmic_bypass_mode == pmic_mode) {
+		vreg->bypassed = true;
+		vreg->status = REGULATOR_STATUS_BYPASS;
+		return 0;
+	}
+
+	for (mode = 0; mode <= REGULATOR_MODE_STANDBY; mode++) {
+		if (pmic_mode == vreg->hw_data->pmic_mode_map[mode]) {
+			vreg->mode = mode;
+			break;
+		}
+	}
+
+	vreg->status = convert_mode_to_status(vreg->mode);
+	return 0;
+}
+
 static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
 	[REGULATOR_MODE_INVALID] = -EINVAL,
 	[REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
@@ -1820,6 +1979,11 @@ static int rpmh_regulator_probe(struct platform_device *pdev)
 						vreg_data);
 		if (ret < 0)
 			return ret;
+
+		ret = rpmh_regulator_determine_initial_status(vreg);
+		if (ret < 0)
+			dev_err(dev, "failed to read initial status for %s\n",
+					vreg->rdesc.name);
 	}
 
 	return 0;

-- 
2.25.1

[PATCH v2 4/4] regulators: qcom-rpmh-regulator: Fix coding style issues

[Kamal Wadhwa]

Fix the code style/format issues reported by checkpatch.pl
script.

Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
 drivers/regulator/qcom-rpmh-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 9f693043cb87aa77a7a529b5b973323450db80be..6a36ef967d5d9e32c005e79a12099ebef824842f 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -109,7 +109,7 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
  *				regulator
  * @ops:			Pointer to regulator ops callback structure
  * @voltage_ranges:		The possible ranges of voltages supported by this
- * 				PMIC regulator type
+ *				PMIC regulator type
  * @n_linear_ranges:		Number of entries in voltage_ranges
  * @n_voltages:			The number of unique voltage set points defined
  *				by voltage_ranges
@@ -387,7 +387,7 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
 		return pmic_mode;
 
 	if (bypassed) {
-		if(!vreg->hw_data->bypass_supported)
+		if (!vreg->hw_data->bypass_supported)
 			return -EINVAL;
 		cmd.data = vreg->hw_data->pmic_bypass_mode;
 	} else {

-- 
2.25.1

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Dmitry Baryshkov]

On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> value for even if its a PMIC5 BOB.

The universe will end, the Sun will explode and Ragnarök will be upon
us. Please describe the issue, why sending bypass value is bad.

> 
> To fix this, introduce new hw_data parameters:
>  - `bypass_supported` to check if bypass is supported.
>  - `pmic_bypass_mode` to store bypass mode value.
> 
> Use these 2 parameters to send correct PMIC bypass mode that
> corresponds to PMIC4/5 BOB regulators from the helper function.
> 
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---
>  drivers/regulator/qcom-rpmh-regulator.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 0a561f1d94523bf479f48e8c2062f79cf64f5b5f..947fb5241233c92eaeda974b1b64d227d5946a59 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -111,6 +111,9 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
>   * @hpm_min_load_uA:		Minimum load current in microamps that requires
>   *				high power mode (HPM) operation.  This is used
>   *				for LDO hardware type regulators only.
> + * @pmic_bypass_mode:		The PMIC bypass mode value. This is only
> + *				used if bypass_supported == true.
> + * @bypass_supported:		Indicates if bypass mode is supported
>   * @pmic_mode_map:		Array indexed by regulator framework mode
>   *				containing PMIC hardware modes.  Must be large
>   *				enough to index all framework modes supported
> @@ -125,6 +128,8 @@ struct rpmh_vreg_hw_data {
>  	int					n_linear_ranges;
>  	int					n_voltages;
>  	int					hpm_min_load_uA;
> +	int					pmic_bypass_mode;
> +	bool					bypass_supported;
>  	const int				*pmic_mode_map;
>  	unsigned int			      (*of_map_mode)(unsigned int mode);
>  };
> @@ -310,10 +315,13 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>  	if (pmic_mode < 0)
>  		return pmic_mode;
>  
> -	if (bypassed)
> -		cmd.data = PMIC4_BOB_MODE_PASS;
> -	else
> +	if (bypassed) {
> +		if(!vreg->hw_data->bypass_supported)
> +			return -EINVAL;

This is redundant, the regulators which don't support bypass should not
be using rpmh_regulator_vrm_bypass_ops.

> +		cmd.data = vreg->hw_data->pmic_bypass_mode;
> +	} else {
>  		cmd.data = pmic_mode;
> +	}

Can we extend pmic_mode_map to include the value for bypass?

>  
>  	return rpmh_regulator_send_request(vreg, &cmd, true);
>  }
> @@ -767,6 +775,8 @@ static const struct rpmh_vreg_hw_data pmic4_bob = {
>  	},
>  	.n_linear_ranges = 1,
>  	.n_voltages = 84,
> +	.bypass_supported = true,
> +	.pmic_bypass_mode = PMIC4_BOB_MODE_PASS,
>  	.pmic_mode_map = pmic_mode_map_pmic4_bob,
>  	.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
>  };
> @@ -975,6 +985,8 @@ static const struct rpmh_vreg_hw_data pmic5_bob = {
>  	},
>  	.n_linear_ranges = 1,
>  	.n_voltages = 32,
> +	.bypass_supported = true,
> +	.pmic_bypass_mode = PMIC5_BOB_MODE_PASS,
>  	.pmic_mode_map = pmic_mode_map_pmic5_bob,
>  	.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
>  };
> 
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Dmitry Baryshkov]

On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> value for even if its a PMIC5 BOB.
> 
> To fix this, introduce new hw_data parameters:
>  - `bypass_supported` to check if bypass is supported.
>  - `pmic_bypass_mode` to store bypass mode value.
> 
> Use these 2 parameters to send correct PMIC bypass mode that
> corresponds to PMIC4/5 BOB regulators from the helper function.
> 
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>

Also missing the Fixes tag.

> ---
>  drivers/regulator/qcom-rpmh-regulator.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Dmitry Baryshkov]

On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
> 
> All rpmh_*() APIs so far have supported placing votes for various
> resource settings but the H/W also have option to read resource
> settings.

Is it supported since SDM845?

> 
> This change adds a new rpmh_read() API to allow clients

See Documentation/process/submitting-patches.rst, "This patch ..."

> to read back resource setting from H/W. This will be useful for
> clients like regulators, which currently don't have a way to know
> the settings applied during bootloader stage.
> 
> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com

This is useless, please drop.

> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/rpmh-rsc.c | 13 +++++++++++--
>  drivers/soc/qcom/rpmh.c     | 47 +++++++++++++++++++++++++++++++++++++++++----
>  include/soc/qcom/rpmh.h     |  5 +++++
>  include/soc/qcom/tcs.h      |  2 ++
>  4 files changed, 61 insertions(+), 6 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Maulik Shah (mkshah)]

On 10/22/2025 3:58 AM, Dmitry Baryshkov wrote:
> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>
>> All rpmh_*() APIs so far have supported placing votes for various
>> resource settings but the H/W also have option to read resource
>> settings.
> 
> Is it supported since SDM845?

Yes, H/W supports reads since SDM845.> 
>>
>> This change adds a new rpmh_read() API to allow clients
> 
> See Documentation/process/submitting-patches.rst, "This patch ..."

I will address in next revision.

> 
>> to read back resource setting from H/W. This will be useful for
>> clients like regulators, which currently don't have a way to know
>> the settings applied during bootloader stage.
>>
>> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com
> 
> This is useless, please drop.

I will address in next revision.

Thanks,
Maulik

> 
>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>> ---
>>  drivers/soc/qcom/rpmh-rsc.c | 13 +++++++++++--
>>  drivers/soc/qcom/rpmh.c     | 47 +++++++++++++++++++++++++++++++++++++++++----
>>  include/soc/qcom/rpmh.h     |  5 +++++
>>  include/soc/qcom/tcs.h      |  2 ++
>>  4 files changed, 61 insertions(+), 6 deletions(-)
>>
> 

Re: [PATCH v2 3/4] regulator: qcom-rpmh: Add support to read regulator settings

[Dmitry Baryshkov]

On Wed, Oct 22, 2025 at 02:38:55AM +0530, Kamal Wadhwa wrote:
> Currently, the RPMH regulator's `get_voltage_sel()` function only
> returns cached values from the last `set_voltage_sel()` operation.
> This limitation prevents the regulator framework from accurately
> reflecting the regulator configurations set during the bootloader
> stage. As a result, the regulator framework may trigger an
> unnecessary `set_voltage_sel()` call with the `min_uV` value

unnecessary or incorrect?

> specified in the regulator's device tree settings, which can
> cause issues for consumers like the display and UFS that require
> a consistent voltage setting from the bootloader state until
> their drivers are probed.

It sounds like this should be handled through the .sync_state rather
than just reading the voltage. Please correct me if I'm wrong, even with
the .get_voltage_sel() in place the regulator framework still can lower
the vote.

> 
> To address this issue, enhance the `get_voltage_sel()`, and also
> add new `get_status()` callbacks to read the regulator settings
> directly from the RPMH hardware using the `rpmh_read()`function.
> This change ensures that the regulator framework accurately
> reflects the actual state of the regulators, avoiding unnecessary
> voltage adjustments and maintaining consistent power settings
> across the transition from bootloader to kernel.
> 
> Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-2-ae583d260195@oss.qualcomm.com
> ---
>  drivers/regulator/qcom-rpmh-regulator.c | 164 ++++++++++++++++++++++++++++++++
>  1 file changed, 164 insertions(+)
> 
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 947fb5241233c92eaeda974b1b64d227d5946a59..9f693043cb87aa77a7a529b5b973323450db80be 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -61,8 +61,13 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
>  };
>  
>  #define RPMH_REGULATOR_REG_VRM_VOLTAGE		0x0
> +#define RPMH_REGULATOR_VOLTAGE_MASK		0x1FFF
> +
>  #define RPMH_REGULATOR_REG_ENABLE		0x4
> +#define RPMH_REGULATOR_ENABLE_MASK		0x1
> +
>  #define RPMH_REGULATOR_REG_VRM_MODE		0x8
> +#define RPMH_REGULATOR_MODE_MASK		0x7
>  
>  #define PMIC4_LDO_MODE_RETENTION		4
>  #define PMIC4_LDO_MODE_LPM			5
> @@ -169,6 +174,7 @@ struct rpmh_vreg {
>  	bool				bypassed;
>  	int				voltage_selector;
>  	unsigned int			mode;
> +	unsigned int			status;
>  };
>  
>  /**
> @@ -213,6 +219,36 @@ static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
>  	return ret;
>  }
>  
> +static int rpmh_regulator_read_data(struct rpmh_vreg *vreg, struct tcs_cmd *cmd)
> +{
> +	return rpmh_read(vreg->dev, cmd);
> +}
> +
> +static int _rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev, int *uV)
> +{
> +	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +	struct tcs_cmd cmd = {
> +		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> +	};
> +	int min_uV = rdev->constraints->min_uV;
> +	int max_uV = rdev->constraints->max_uV;
> +	int ret, _uV = 0;
> +
> +	ret = rpmh_regulator_read_data(vreg, &cmd);
> +	if (!ret)
> +		_uV = (cmd.data & RPMH_REGULATOR_VOLTAGE_MASK) * 1000;
> +	else
> +		dev_err(vreg->dev, "failed to read VOLTAGE ret = %d\n", ret);
> +
> +	if (!_uV || (_uV >= min_uV && _uV <= max_uV))
> +		*uV = _uV;
> +	else
> +		dev_dbg(vreg->dev, "read voltage %d is out-of-range[%d:%d]\n",
> +						_uV, min_uV, max_uV);
> +
> +	return ret;
> +}
> +
>  static int _rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
>  				unsigned int selector, bool wait_for_ack)
>  {
> @@ -254,10 +290,36 @@ static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
>  static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
>  {
>  	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +	int ret, uV = 0;
> +
> +	if (vreg->voltage_selector < 0) {

Why do we return the cached value instead of always reading it from the
hardware?

> +		ret = _rpmh_regulator_vrm_get_voltage(rdev, &uV);
> +		if (!ret && uV != 0)
> +			vreg->voltage_selector = regulator_map_voltage_linear_range(rdev,
> +							uV, INT_MAX);
> +	}
>  
>  	return vreg->voltage_selector;
>  }
>  
> +static enum regulator_status convert_mode_to_status(int mode)
> +{
> +	switch (mode) {
> +	case REGULATOR_MODE_FAST:
> +		return REGULATOR_STATUS_FAST;
> +	case REGULATOR_MODE_NORMAL:
> +		return REGULATOR_STATUS_NORMAL;
> +	case REGULATOR_MODE_IDLE:
> +		return REGULATOR_STATUS_IDLE;
> +	case REGULATOR_MODE_STANDBY:
> +		return REGULATOR_STATUS_STANDBY;
> +	case REGULATOR_MODE_INVALID:
> +		return REGULATOR_STATUS_ERROR;
> +	default:
> +		return REGULATOR_STATUS_UNDEFINED;
> +	};
> +}
> +
>  static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
>  {
>  	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> @@ -287,6 +349,15 @@ static int rpmh_regulator_set_enable_state(struct regulator_dev *rdev,
>  	if (!ret)
>  		vreg->enabled = enable;
>  
> +	if (vreg->enabled) {
> +		if (vreg->bypassed)
> +			vreg->status = REGULATOR_STATUS_BYPASS;
> +		else
> +			vreg->status = convert_mode_to_status(vreg->mode);
> +	} else {
> +		vreg->status = REGULATOR_STATUS_OFF;
> +	}
> +
>  	return ret;
>  }
>  
> @@ -323,6 +394,15 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>  		cmd.data = pmic_mode;
>  	}
>  
> +	if (vreg->enabled) {
> +		if (bypassed)
> +			vreg->status = REGULATOR_STATUS_BYPASS;
> +		else
> +			vreg->status = convert_mode_to_status(mode);
> +	} else {
> +		vreg->status = REGULATOR_STATUS_OFF;
> +	}
> +
>  	return rpmh_regulator_send_request(vreg, &cmd, true);
>  }
>  
> @@ -342,6 +422,22 @@ static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
>  	return ret;
>  }
>  
> +static int rpmh_regulator_vrm_get_pmic_mode(struct rpmh_vreg *vreg, int *pmic_mode)
> +{
> +	struct tcs_cmd cmd = {
> +		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> +	};
> +	int ret;
> +
> +	ret = rpmh_regulator_read_data(vreg, &cmd);
> +	if (!ret)
> +		*pmic_mode = cmd.data & RPMH_REGULATOR_MODE_MASK;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
>  {
>  	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> @@ -349,6 +445,13 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
>  	return vreg->mode;
>  }
>  
> +static int rpmh_regulator_vrm_get_status(struct regulator_dev *rdev)
> +{
> +	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> +	return vreg->status;
> +}
> +
>  /**
>   * rpmh_regulator_vrm_get_optimum_mode() - get the mode based on the  load
>   * @rdev:		Regulator device pointer for the rpmh-regulator
> @@ -407,6 +510,7 @@ static const struct regulator_ops rpmh_regulator_vrm_ops = {
>  	.list_voltage		= regulator_list_voltage_linear_range,
>  	.set_mode		= rpmh_regulator_vrm_set_mode,
>  	.get_mode		= rpmh_regulator_vrm_get_mode,
> +	.get_status		= rpmh_regulator_vrm_get_status,
>  };
>  
>  static const struct regulator_ops rpmh_regulator_vrm_drms_ops = {
> @@ -418,6 +522,7 @@ static const struct regulator_ops rpmh_regulator_vrm_drms_ops = {
>  	.list_voltage		= regulator_list_voltage_linear_range,
>  	.set_mode		= rpmh_regulator_vrm_set_mode,
>  	.get_mode		= rpmh_regulator_vrm_get_mode,
> +	.get_status		= rpmh_regulator_vrm_get_status,
>  	.get_optimum_mode	= rpmh_regulator_vrm_get_optimum_mode,
>  };
>  
> @@ -430,6 +535,7 @@ static const struct regulator_ops rpmh_regulator_vrm_bypass_ops = {
>  	.list_voltage		= regulator_list_voltage_linear_range,
>  	.set_mode		= rpmh_regulator_vrm_set_mode,
>  	.get_mode		= rpmh_regulator_vrm_get_mode,
> +	.get_status		= rpmh_regulator_vrm_get_status,
>  	.set_bypass		= rpmh_regulator_vrm_set_bypass,
>  	.get_bypass		= rpmh_regulator_vrm_get_bypass,
>  };
> @@ -438,6 +544,7 @@ static const struct regulator_ops rpmh_regulator_xob_ops = {
>  	.enable			= rpmh_regulator_enable,
>  	.disable		= rpmh_regulator_disable,
>  	.is_enabled		= rpmh_regulator_is_enabled,
> +	.get_status		= rpmh_regulator_vrm_get_status,
>  };
>  
>  /**
> @@ -546,6 +653,58 @@ static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
>  	return 0;
>  }
>  
> +static int rpmh_regulator_determine_initial_status(struct rpmh_vreg *vreg)
> +{
> +	struct tcs_cmd cmd = {
> +		.addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
> +	};
> +	int ret, pmic_mode, mode;
> +	int sts = 0;
> +
> +	ret = rpmh_regulator_read_data(vreg, &cmd);
> +	if (ret) {
> +		dev_dbg(vreg->dev, "failed to read ENABLE status ret = %d\n", ret);
> +		vreg->status = REGULATOR_STATUS_UNDEFINED;
> +		return ret;
> +	}
> +
> +	sts = cmd.data & RPMH_REGULATOR_ENABLE_MASK;
> +	if (!sts) {
> +		vreg->status = REGULATOR_STATUS_OFF;
> +		return 0;
> +	}
> +
> +	if (vreg->hw_data->regulator_type == XOB) {
> +		vreg->status = sts ? REGULATOR_STATUS_ON : REGULATOR_STATUS_OFF;
> +		return 0;
> +	}
> +
> +	ret = rpmh_regulator_vrm_get_pmic_mode(vreg, &pmic_mode);
> +	if (ret < 0) {
> +		dev_dbg(vreg->dev, "failed to read pmic_mode ret = %d\n", ret);
> +		vreg->mode = REGULATOR_MODE_INVALID;
> +		vreg->status = REGULATOR_STATUS_UNDEFINED;
> +		return ret;
> +	}
> +
> +	if (vreg->hw_data->bypass_supported &&
> +			vreg->hw_data->pmic_bypass_mode == pmic_mode) {

Wrong indentation

> +		vreg->bypassed = true;
> +		vreg->status = REGULATOR_STATUS_BYPASS;
> +		return 0;
> +	}
> +
> +	for (mode = 0; mode <= REGULATOR_MODE_STANDBY; mode++) {
> +		if (pmic_mode == vreg->hw_data->pmic_mode_map[mode]) {
> +			vreg->mode = mode;
> +			break;
> +		}
> +	}
> +
> +	vreg->status = convert_mode_to_status(vreg->mode);
> +	return 0;
> +}
> +
>  static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
>  	[REGULATOR_MODE_INVALID] = -EINVAL,
>  	[REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
> @@ -1820,6 +1979,11 @@ static int rpmh_regulator_probe(struct platform_device *pdev)
>  						vreg_data);
>  		if (ret < 0)
>  			return ret;
> +
> +		ret = rpmh_regulator_determine_initial_status(vreg);
> +		if (ret < 0)
> +			dev_err(dev, "failed to read initial status for %s\n",
> +					vreg->rdesc.name);
>  	}
>  
>  	return 0;
> 
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/4] regulators: qcom-rpmh-regulator: Fix coding style issues

[Dmitry Baryshkov]

On Wed, Oct 22, 2025 at 02:38:56AM +0530, Kamal Wadhwa wrote:
> Fix the code style/format issues reported by checkpatch.pl
> script.
> 
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---
>  drivers/regulator/qcom-rpmh-regulator.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Why are you fixing a patch that was a part of this series?

> 
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 9f693043cb87aa77a7a529b5b973323450db80be..6a36ef967d5d9e32c005e79a12099ebef824842f 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -109,7 +109,7 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
>   *				regulator
>   * @ops:			Pointer to regulator ops callback structure
>   * @voltage_ranges:		The possible ranges of voltages supported by this
> - * 				PMIC regulator type
> + *				PMIC regulator type
>   * @n_linear_ranges:		Number of entries in voltage_ranges
>   * @n_voltages:			The number of unique voltage set points defined
>   *				by voltage_ranges
> @@ -387,7 +387,7 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>  		return pmic_mode;
>  
>  	if (bypassed) {
> -		if(!vreg->hw_data->bypass_supported)
> +		if (!vreg->hw_data->bypass_supported)
>  			return -EINVAL;
>  		cmd.data = vreg->hw_data->pmic_bypass_mode;
>  	} else {
> 
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] regulator: qcom-rpmh: Add support to read regulator settings

[kernel test robot]

Hi Kamal,

kernel test robot noticed the following build warnings:

[auto build test WARNING on fe45352cd106ae41b5ad3f0066c2e54dbb2dfd70]

url:    https://github.com/intel-lab-lkp/linux/commits/Kamal-Wadhwa/regulator-rpmh-regulator-Fix-PMIC5-BOB-bypass-mode-handling/20251022-051042
base:   fe45352cd106ae41b5ad3f0066c2e54dbb2dfd70
patch link:    https://lore.kernel.org/r/20251022-add-rpmh-read-support-v2-3-5c7a8e4df601%40oss.qualcomm.com
patch subject: [PATCH v2 3/4] regulator: qcom-rpmh: Add support to read regulator settings
config: i386-buildonly-randconfig-002-20251022 (https://download.01.org/0day-ci/archive/20251022/202510222018.4wv8ONuO-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251022/202510222018.4wv8ONuO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510222018.4wv8ONuO-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/regulator/qcom-rpmh-regulator.c:177 struct member 'status' not described in 'rpmh_vreg'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Konrad Dybcio]

On 10/21/25 11:08 PM, Kamal Wadhwa wrote:
> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> value for even if its a PMIC5 BOB.
> 
> To fix this, introduce new hw_data parameters:
>  - `bypass_supported` to check if bypass is supported.
>  - `pmic_bypass_mode` to store bypass mode value.
> 
> Use these 2 parameters to send correct PMIC bypass mode that
> corresponds to PMIC4/5 BOB regulators from the helper function.
> 
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Konrad Dybcio]

On 10/22/25 12:23 AM, Dmitry Baryshkov wrote:
> On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
>> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
>> is called to set bypass mode, it sends PMIC4's BOB bypass mode
>> value for even if its a PMIC5 BOB.
> 
> The universe will end, the Sun will explode and Ragnarök will be upon
> us. Please describe the issue, why sending bypass value is bad.

I think you misread, it sends the magic value which corresponds
to BYPASS, but one that worked for the previous generation

Konrad

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Dmitry Baryshkov]

On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote:
> On 10/22/25 12:23 AM, Dmitry Baryshkov wrote:
> > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> >> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> >> value for even if its a PMIC5 BOB.
> > 
> > The universe will end, the Sun will explode and Ragnarök will be upon
> > us. Please describe the issue, why sending bypass value is bad.
> 
> I think you misread, it sends the magic value which corresponds
> to BYPASS, but one that worked for the previous generation

I just wanted to point out that the commit message makes a statement
that it sends some value. It should document, why the sent value is bad.

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Mark Brown]

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

On Wed, Oct 22, 2025 at 06:11:46PM +0300, Dmitry Baryshkov wrote:
> On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote:
> > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote:
> > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:

> > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> > >> value for even if its a PMIC5 BOB.

> > > The universe will end, the Sun will explode and Ragnarök will be upon
> > > us. Please describe the issue, why sending bypass value is bad.

> > I think you misread, it sends the magic value which corresponds
> > to BYPASS, but one that worked for the previous generation

> I just wanted to point out that the commit message makes a statement
> that it sends some value. It should document, why the sent value is bad.

It seems fairly clear to me from the above that the driver is sending
the value for the wrong device type which is something so obviously
wrong I'm not sure it requires further explanation.

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

Re: [PATCH v2 0/4] Add support to read RPMH regulator settings

[Bjorn Andersson]

On Wed, Oct 22, 2025 at 02:38:52AM +0530, Kamal Wadhwa wrote:
> This patch series adds a new `rpmh_read()` API to allow reading RPMH
> addresses. Also, using this new API, enhances the RPMH regulator driver
> get_regulator* APIs like `get_regulator_sel()`, `get_mode()` and
> `get_status()` to allow reading voltage/mode/enable setting from H/W.
> 
> This is needed because current design has a limitation - regulator
> framework can only get the cached values from the last voltage set
> operation. Because of this right after bootup a `get_voltage_sel()`
> from regulator framework will return -ENOTRECOVERABLE error, causing
> regulator framework to trigger an unnecessary `set_voltage_sel()` call
> with the `min_uV` value specified in the regulator's device tree
> settings, which can cause issues for consumers like the display and
> UFS that require a consistent voltage setting from the bootloader
> state until their drivers are probed.
> 
> With this change regulator framework will get the regulator voltage
> and other settings, as configured during bootloader stage, avoiding
> unnecessary voltage adjustments and maintaining consistent power
> settings across the transition from bootloader to kernel.
> 
> Besides this feature this series also fixes:-
> - An existing issue with the BOB5 pass code value. This is needed
>   by `get_status()`.
> - Code format/style related errors reported by checkpatch.pl.
> 
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---
> Changes in v2:
> - Fixed the BOB bypass mode handling (existing issue in current driver).
>   This was needed for `get_status()` implementation.
> - Implemented `get_status()` callback.
> - Callbacks for `is_enabled()` & `get_mode()` will now be used as-is
>   ie. v1 changes reverted.
> - Bootstapped the read values for `mode` and `status` in probe, based on
>   comments recieved from reviewer. 
> - Callback for `get_voltage_sel()` has been modified to handle cases
>   where read voltage is out-of-range defined in the regulator DT settings,
>   this is needed to ensure backward compatibilty. Regulator probes may
>   fail otherwise for some older targets.
> - This patch is rebased & tested on:
>    https://lore.kernel.org/all/176070318151.57631.15443673679580823321.b4-ty@kernel.org/
>   to avoid any merge issues.
> - Fixed code style issues reported by checkpatch.pl script. 
> - Link to v1: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-0-ae583d260195@oss.qualcomm.com
> 
> ---
> Kamal Wadhwa (3):
>       regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling
>       regulator: qcom-rpmh: Add support to read regulator settings
>       regulators: qcom-rpmh-regulator: Fix coding style issues

These three changes all changes the same one file, but you have 3
different subject prefixes. This is just sloppy, please follow other
changes to this one file.

> 
> Maulik Shah (1):
>       soc: qcom: rpmh: Add support to read back resource settings

I'd like to merge this through the soc tree, please put it first in the
series to simplify this.

Regards,
Bjorn

> 
>  drivers/regulator/qcom-rpmh-regulator.c | 184 +++++++++++++++++++++++++++++++-
>  drivers/soc/qcom/rpmh-rsc.c             |  13 ++-
>  drivers/soc/qcom/rpmh.c                 |  47 +++++++-
>  include/soc/qcom/rpmh.h                 |   5 +
>  include/soc/qcom/tcs.h                  |   2 +
>  5 files changed, 241 insertions(+), 10 deletions(-)
> ---
> base-commit: fe45352cd106ae41b5ad3f0066c2e54dbb2dfd70
> change-id: 20250623-add-rpmh-read-support-3288f83cc20a
> 
> Best regards,
> -- 
> Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> 

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Bjorn Andersson]

On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
> 
> All rpmh_*() APIs so far have supported placing votes for various
> resource settings but the H/W also have option to read resource
> settings.
> 
> This change adds a new rpmh_read() API to allow clients
> to read back resource setting from H/W. This will be useful for
> clients like regulators, which currently don't have a way to know
> the settings applied during bootloader stage.
> 

Allow me to express my disappointment over the fact that you sat on this
for 7 years!

> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com

Why is there a Link here?

> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/rpmh-rsc.c | 13 +++++++++++--
>  drivers/soc/qcom/rpmh.c     | 47 +++++++++++++++++++++++++++++++++++++++++----
>  include/soc/qcom/rpmh.h     |  5 +++++
>  include/soc/qcom/tcs.h      |  2 ++
>  4 files changed, 61 insertions(+), 6 deletions(-)
> 
[..]
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
[..]
> +/**
> + * rpmh_read: Read a resource value
> + *
> + * @dev: The device making the request
> + * @cmd: The payload having address of resource to read
> + *
> + * Reads the value for the resource address given in tcs_cmd->addr
> + * and returns the tcs_cmd->data filled with same.
> + *
> + * May sleep. Do not call from atomic contexts.

* Context: May sleep...

Regards,
Bjorn

> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Maulik Shah (mkshah)]

On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>
>> All rpmh_*() APIs so far have supported placing votes for various
>> resource settings but the H/W also have option to read resource
>> settings.
>>
>> This change adds a new rpmh_read() API to allow clients
>> to read back resource setting from H/W. This will be useful for
>> clients like regulators, which currently don't have a way to know
>> the settings applied during bootloader stage.
>>
> 
> Allow me to express my disappointment over the fact that you sat on this
> for 7 years!

This was a dead API (even in downstream) with no user since SDM845/ 7 years.
Read support was eventually removed from downstream driver too for the same reason.
There were early discussions to remove read support from RSC H/W, due to lack of users.
Its not realized yet and all SoCs still supports read.

Now we have a regulator client requirement to read resource votes and reason to bring back this API.

> 
>> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com
> 
> Why is there a Link here?

I will address this in next revision.

> 
>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>> ---
>>  drivers/soc/qcom/rpmh-rsc.c | 13 +++++++++++--
>>  drivers/soc/qcom/rpmh.c     | 47 +++++++++++++++++++++++++++++++++++++++++----
>>  include/soc/qcom/rpmh.h     |  5 +++++
>>  include/soc/qcom/tcs.h      |  2 ++
>>  4 files changed, 61 insertions(+), 6 deletions(-)
>>
> [..]
>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> [..]
>> +/**
>> + * rpmh_read: Read a resource value
>> + *
>> + * @dev: The device making the request
>> + * @cmd: The payload having address of resource to read
>> + *
>> + * Reads the value for the resource address given in tcs_cmd->addr
>> + * and returns the tcs_cmd->data filled with same.
>> + *
>> + * May sleep. Do not call from atomic contexts.
> 
> * Context: May sleep...

I will address this in next revision.

Thanks,
Maulik> 
> Regards,
> Bjorn
> 
>> + *
>> + * Return: 0 on success, negative errno on failure
>> + */
>> +int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Konrad Dybcio]

On 10/23/25 6:46 AM, Maulik Shah (mkshah) wrote:
> 
> 
> On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
>> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>
>>> All rpmh_*() APIs so far have supported placing votes for various
>>> resource settings but the H/W also have option to read resource
>>> settings.
>>>
>>> This change adds a new rpmh_read() API to allow clients
>>> to read back resource setting from H/W. This will be useful for
>>> clients like regulators, which currently don't have a way to know
>>> the settings applied during bootloader stage.
>>>
>>
>> Allow me to express my disappointment over the fact that you sat on this
>> for 7 years!
> 
> This was a dead API (even in downstream) with no user since SDM845/ 7 years.
> Read support was eventually removed from downstream driver too for the same reason.
> There were early discussions to remove read support from RSC H/W, due to lack of users.
> Its not realized yet and all SoCs still supports read.

Can we read BCM states from HLOS this way too?

Konrad

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Maulik Shah (mkshah)]

On 10/23/2025 1:47 PM, Konrad Dybcio wrote:
> On 10/23/25 6:46 AM, Maulik Shah (mkshah) wrote:
>>
>>
>> On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
>>> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>
>>>> All rpmh_*() APIs so far have supported placing votes for various
>>>> resource settings but the H/W also have option to read resource
>>>> settings.
>>>>
>>>> This change adds a new rpmh_read() API to allow clients
>>>> to read back resource setting from H/W. This will be useful for
>>>> clients like regulators, which currently don't have a way to know
>>>> the settings applied during bootloader stage.
>>>>
>>>
>>> Allow me to express my disappointment over the fact that you sat on this
>>> for 7 years!
>>
>> This was a dead API (even in downstream) with no user since SDM845/ 7 years.
>> Read support was eventually removed from downstream driver too for the same reason.
>> There were early discussions to remove read support from RSC H/W, due to lack of users.
>> Its not realized yet and all SoCs still supports read.
> 
> Can we read BCM states from HLOS this way too?

Yes, Any of ARC/BCM/VRM can be read to get HLOS/DRV2 votes.

Thanks,
Maulik

> 
> Konrad

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Konrad Dybcio]

On 10/23/25 10:57 AM, Maulik Shah (mkshah) wrote:
> 
> 
> On 10/23/2025 1:47 PM, Konrad Dybcio wrote:
>> On 10/23/25 6:46 AM, Maulik Shah (mkshah) wrote:
>>>
>>>
>>> On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
>>>> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>>
>>>>> All rpmh_*() APIs so far have supported placing votes for various
>>>>> resource settings but the H/W also have option to read resource
>>>>> settings.
>>>>>
>>>>> This change adds a new rpmh_read() API to allow clients
>>>>> to read back resource setting from H/W. This will be useful for
>>>>> clients like regulators, which currently don't have a way to know
>>>>> the settings applied during bootloader stage.
>>>>>
>>>>
>>>> Allow me to express my disappointment over the fact that you sat on this
>>>> for 7 years!
>>>
>>> This was a dead API (even in downstream) with no user since SDM845/ 7 years.
>>> Read support was eventually removed from downstream driver too for the same reason.
>>> There were early discussions to remove read support from RSC H/W, due to lack of users.
>>> Its not realized yet and all SoCs still supports read.
>>
>> Can we read BCM states from HLOS this way too?
> 
> Yes, Any of ARC/BCM/VRM can be read to get HLOS/DRV2 votes.

Wow this is amazing..

Do you have code for this already, or should I hack on it?

Konrad

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Maulik Shah (mkshah)]

On 10/23/2025 2:39 PM, Konrad Dybcio wrote:
> On 10/23/25 10:57 AM, Maulik Shah (mkshah) wrote:
>>
>>
>> On 10/23/2025 1:47 PM, Konrad Dybcio wrote:
>>> On 10/23/25 6:46 AM, Maulik Shah (mkshah) wrote:
>>>>
>>>>
>>>> On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
>>>>> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>>>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>>>
>>>>>> All rpmh_*() APIs so far have supported placing votes for various
>>>>>> resource settings but the H/W also have option to read resource
>>>>>> settings.
>>>>>>
>>>>>> This change adds a new rpmh_read() API to allow clients
>>>>>> to read back resource setting from H/W. This will be useful for
>>>>>> clients like regulators, which currently don't have a way to know
>>>>>> the settings applied during bootloader stage.
>>>>>>
>>>>>
>>>>> Allow me to express my disappointment over the fact that you sat on this
>>>>> for 7 years!
>>>>
>>>> This was a dead API (even in downstream) with no user since SDM845/ 7 years.
>>>> Read support was eventually removed from downstream driver too for the same reason.
>>>> There were early discussions to remove read support from RSC H/W, due to lack of users.
>>>> Its not realized yet and all SoCs still supports read.
>>>
>>> Can we read BCM states from HLOS this way too?
>>
>> Yes, Any of ARC/BCM/VRM can be read to get HLOS/DRV2 votes.
> 
> Wow this is amazing..
> 
> Do you have code for this already, or should I hack on it?

No, it won't be of much help, as i said above it gets HLOS/DRV2 votes only for a given resource.
More specifically, the read does not give the aggregated vote result across all the DRVs.

Thanks,
Maulik

> 
> Konrad

Re: [PATCH v2 0/4] Add support to read RPMH regulator settings

[Kamal Wadhwa]

On Wed, Oct 22, 2025 at 04:02:00PM -0500, Bjorn Andersson wrote:
> On Wed, Oct 22, 2025 at 02:38:52AM +0530, Kamal Wadhwa wrote:
> > This patch series adds a new `rpmh_read()` API to allow reading RPMH
> > addresses. Also, using this new API, enhances the RPMH regulator driver
> > get_regulator* APIs like `get_regulator_sel()`, `get_mode()` and
> > `get_status()` to allow reading voltage/mode/enable setting from H/W.
> > 
> > This is needed because current design has a limitation - regulator
> > framework can only get the cached values from the last voltage set
> > operation. Because of this right after bootup a `get_voltage_sel()`
> > from regulator framework will return -ENOTRECOVERABLE error, causing
> > regulator framework to trigger an unnecessary `set_voltage_sel()` call
> > with the `min_uV` value specified in the regulator's device tree
> > settings, which can cause issues for consumers like the display and
> > UFS that require a consistent voltage setting from the bootloader
> > state until their drivers are probed.
> > 
> > With this change regulator framework will get the regulator voltage
> > and other settings, as configured during bootloader stage, avoiding
> > unnecessary voltage adjustments and maintaining consistent power
> > settings across the transition from bootloader to kernel.
> > 
> > Besides this feature this series also fixes:-
> > - An existing issue with the BOB5 pass code value. This is needed
> >   by `get_status()`.
> > - Code format/style related errors reported by checkpatch.pl.
> > 
> > Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> > ---
> > Changes in v2:
> > - Fixed the BOB bypass mode handling (existing issue in current driver).
> >   This was needed for `get_status()` implementation.
> > - Implemented `get_status()` callback.
> > - Callbacks for `is_enabled()` & `get_mode()` will now be used as-is
> >   ie. v1 changes reverted.
> > - Bootstapped the read values for `mode` and `status` in probe, based on
> >   comments recieved from reviewer. 
> > - Callback for `get_voltage_sel()` has been modified to handle cases
> >   where read voltage is out-of-range defined in the regulator DT settings,
> >   this is needed to ensure backward compatibilty. Regulator probes may
> >   fail otherwise for some older targets.
> > - This patch is rebased & tested on:
> >    https://lore.kernel.org/all/176070318151.57631.15443673679580823321.b4-ty@kernel.org/
> >   to avoid any merge issues.
> > - Fixed code style issues reported by checkpatch.pl script. 
> > - Link to v1: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-0-ae583d260195@oss.qualcomm.com
> > 
> > ---
> > Kamal Wadhwa (3):
> >       regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling
> >       regulator: qcom-rpmh: Add support to read regulator settings
> >       regulators: qcom-rpmh-regulator: Fix coding style issues
> 
> These three changes all changes the same one file, but you have 3
> different subject prefixes. This is just sloppy, please follow other
> changes to this one file.

sorry will take care of this in the next version.

> 
> > 
> > Maulik Shah (1):
> >       soc: qcom: rpmh: Add support to read back resource settings
> 
> I'd like to merge this through the soc tree, please put it first in the
> series to simplify this.

Ok will do that in next series.

> 
> Regards,
> Bjorn
> 
> > 
> >  drivers/regulator/qcom-rpmh-regulator.c | 184 +++++++++++++++++++++++++++++++-
> >  drivers/soc/qcom/rpmh-rsc.c             |  13 ++-
> >  drivers/soc/qcom/rpmh.c                 |  47 +++++++-
> >  include/soc/qcom/rpmh.h                 |   5 +
> >  include/soc/qcom/tcs.h                  |   2 +
> >  5 files changed, 241 insertions(+), 10 deletions(-)
> > ---
> > base-commit: fe45352cd106ae41b5ad3f0066c2e54dbb2dfd70
> > change-id: 20250623-add-rpmh-read-support-3288f83cc20a
> > 
> > Best regards,
> > -- 
> > Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> > 

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Dmitry Baryshkov]

On Wed, Oct 22, 2025 at 04:15:51PM +0100, Mark Brown wrote:
> On Wed, Oct 22, 2025 at 06:11:46PM +0300, Dmitry Baryshkov wrote:
> > On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote:
> > > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote:
> > > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> 
> > > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> > > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> > > >> value for even if its a PMIC5 BOB.
> 
> > > > The universe will end, the Sun will explode and Ragnarök will be upon
> > > > us. Please describe the issue, why sending bypass value is bad.
> 
> > > I think you misread, it sends the magic value which corresponds
> > > to BYPASS, but one that worked for the previous generation
> 
> > I just wanted to point out that the commit message makes a statement
> > that it sends some value. It should document, why the sent value is bad.
> 
> It seems fairly clear to me from the above that the driver is sending
> the value for the wrong device type which is something so obviously
> wrong I'm not sure it requires further explanation.

Okay. I'm sorry if I'm overreacting.

The bypass_supported field still needs to go away in my opinion.

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Kamal Wadhwa]

On Thu, Oct 23, 2025 at 02:37:07PM +0300, Dmitry Baryshkov wrote:
> On Wed, Oct 22, 2025 at 04:15:51PM +0100, Mark Brown wrote:
> > On Wed, Oct 22, 2025 at 06:11:46PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote:
> > > > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote:
> > > > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> > 
> > > > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> > > > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> > > > >> value for even if its a PMIC5 BOB.
> > 
> > > > > The universe will end, the Sun will explode and Ragnarök will be upon
> > > > > us. Please describe the issue, why sending bypass value is bad.
> > 
> > > > I think you misread, it sends the magic value which corresponds
> > > > to BYPASS, but one that worked for the previous generation
> > 
> > > I just wanted to point out that the commit message makes a statement
> > > that it sends some value. It should document, why the sent value is bad.
> > 
> > It seems fairly clear to me from the above that the driver is sending
> > the value for the wrong device type which is something so obviously
> > wrong I'm not sure it requires further explanation.
> 
> Okay. I'm sorry if I'm overreacting.
> 
> The bypass_supported field still needs to go away in my opinion.

@Dmitry - one way to avoid it is if i re-use `.pmic_bypass_mode` and
keep it  `= -EINVAL` for the checking if the bypass mode is not
supported? and drop the `.bypass_supported`.

But do note that currently only BOB type regulator supports bypass
mode, and this above change will also require modifying all of the
existing (and future) configs for regulator types that do not support
bypass control.

In all below 28 structures we will have to define
`.pmic_bypass_mode = -EINVAL` 

static const struct rpmh_vreg_hw_data pmic4_pldo = {
static const struct rpmh_vreg_hw_data pmic4_pldo_lv = {
static const struct rpmh_vreg_hw_data pmic4_nldo = {
static const struct rpmh_vreg_hw_data pmic4_hfsmps3 = {
static const struct rpmh_vreg_hw_data pmic4_ftsmps426 = {
static const struct rpmh_vreg_hw_data pmic4_lvs = {
static const struct rpmh_vreg_hw_data pmic5_pldo = {
static const struct rpmh_vreg_hw_data pmic5_pldo_lv = {
static const struct rpmh_vreg_hw_data pmic5_pldo515_mv = {
static const struct rpmh_vreg_hw_data pmic5_pldo502 = {
static const struct rpmh_vreg_hw_data pmic5_pldo502ln = {
static const struct rpmh_vreg_hw_data pmic5_nldo = {
static const struct rpmh_vreg_hw_data pmic5_nldo515 = {
static const struct rpmh_vreg_hw_data pmic5_nldo502 = {
static const struct rpmh_vreg_hw_data pmic5_hfsmps510 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps510 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps520 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps525 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps527 = {
static const struct rpmh_vreg_hw_data pmic5_hfsmps515 = {
static const struct rpmh_vreg_hw_data pmic5_hfsmps515_1 = {
static const struct rpmh_vreg_hw_data pmic5_nldo530 = {
static const struct rpmh_vreg_hw_data pmic5_pldo530_mvp150 = {
static const struct rpmh_vreg_hw_data pmic5_pldo530_mvp300 = {
static const struct rpmh_vreg_hw_data pmic5_pldo530_mvp600 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps530 = {

while in the current patch i dont need to touch any of these above
structures and just add new property and define it whereever
`bypass_supported` is set to true.

i.e just change these 2 bob nodes only.

static const struct rpmh_vreg_hw_data pmic5_bob = {
static const struct rpmh_vreg_hw_data pmic4_bob = {

Please suggest, if we can do this in a better way.

Regards,
Kamal

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Konrad Dybcio]

On 10/23/25 11:46 AM, Maulik Shah (mkshah) wrote:
> 
> 
> On 10/23/2025 2:39 PM, Konrad Dybcio wrote:
>> On 10/23/25 10:57 AM, Maulik Shah (mkshah) wrote:
>>>
>>>
>>> On 10/23/2025 1:47 PM, Konrad Dybcio wrote:
>>>> On 10/23/25 6:46 AM, Maulik Shah (mkshah) wrote:
>>>>>
>>>>>
>>>>> On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
>>>>>> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>>>>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>>>>
>>>>>>> All rpmh_*() APIs so far have supported placing votes for various
>>>>>>> resource settings but the H/W also have option to read resource
>>>>>>> settings.
>>>>>>>
>>>>>>> This change adds a new rpmh_read() API to allow clients
>>>>>>> to read back resource setting from H/W. This will be useful for
>>>>>>> clients like regulators, which currently don't have a way to know
>>>>>>> the settings applied during bootloader stage.
>>>>>>>
>>>>>>
>>>>>> Allow me to express my disappointment over the fact that you sat on this
>>>>>> for 7 years!
>>>>>
>>>>> This was a dead API (even in downstream) with no user since SDM845/ 7 years.
>>>>> Read support was eventually removed from downstream driver too for the same reason.
>>>>> There were early discussions to remove read support from RSC H/W, due to lack of users.
>>>>> Its not realized yet and all SoCs still supports read.
>>>>
>>>> Can we read BCM states from HLOS this way too?
>>>
>>> Yes, Any of ARC/BCM/VRM can be read to get HLOS/DRV2 votes.
>>
>> Wow this is amazing..
>>
>> Do you have code for this already, or should I hack on it?
> 
> No, it won't be of much help, as i said above it gets HLOS/DRV2 votes only for a given resource.
> More specifically, the read does not give the aggregated vote result across all the DRVs.

Hm, perhaps it could still be of *some* use

But maybe reading back rpmhpd and rpmhcc states would be of more
use!

Konrad

Re: [PATCH v2 1/4] regulator: rpmh-regulator: Fix PMIC5 BOB bypass mode handling

[Dmitry Baryshkov]

On Fri, Oct 24, 2025 at 01:33:58PM +0530, Kamal Wadhwa wrote:
> On Thu, Oct 23, 2025 at 02:37:07PM +0300, Dmitry Baryshkov wrote:
> > On Wed, Oct 22, 2025 at 04:15:51PM +0100, Mark Brown wrote:
> > > On Wed, Oct 22, 2025 at 06:11:46PM +0300, Dmitry Baryshkov wrote:
> > > > On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote:
> > > > > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote:
> > > > > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> > > 
> > > > > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> > > > > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> > > > > >> value for even if its a PMIC5 BOB.
> > > 
> > > > > > The universe will end, the Sun will explode and Ragnarök will be upon
> > > > > > us. Please describe the issue, why sending bypass value is bad.
> > > 
> > > > > I think you misread, it sends the magic value which corresponds
> > > > > to BYPASS, but one that worked for the previous generation
> > > 
> > > > I just wanted to point out that the commit message makes a statement
> > > > that it sends some value. It should document, why the sent value is bad.
> > > 
> > > It seems fairly clear to me from the above that the driver is sending
> > > the value for the wrong device type which is something so obviously
> > > wrong I'm not sure it requires further explanation.
> > 
> > Okay. I'm sorry if I'm overreacting.
> > 
> > The bypass_supported field still needs to go away in my opinion.
> 
> @Dmitry - one way to avoid it is if i re-use `.pmic_bypass_mode` and
> keep it  `= -EINVAL` for the checking if the bypass mode is not
> supported? and drop the `.bypass_supported`.

You don't need .bypass_supported because the regulators that don't
support bypass don't have .set_bypass callback in their ops and as such
it is impossible to get the vreg->bypassed flag to be set.

> But do note that currently only BOB type regulator supports bypass
> mode, and this above change will also require modifying all of the
> existing (and future) configs for regulator types that do not support
> bypass control.

[...]

> 
> while in the current patch i dont need to touch any of these above
> structures and just add new property and define it whereever
> `bypass_supported` is set to true.
> 
> i.e just change these 2 bob nodes only.
> 
> static const struct rpmh_vreg_hw_data pmic5_bob = {
> static const struct rpmh_vreg_hw_data pmic4_bob = {
> 
> Please suggest, if we can do this in a better way.

Don't change any of the nodes, the necessary changes are already in
place. Just fix the value being programmed for PMIC5.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Neil Armstrong]

On 10/27/25 14:29, Konrad Dybcio wrote:
> On 10/23/25 11:46 AM, Maulik Shah (mkshah) wrote:
>>
>>
>> On 10/23/2025 2:39 PM, Konrad Dybcio wrote:
>>> On 10/23/25 10:57 AM, Maulik Shah (mkshah) wrote:
>>>>
>>>>
>>>> On 10/23/2025 1:47 PM, Konrad Dybcio wrote:
>>>>> On 10/23/25 6:46 AM, Maulik Shah (mkshah) wrote:
>>>>>>
>>>>>>
>>>>>> On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
>>>>>>> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>>>>>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>>>>>
>>>>>>>> All rpmh_*() APIs so far have supported placing votes for various
>>>>>>>> resource settings but the H/W also have option to read resource
>>>>>>>> settings.
>>>>>>>>
>>>>>>>> This change adds a new rpmh_read() API to allow clients
>>>>>>>> to read back resource setting from H/W. This will be useful for
>>>>>>>> clients like regulators, which currently don't have a way to know
>>>>>>>> the settings applied during bootloader stage.
>>>>>>>>
>>>>>>>
>>>>>>> Allow me to express my disappointment over the fact that you sat on this
>>>>>>> for 7 years!
>>>>>>
>>>>>> This was a dead API (even in downstream) with no user since SDM845/ 7 years.
>>>>>> Read support was eventually removed from downstream driver too for the same reason.
>>>>>> There were early discussions to remove read support from RSC H/W, due to lack of users.
>>>>>> Its not realized yet and all SoCs still supports read.
>>>>>
>>>>> Can we read BCM states from HLOS this way too?
>>>>
>>>> Yes, Any of ARC/BCM/VRM can be read to get HLOS/DRV2 votes.
>>>
>>> Wow this is amazing..
>>>
>>> Do you have code for this already, or should I hack on it?
>>
>> No, it won't be of much help, as i said above it gets HLOS/DRV2 votes only for a given resource.
>> More specifically, the read does not give the aggregated vote result across all the DRVs.
> 
> Hm, perhaps it could still be of *some* use
> 
> But maybe reading back rpmhpd and rpmhcc states would be of more
> use!

The interconnect core definitely supports reading back the state at boot.

Neil

> 
> Konrad

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Konrad Dybcio]

On 10/27/25 3:38 PM, Neil Armstrong wrote:
> On 10/27/25 14:29, Konrad Dybcio wrote:
>> On 10/23/25 11:46 AM, Maulik Shah (mkshah) wrote:
>>>
>>>
>>> On 10/23/2025 2:39 PM, Konrad Dybcio wrote:
>>>> On 10/23/25 10:57 AM, Maulik Shah (mkshah) wrote:
>>>>>
>>>>>
>>>>> On 10/23/2025 1:47 PM, Konrad Dybcio wrote:
>>>>>> On 10/23/25 6:46 AM, Maulik Shah (mkshah) wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
>>>>>>>> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
>>>>>>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>>>>>>
>>>>>>>>> All rpmh_*() APIs so far have supported placing votes for various
>>>>>>>>> resource settings but the H/W also have option to read resource
>>>>>>>>> settings.
>>>>>>>>>
>>>>>>>>> This change adds a new rpmh_read() API to allow clients
>>>>>>>>> to read back resource setting from H/W. This will be useful for
>>>>>>>>> clients like regulators, which currently don't have a way to know
>>>>>>>>> the settings applied during bootloader stage.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Allow me to express my disappointment over the fact that you sat on this
>>>>>>>> for 7 years!
>>>>>>>
>>>>>>> This was a dead API (even in downstream) with no user since SDM845/ 7 years.
>>>>>>> Read support was eventually removed from downstream driver too for the same reason.
>>>>>>> There were early discussions to remove read support from RSC H/W, due to lack of users.
>>>>>>> Its not realized yet and all SoCs still supports read.
>>>>>>
>>>>>> Can we read BCM states from HLOS this way too?
>>>>>
>>>>> Yes, Any of ARC/BCM/VRM can be read to get HLOS/DRV2 votes.
>>>>
>>>> Wow this is amazing..
>>>>
>>>> Do you have code for this already, or should I hack on it?
>>>
>>> No, it won't be of much help, as i said above it gets HLOS/DRV2 votes only for a given resource.
>>> More specifically, the read does not give the aggregated vote result across all the DRVs.
>>
>> Hm, perhaps it could still be of *some* use
>>
>> But maybe reading back rpmhpd and rpmhcc states would be of more
>> use!
> 
> The interconnect core definitely supports reading back the state at boot.

Maulik probably isn't impressed with us only being able to provide
information about HLOS votes, as e.g. ADSP could be voting on the same
bus in parallel.

I suppose the very same applies to what I suggested with clk and rpmhpd
although probably it's less of a problem there

Konrad

Re: [PATCH v2 3/4] regulator: qcom-rpmh: Add support to read regulator settings

[Kamal Wadhwa]

On Wed, Oct 22, 2025 at 11:43:44AM +0300, Dmitry Baryshkov wrote:
> On Wed, Oct 22, 2025 at 02:38:55AM +0530, Kamal Wadhwa wrote:
> > Currently, the RPMH regulator's `get_voltage_sel()` function only
> > returns cached values from the last `set_voltage_sel()` operation.
> > This limitation prevents the regulator framework from accurately
> > reflecting the regulator configurations set during the bootloader
> > stage. As a result, the regulator framework may trigger an
> > unnecessary `set_voltage_sel()` call with the `min_uV` value
> 
> unnecessary or incorrect?

Well for most part its unnecessary and for some cases incorrect.

Its `unnecessary` because most drivers dont have a requirement to keep their
rails ON (as specific voltage) before their driver probe happens. So a drop
in voltage should mostly not impact them.

Only for clients which need continuous voltage can have a possible impact.
(explained in subsequent para)

let me know if you still think the commit needs modifications/improvement.

> 
> > specified in the regulator's device tree settings, which can
> > cause issues for consumers like the display and UFS that require
> > a consistent voltage setting from the bootloader state until
> > their drivers are probed.
> 
> It sounds like this should be handled through the .sync_state rather
> than just reading the voltage. Please correct me if I'm wrong, even with
> the .get_voltage_sel() in place the regulator framework still can lower
> the vote.

I think i understand, what problem you are alluding to.. for which .sync_state
is needed. We have a different series(with sync_state() logic) under testing
for that. We will be posting that out soon.

(Why sending it in 2 series?)

To elaborate,
there are 3 possible cases for a rail that is ON during boot:-

Case#1 (no client probe() voting for voltage) 
------
Don't care - regulator framework will turn OFF unused rails in late_cleanup()


case#2 (1 client needs rail ON during boot and votes for it )
------
We have real use case for UFS2.0/3.0 where a (dedicated/unshared) rail is
turned ON by bootloader with voltage that is good for UFS2.0 but gets lowered
to `min-microvolt` voltage (OK for UFS3.0, but NOK for UFS2.0) by the regulator
framework because it can't read the voltage set in boot, leading to UFS2.0 to
malfunction or get damaged.

NOTE - This is `what` we are handling in this series. Avoiding the unnecessary
overwriting of the voltage to min, by regulator framework by providing it a
way to read voltage set in boot.


case#3 (2 or more clients case) 
------
Voltage/ON/OFF state can be different based on the order of client probe().

To avoid issues due to a possible race, it needs `.sync_state()`to hold the
voltage to a value >= boot voltage.

But this we have planned to handle in next series, as that would probably
make this series more complicated.

(do you think we should merge this together?)

> 
> > 
> > To address this issue, enhance the `get_voltage_sel()`, and also
> > add new `get_status()` callbacks to read the regulator settings
> > directly from the RPMH hardware using the `rpmh_read()`function.
> > This change ensures that the regulator framework accurately
> > reflects the actual state of the regulators, avoiding unnecessary
> > voltage adjustments and maintaining consistent power settings
> > across the transition from bootloader to kernel.
> > 
> > Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
> > Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> > Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-2-ae583d260195@oss.qualcomm.com
> > ---
> >  drivers/regulator/qcom-rpmh-regulator.c | 164 ++++++++++++++++++++++++++++++++
> >  1 file changed, 164 insertions(+)
> > 
> > diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> > index 947fb5241233c92eaeda974b1b64d227d5946a59..9f693043cb87aa77a7a529b5b973323450db80be 100644
> > --- a/drivers/regulator/qcom-rpmh-regulator.c
> > +++ b/drivers/regulator/qcom-rpmh-regulator.c
> > @@ -61,8 +61,13 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
> >  };
> >  
> >  #define RPMH_REGULATOR_REG_VRM_VOLTAGE		0x0
> > +#define RPMH_REGULATOR_VOLTAGE_MASK		0x1FFF
> > +
> >  #define RPMH_REGULATOR_REG_ENABLE		0x4
> > +#define RPMH_REGULATOR_ENABLE_MASK		0x1
> > +
> >  #define RPMH_REGULATOR_REG_VRM_MODE		0x8
> > +#define RPMH_REGULATOR_MODE_MASK		0x7
> >  
> >  #define PMIC4_LDO_MODE_RETENTION		4
> >  #define PMIC4_LDO_MODE_LPM			5
> > @@ -169,6 +174,7 @@ struct rpmh_vreg {
> >  	bool				bypassed;
> >  	int				voltage_selector;
> >  	unsigned int			mode;
> > +	unsigned int			status;
> >  };
> >  
> >  /**
> > @@ -213,6 +219,36 @@ static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
> >  	return ret;
> >  }
> >  
> > +static int rpmh_regulator_read_data(struct rpmh_vreg *vreg, struct tcs_cmd *cmd)
> > +{
> > +	return rpmh_read(vreg->dev, cmd);
> > +}
> > +
> > +static int _rpmh_regulator_vrm_get_voltage(struct regulator_dev *rdev, int *uV)
> > +{
> > +	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> > +	struct tcs_cmd cmd = {
> > +		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> > +	};
> > +	int min_uV = rdev->constraints->min_uV;
> > +	int max_uV = rdev->constraints->max_uV;
> > +	int ret, _uV = 0;
> > +
> > +	ret = rpmh_regulator_read_data(vreg, &cmd);
> > +	if (!ret)
> > +		_uV = (cmd.data & RPMH_REGULATOR_VOLTAGE_MASK) * 1000;
> > +	else
> > +		dev_err(vreg->dev, "failed to read VOLTAGE ret = %d\n", ret);
> > +
> > +	if (!_uV || (_uV >= min_uV && _uV <= max_uV))
> > +		*uV = _uV;
> > +	else
> > +		dev_dbg(vreg->dev, "read voltage %d is out-of-range[%d:%d]\n",
> > +						_uV, min_uV, max_uV);
> > +
> > +	return ret;
> > +}
> > +
> >  static int _rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
> >  				unsigned int selector, bool wait_for_ack)
> >  {
> > @@ -254,10 +290,36 @@ static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
> >  static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
> >  {
> >  	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> > +	int ret, uV = 0;
> > +
> > +	if (vreg->voltage_selector < 0) {
> 
> Why do we return the cached value instead of always reading it from the
> hardware?

The rpmh register that we are reading here is dedicated to APPS, it will not
show the `actual` voltage on the rail, but the voltage that is voted from APPS.

So its only needed when the device boots and we have no cached value for the
voltage. However, once the value is set once, the cache value will be always
a shadow copy of what is written in the rpmh register.

side note - Mark had initially (in v1) suggested to bootstrape mode & status
and keep get_voltage_sel() as is, so i did not bootstrape voltage.

> 
> > +		ret = _rpmh_regulator_vrm_get_voltage(rdev, &uV);
> > +		if (!ret && uV != 0)
> > +			vreg->voltage_selector = regulator_map_voltage_linear_range(rdev,
> > +							uV, INT_MAX);
> > +	}
> >  
> >  	return vreg->voltage_selector;
> >  }
> >  
> > +static enum regulator_status convert_mode_to_status(int mode)
> > +{
> > +	switch (mode) {
> > +	case REGULATOR_MODE_FAST:
> > +		return REGULATOR_STATUS_FAST;
> > +	case REGULATOR_MODE_NORMAL:
> > +		return REGULATOR_STATUS_NORMAL;
> > +	case REGULATOR_MODE_IDLE:
> > +		return REGULATOR_STATUS_IDLE;
> > +	case REGULATOR_MODE_STANDBY:
> > +		return REGULATOR_STATUS_STANDBY;
> > +	case REGULATOR_MODE_INVALID:
> > +		return REGULATOR_STATUS_ERROR;
> > +	default:
> > +		return REGULATOR_STATUS_UNDEFINED;
> > +	};
> > +}
> > +
> >  static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
> >  {
> >  	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> > @@ -287,6 +349,15 @@ static int rpmh_regulator_set_enable_state(struct regulator_dev *rdev,
> >  	if (!ret)
> >  		vreg->enabled = enable;
> >  
> > +	if (vreg->enabled) {
> > +		if (vreg->bypassed)
> > +			vreg->status = REGULATOR_STATUS_BYPASS;
> > +		else
> > +			vreg->status = convert_mode_to_status(vreg->mode);
> > +	} else {
> > +		vreg->status = REGULATOR_STATUS_OFF;
> > +	}
> > +
> >  	return ret;
> >  }
> >  
> > @@ -323,6 +394,15 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
> >  		cmd.data = pmic_mode;
> >  	}
> >  
> > +	if (vreg->enabled) {
> > +		if (bypassed)
> > +			vreg->status = REGULATOR_STATUS_BYPASS;
> > +		else
> > +			vreg->status = convert_mode_to_status(mode);
> > +	} else {
> > +		vreg->status = REGULATOR_STATUS_OFF;
> > +	}
> > +
> >  	return rpmh_regulator_send_request(vreg, &cmd, true);
> >  }
> >  
> > @@ -342,6 +422,22 @@ static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
> >  	return ret;
> >  }
> >  
> > +static int rpmh_regulator_vrm_get_pmic_mode(struct rpmh_vreg *vreg, int *pmic_mode)
> > +{
> > +	struct tcs_cmd cmd = {
> > +		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> > +	};
> > +	int ret;
> > +
> > +	ret = rpmh_regulator_read_data(vreg, &cmd);
> > +	if (!ret)
> > +		*pmic_mode = cmd.data & RPMH_REGULATOR_MODE_MASK;
> > +	else
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> >  static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
> >  {
> >  	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> > @@ -349,6 +445,13 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
> >  	return vreg->mode;
> >  }
> >  
> > +static int rpmh_regulator_vrm_get_status(struct regulator_dev *rdev)
> > +{
> > +	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> > +
> > +	return vreg->status;
> > +}
> > +
> >  /**
> >   * rpmh_regulator_vrm_get_optimum_mode() - get the mode based on the  load
> >   * @rdev:		Regulator device pointer for the rpmh-regulator
> > @@ -407,6 +510,7 @@ static const struct regulator_ops rpmh_regulator_vrm_ops = {
> >  	.list_voltage		= regulator_list_voltage_linear_range,
> >  	.set_mode		= rpmh_regulator_vrm_set_mode,
> >  	.get_mode		= rpmh_regulator_vrm_get_mode,
> > +	.get_status		= rpmh_regulator_vrm_get_status,
> >  };
> >  
> >  static const struct regulator_ops rpmh_regulator_vrm_drms_ops = {
> > @@ -418,6 +522,7 @@ static const struct regulator_ops rpmh_regulator_vrm_drms_ops = {
> >  	.list_voltage		= regulator_list_voltage_linear_range,
> >  	.set_mode		= rpmh_regulator_vrm_set_mode,
> >  	.get_mode		= rpmh_regulator_vrm_get_mode,
> > +	.get_status		= rpmh_regulator_vrm_get_status,
> >  	.get_optimum_mode	= rpmh_regulator_vrm_get_optimum_mode,
> >  };
> >  
> > @@ -430,6 +535,7 @@ static const struct regulator_ops rpmh_regulator_vrm_bypass_ops = {
> >  	.list_voltage		= regulator_list_voltage_linear_range,
> >  	.set_mode		= rpmh_regulator_vrm_set_mode,
> >  	.get_mode		= rpmh_regulator_vrm_get_mode,
> > +	.get_status		= rpmh_regulator_vrm_get_status,
> >  	.set_bypass		= rpmh_regulator_vrm_set_bypass,
> >  	.get_bypass		= rpmh_regulator_vrm_get_bypass,
> >  };
> > @@ -438,6 +544,7 @@ static const struct regulator_ops rpmh_regulator_xob_ops = {
> >  	.enable			= rpmh_regulator_enable,
> >  	.disable		= rpmh_regulator_disable,
> >  	.is_enabled		= rpmh_regulator_is_enabled,
> > +	.get_status		= rpmh_regulator_vrm_get_status,
> >  };
> >  
> >  /**
> > @@ -546,6 +653,58 @@ static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
> >  	return 0;
> >  }
> >  
> > +static int rpmh_regulator_determine_initial_status(struct rpmh_vreg *vreg)
> > +{
> > +	struct tcs_cmd cmd = {
> > +		.addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
> > +	};
> > +	int ret, pmic_mode, mode;
> > +	int sts = 0;
> > +
> > +	ret = rpmh_regulator_read_data(vreg, &cmd);
> > +	if (ret) {
> > +		dev_dbg(vreg->dev, "failed to read ENABLE status ret = %d\n", ret);
> > +		vreg->status = REGULATOR_STATUS_UNDEFINED;
> > +		return ret;
> > +	}
> > +
> > +	sts = cmd.data & RPMH_REGULATOR_ENABLE_MASK;
> > +	if (!sts) {
> > +		vreg->status = REGULATOR_STATUS_OFF;
> > +		return 0;
> > +	}
> > +
> > +	if (vreg->hw_data->regulator_type == XOB) {
> > +		vreg->status = sts ? REGULATOR_STATUS_ON : REGULATOR_STATUS_OFF;
> > +		return 0;
> > +	}
> > +
> > +	ret = rpmh_regulator_vrm_get_pmic_mode(vreg, &pmic_mode);
> > +	if (ret < 0) {
> > +		dev_dbg(vreg->dev, "failed to read pmic_mode ret = %d\n", ret);
> > +		vreg->mode = REGULATOR_MODE_INVALID;
> > +		vreg->status = REGULATOR_STATUS_UNDEFINED;
> > +		return ret;
> > +	}
> > +
> > +	if (vreg->hw_data->bypass_supported &&
> > +			vreg->hw_data->pmic_bypass_mode == pmic_mode) {
> 
> Wrong indentation

sorry will fix this.

> 
> > +		vreg->bypassed = true;
> > +		vreg->status = REGULATOR_STATUS_BYPASS;
> > +		return 0;
> > +	}
> > +
> > +	for (mode = 0; mode <= REGULATOR_MODE_STANDBY; mode++) {
> > +		if (pmic_mode == vreg->hw_data->pmic_mode_map[mode]) {
> > +			vreg->mode = mode;
> > +			break;
> > +		}
> > +	}
> > +
> > +	vreg->status = convert_mode_to_status(vreg->mode);
> > +	return 0;
> > +}
> > +
> >  static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> >  	[REGULATOR_MODE_INVALID] = -EINVAL,
> >  	[REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
> > @@ -1820,6 +1979,11 @@ static int rpmh_regulator_probe(struct platform_device *pdev)
> >  						vreg_data);
> >  		if (ret < 0)
> >  			return ret;
> > +
> > +		ret = rpmh_regulator_determine_initial_status(vreg);
> > +		if (ret < 0)
> > +			dev_err(dev, "failed to read initial status for %s\n",
> > +					vreg->rdesc.name);
> >  	}
> >  
> >  	return 0;
> > 
> > -- 
> > 2.25.1
> > 
> 
> -- 
> With best wishes
> Dmitry


Regards,
Kamal

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Bjorn Andersson]

On Mon, Oct 27, 2025 at 04:47:26PM +0100, Konrad Dybcio wrote:
> On 10/27/25 3:38 PM, Neil Armstrong wrote:
> > On 10/27/25 14:29, Konrad Dybcio wrote:
> >> On 10/23/25 11:46 AM, Maulik Shah (mkshah) wrote:
> >>>
> >>>
> >>> On 10/23/2025 2:39 PM, Konrad Dybcio wrote:
> >>>> On 10/23/25 10:57 AM, Maulik Shah (mkshah) wrote:
> >>>>>
> >>>>>
> >>>>> On 10/23/2025 1:47 PM, Konrad Dybcio wrote:
> >>>>>> On 10/23/25 6:46 AM, Maulik Shah (mkshah) wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 10/23/2025 2:51 AM, Bjorn Andersson wrote:
> >>>>>>>> On Wed, Oct 22, 2025 at 02:38:54AM +0530, Kamal Wadhwa wrote:
> >>>>>>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
> >>>>>>>>>
> >>>>>>>>> All rpmh_*() APIs so far have supported placing votes for various
> >>>>>>>>> resource settings but the H/W also have option to read resource
> >>>>>>>>> settings.
> >>>>>>>>>
> >>>>>>>>> This change adds a new rpmh_read() API to allow clients
> >>>>>>>>> to read back resource setting from H/W. This will be useful for
> >>>>>>>>> clients like regulators, which currently don't have a way to know
> >>>>>>>>> the settings applied during bootloader stage.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Allow me to express my disappointment over the fact that you sat on this
> >>>>>>>> for 7 years!
> >>>>>>>
> >>>>>>> This was a dead API (even in downstream) with no user since SDM845/ 7 years.
> >>>>>>> Read support was eventually removed from downstream driver too for the same reason.
> >>>>>>> There were early discussions to remove read support from RSC H/W, due to lack of users.
> >>>>>>> Its not realized yet and all SoCs still supports read.
> >>>>>>
> >>>>>> Can we read BCM states from HLOS this way too?
> >>>>>
> >>>>> Yes, Any of ARC/BCM/VRM can be read to get HLOS/DRV2 votes.
> >>>>
> >>>> Wow this is amazing..
> >>>>
> >>>> Do you have code for this already, or should I hack on it?
> >>>
> >>> No, it won't be of much help, as i said above it gets HLOS/DRV2 votes only for a given resource.
> >>> More specifically, the read does not give the aggregated vote result across all the DRVs.
> >>
> >> Hm, perhaps it could still be of *some* use
> >>
> >> But maybe reading back rpmhpd and rpmhcc states would be of more
> >> use!
> > 
> > The interconnect core definitely supports reading back the state at boot.
> 
> Maulik probably isn't impressed with us only being able to provide
> information about HLOS votes, as e.g. ADSP could be voting on the same
> bus in parallel.
> 
> I suppose the very same applies to what I suggested with clk and rpmhpd
> although probably it's less of a problem there
> 

Reading back the state serves the purpose of dealing with smooth
transition from bootloader, which we very much would like to have.

Being able to read the aggregated state is useful for debugging, but
it's a shared state that can change at any point in time, so we should
never act upon such information.

Regards,
Bjorn

> Konrad

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Konrad Dybcio]

On 10/21/25 11:08 PM, Kamal Wadhwa wrote:
> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
> 
> All rpmh_*() APIs so far have supported placing votes for various
> resource settings but the H/W also have option to read resource
> settings.
> 
> This change adds a new rpmh_read() API to allow clients
> to read back resource setting from H/W. This will be useful for
> clients like regulators, which currently don't have a way to know
> the settings applied during bootloader stage.
> 
> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com
> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---

[...]

> +int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)
> +{
> +	DECLARE_COMPLETION_ONSTACK(compl);
> +	DEFINE_RPMH_MSG_ONSTACK(dev, RPMH_ACTIVE_ONLY_STATE, &compl, rpm_msg);
> +	int ret;
> +
> +	ret = __fill_rpmh_msg(&rpm_msg, RPMH_ACTIVE_ONLY_STATE, cmd, 1, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = __rpmh_write(dev, RPMH_ACTIVE_ONLY_STATE, &rpm_msg);

Is there a reason for making this ACTIVE_ONLY?

Konrad

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Maulik Shah (mkshah)]

On 11/12/2025 4:56 PM, Konrad Dybcio wrote:
> On 10/21/25 11:08 PM, Kamal Wadhwa wrote:
>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>
>> All rpmh_*() APIs so far have supported placing votes for various
>> resource settings but the H/W also have option to read resource
>> settings.
>>
>> This change adds a new rpmh_read() API to allow clients
>> to read back resource setting from H/W. This will be useful for
>> clients like regulators, which currently don't have a way to know
>> the settings applied during bootloader stage.
>>
>> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com
>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)
>> +{
>> +	DECLARE_COMPLETION_ONSTACK(compl);
>> +	DEFINE_RPMH_MSG_ONSTACK(dev, RPMH_ACTIVE_ONLY_STATE, &compl, rpm_msg);
>> +	int ret;
>> +
>> +	ret = __fill_rpmh_msg(&rpm_msg, RPMH_ACTIVE_ONLY_STATE, cmd, 1, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = __rpmh_write(dev, RPMH_ACTIVE_ONLY_STATE, &rpm_msg);
> 
> Is there a reason for making this ACTIVE_ONLY?

Yes, using ACTIVE_ONLY makes the read request place immediately to read back the current resource setting.
Sleep/Wake are H/W based trigger and are not applicable for this API.

Thanks,
Maulik

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Konrad Dybcio]

On 11/17/25 9:26 AM, Maulik Shah (mkshah) wrote:
> 
> 
> On 11/12/2025 4:56 PM, Konrad Dybcio wrote:
>> On 10/21/25 11:08 PM, Kamal Wadhwa wrote:
>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>
>>> All rpmh_*() APIs so far have supported placing votes for various
>>> resource settings but the H/W also have option to read resource
>>> settings.
>>>
>>> This change adds a new rpmh_read() API to allow clients
>>> to read back resource setting from H/W. This will be useful for
>>> clients like regulators, which currently don't have a way to know
>>> the settings applied during bootloader stage.
>>>
>>> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com
>>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>>> ---
>>
>> [...]
>>
>>> +int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)
>>> +{
>>> +	DECLARE_COMPLETION_ONSTACK(compl);
>>> +	DEFINE_RPMH_MSG_ONSTACK(dev, RPMH_ACTIVE_ONLY_STATE, &compl, rpm_msg);
>>> +	int ret;
>>> +
>>> +	ret = __fill_rpmh_msg(&rpm_msg, RPMH_ACTIVE_ONLY_STATE, cmd, 1, true);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = __rpmh_write(dev, RPMH_ACTIVE_ONLY_STATE, &rpm_msg);
>>
>> Is there a reason for making this ACTIVE_ONLY?
> 
> Yes, using ACTIVE_ONLY makes the read request place immediately to read back the current resource setting.
> Sleep/Wake are H/W based trigger and are not applicable for this API.

Huh? So if I send a read request with e.g. SLEEP_STATE, it would only
get fulfilled upon an active->sleep transition?

Konrad

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Maulik Shah (mkshah)]

On 11/17/2025 6:04 PM, Konrad Dybcio wrote:
> On 11/17/25 9:26 AM, Maulik Shah (mkshah) wrote:
>>
>>
>> On 11/12/2025 4:56 PM, Konrad Dybcio wrote:
>>> On 10/21/25 11:08 PM, Kamal Wadhwa wrote:
>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>
>>>> All rpmh_*() APIs so far have supported placing votes for various
>>>> resource settings but the H/W also have option to read resource
>>>> settings.
>>>>
>>>> This change adds a new rpmh_read() API to allow clients
>>>> to read back resource setting from H/W. This will be useful for
>>>> clients like regulators, which currently don't have a way to know
>>>> the settings applied during bootloader stage.
>>>>
>>>> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com
>>>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>>>> ---
>>>
>>> [...]
>>>
>>>> +int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)
>>>> +{
>>>> +	DECLARE_COMPLETION_ONSTACK(compl);
>>>> +	DEFINE_RPMH_MSG_ONSTACK(dev, RPMH_ACTIVE_ONLY_STATE, &compl, rpm_msg);
>>>> +	int ret;
>>>> +
>>>> +	ret = __fill_rpmh_msg(&rpm_msg, RPMH_ACTIVE_ONLY_STATE, cmd, 1, true);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = __rpmh_write(dev, RPMH_ACTIVE_ONLY_STATE, &rpm_msg);
>>>
>>> Is there a reason for making this ACTIVE_ONLY?
>>
>> Yes, using ACTIVE_ONLY makes the read request place immediately to read back the current resource setting.
>> Sleep/Wake are H/W based trigger and are not applicable for this API.
> 
> Huh? So if I send a read request with e.g. SLEEP_STATE, it would only
> get fulfilled upon an active->sleep transition?
> 

Read requests will get fulfilled immediately with the return of the current resource setting,
there is no separate active/sleep/wake vote values that can be read, put it other way the
rpmh_read() API do not take any "enum rpmh_state state" argument like various rpmh_write_*() APIs.

Thanks,
Maulik

Re: [PATCH v2 2/4] soc: qcom: rpmh: Add support to read back resource settings

[Konrad Dybcio]

On 11/21/25 9:41 AM, Maulik Shah (mkshah) wrote:
> 
> 
> On 11/17/2025 6:04 PM, Konrad Dybcio wrote:
>> On 11/17/25 9:26 AM, Maulik Shah (mkshah) wrote:
>>>
>>>
>>> On 11/12/2025 4:56 PM, Konrad Dybcio wrote:
>>>> On 10/21/25 11:08 PM, Kamal Wadhwa wrote:
>>>>> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>>
>>>>> All rpmh_*() APIs so far have supported placing votes for various
>>>>> resource settings but the H/W also have option to read resource
>>>>> settings.
>>>>>
>>>>> This change adds a new rpmh_read() API to allow clients
>>>>> to read back resource setting from H/W. This will be useful for
>>>>> clients like regulators, which currently don't have a way to know
>>>>> the settings applied during bootloader stage.
>>>>>
>>>>> Link: https://lore.kernel.org/r/20250623-add-rpmh-read-support-v1-1-ae583d260195@oss.qualcomm.com
>>>>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>> +int rpmh_read(const struct device *dev, struct tcs_cmd *cmd)
>>>>> +{
>>>>> +	DECLARE_COMPLETION_ONSTACK(compl);
>>>>> +	DEFINE_RPMH_MSG_ONSTACK(dev, RPMH_ACTIVE_ONLY_STATE, &compl, rpm_msg);
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = __fill_rpmh_msg(&rpm_msg, RPMH_ACTIVE_ONLY_STATE, cmd, 1, true);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	ret = __rpmh_write(dev, RPMH_ACTIVE_ONLY_STATE, &rpm_msg);
>>>>
>>>> Is there a reason for making this ACTIVE_ONLY?
>>>
>>> Yes, using ACTIVE_ONLY makes the read request place immediately to read back the current resource setting.
>>> Sleep/Wake are H/W based trigger and are not applicable for this API.
>>
>> Huh? So if I send a read request with e.g. SLEEP_STATE, it would only
>> get fulfilled upon an active->sleep transition?
>>
> 
> Read requests will get fulfilled immediately with the return of the current resource setting,
> there is no separate active/sleep/wake vote values that can be read, put it other way the
> rpmh_read() API do not take any "enum rpmh_state state" argument like various rpmh_write_*() APIs.

I see, thanks

Konrad