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

[PATCH 0/2] 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
`is_enabled()` 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.

Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
Kamal Wadhwa (1):
      regulator: qcom-rpmh: Add support to read regulator settings

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

 drivers/regulator/qcom-rpmh-regulator.c | 71 +++++++++++++++++++++++++++++++++
 drivers/soc/qcom/rpmh-rsc.c             | 12 +++++-
 drivers/soc/qcom/rpmh.c                 | 54 +++++++++++++++++++++++--
 include/soc/qcom/rpmh.h                 |  7 ++++
 include/soc/qcom/tcs.h                  |  2 +
 5 files changed, 140 insertions(+), 6 deletions(-)
---
base-commit: 393d0c54cae31317deaa9043320c5fd9454deabc
change-id: 20250623-add-rpmh-read-support-3288f83cc20a

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

[PATCH 1/2] 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.

Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
---
 drivers/soc/qcom/rpmh-rsc.c | 12 ++++++++--
 drivers/soc/qcom/rpmh.c     | 54 +++++++++++++++++++++++++++++++++++++++++----
 include/soc/qcom/rpmh.h     |  7 ++++++
 include/soc/qcom/tcs.h      |  2 ++
 4 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index cb82e887b51d4401edba713348eed9621622b73b..18eed2cb8d300e862991efc20be58ff7ead378ce 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);
+		}
+
 		/*
 		 * If wake tcs was re-purposed for sending active
 		 * votes, clear AMC trigger & enable modes and
@@ -496,13 +502,14 @@ 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;
+	cmd_msgid |= (!msg->is_read) ? CMD_MSGID_WRITE : 0;
 
 	for (i = 0, j = cmd_id; i < msg->num_cmds; i++, j++) {
 		cmd = &msg->cmds[i];
@@ -516,7 +523,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..c69f08fc76eea724f2c9933c7e25fbf671820764 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,52 @@ 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
+ * @state: Must be Active state
+ * @cmd: The payload having address of resource to read
+ * @n: The number of elements in @cmd, must be single command
+ *
+ * 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			- Success
+ * * -Error             - Error code
+ */
+int rpmh_read(const struct device *dev, enum rpmh_state state, struct tcs_cmd *cmd, u32 n)
+{
+	int ret;
+	DECLARE_COMPLETION_ONSTACK(compl);
+	DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
+
+	if (n != 1 || state != RPMH_ACTIVE_ONLY_STATE)
+		return -EINVAL;
+
+	ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n, true);
+	if (ret)
+		return ret;
+
+	ret = __rpmh_write(dev, 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 +276,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 +303,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 +398,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..03eea0bc798e6b66a4d47e9ae1bc408de696c059 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -11,6 +11,9 @@
 
 
 #if IS_ENABLED(CONFIG_QCOM_RPMH)
+int rpmh_read(const struct device *dev, enum rpmh_state state,
+	      struct tcs_cmd *cmd, u32 n);
+
 int rpmh_write(const struct device *dev, enum rpmh_state state,
 	       const struct tcs_cmd *cmd, u32 n);
 
@@ -24,6 +27,10 @@ void rpmh_invalidate(const struct device *dev);
 
 #else
 
+static inline int rpmh_read(const struct device *dev, enum rpmh_state state,
+			    struct tcs_cmd *cmd, u32 n)
+{ 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 2/2] 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()`,
`get_mode()`, and `is_enabled()` 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>
---
 drivers/regulator/qcom-rpmh-regulator.c | 71 +++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 7870722b6ee21ce487c2cf911760fb4a385fc44b..ba5bd4ecec7429a1ada008c237cf7444a37a9cc6 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2018-2021, The Linux Foundation. All rights reserved.
 // Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
 
 #define pr_fmt(fmt) "%s: " fmt, __func__
 
@@ -33,8 +34,13 @@ enum rpmh_regulator_type {
 };
 
 #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
@@ -174,6 +180,28 @@ 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, RPMH_ACTIVE_ONLY_STATE, cmd, 1);
+}
+
+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 ret;
+
+	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);
+
+	return ret;
+}
+
 static int _rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
 				unsigned int selector, bool wait_for_ack)
 {
@@ -215,6 +243,14 @@ 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;
 }
@@ -222,6 +258,18 @@ static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
 static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
 {
 	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+	};
+	int ret;
+
+	if (vreg->enabled < 0) {
+		ret = rpmh_regulator_read_data(vreg, &cmd);
+		if (!ret)
+			vreg->enabled = cmd.data & RPMH_REGULATOR_ENABLE_MASK;
+		else
+			dev_err(vreg->dev, "failed to read ENABLE status ret = %d\n", ret);
+	}
 
 	return vreg->enabled;
 }
@@ -303,6 +351,29 @@ static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
 static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
 {
 	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+	struct tcs_cmd cmd = {
+		.addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+	};
+	int ret, pmic_mode, mode;
+
+	if (vreg->mode > REGULATOR_MODE_INVALID && vreg->mode <= REGULATOR_MODE_STANDBY)
+		return vreg->mode;
+
+	ret = rpmh_regulator_read_data(vreg, &cmd);
+	if (!ret) {
+		pmic_mode = cmd.data & RPMH_REGULATOR_MODE_MASK;
+		if (pmic_mode == 0)
+			return vreg->mode;
+
+		for (mode = 0; mode <= REGULATOR_MODE_STANDBY; mode++) {
+			if (pmic_mode == vreg->hw_data->pmic_mode_map[mode]) {
+				vreg->mode = mode;
+				break;
+			}
+		}
+	} else {
+		dev_err(vreg->dev, "failed to read MODE ret = %d\n", ret);
+	}
 
 	return vreg->mode;
 }

-- 
2.25.1

Re: [PATCH 2/2] regulator: qcom-rpmh: Add support to read regulator settings

[Mark Brown]

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

On Mon, Jun 23, 2025 at 10:13:41PM +0530, Kamal Wadhwa wrote:

> To address this issue, enhance the `get_voltage_sel()`,
> `get_mode()`, and `is_enabled()` callbacks to read the regulator
> settings directly from the RPMH hardware using the `rpmh_read()`

Two things here.  One is that my understanding was that at least some of
the firmwares simply do not provide read functionality - this new code
will turn that into an error if it's the case.  The other is that
there's an expectation that the read operations will return the value
that was configured by the host, we might get confused if that's not the
case.  I'm not sure if there's paths that are currently implemented
that'd have issues, but it's a concern.

For the enable there's a separate status callback that should be
implemented, and you could bootstrap the state.  For the voltage
readback it's a range that's configured so it should be fine to just do
this I think, though I'd need to go double check the code for keeping
multiple supplies tied within a range.

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

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

[Konrad Dybcio]

On 6/23/25 6:43 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.
> 
> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
> ---

[...]

>  	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;
> +	cmd_msgid |= (!msg->is_read) ? CMD_MSGID_WRITE : 0;

if (!msg->is_read)
	cmd_msgid |= CMD_MSGID_WRITE

looks more human-readable

[...]

> +/**
> + * rpmh_read: Read a resource value
> + *
> + * @dev: The device making the request
> + * @state: Must be Active state
> + * @cmd: The payload having address of resource to read
> + * @n: The number of elements in @cmd, must be single command
> + *
> + * 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			- Success
> + * * -Error             - Error code

This isn't valid kerneldoc

> + */
> +int rpmh_read(const struct device *dev, enum rpmh_state state, struct tcs_cmd *cmd, u32 n)
> +{
> +	int ret;
> +	DECLARE_COMPLETION_ONSTACK(compl);
> +	DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);

A reverse-Christmas-tree sorting would be nice here

> +
> +	if (n != 1 || state != RPMH_ACTIVE_ONLY_STATE)

if n must be one, why is it a parameter?

Konrad

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

[Maulik Shah (mkshah)]

On 6/24/2025 8:14 PM, Konrad Dybcio wrote:
> On 6/23/25 6:43 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.
>>
>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>>  	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;
>> +	cmd_msgid |= (!msg->is_read) ? CMD_MSGID_WRITE : 0;
> 
> if (!msg->is_read)
> 	cmd_msgid |= CMD_MSGID_WRITE
> 
> looks more human-readable

I will update in v2.

> 
> [...]
> 
>> +/**
>> + * rpmh_read: Read a resource value
>> + *
>> + * @dev: The device making the request
>> + * @state: Must be Active state
>> + * @cmd: The payload having address of resource to read
>> + * @n: The number of elements in @cmd, must be single command
>> + *
>> + * 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			- Success
>> + * * -Error             - Error code
> 
> This isn't valid kerneldoc

I will update in v2.

> 
>> + */
>> +int rpmh_read(const struct device *dev, enum rpmh_state state, struct tcs_cmd *cmd, u32 n)
>> +{
>> +	int ret;
>> +	DECLARE_COMPLETION_ONSTACK(compl);
>> +	DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
> 
> A reverse-Christmas-tree sorting would be nice here

I will update in v2 per reverse-Christmas-tree sorting.

> 
>> +
>> +	if (n != 1 || state != RPMH_ACTIVE_ONLY_STATE)
> 
> if n must be one, why is it a parameter?

I will Remove n (which is always 1) and also the state (which is also always RPMH_ACTIVE_ONLY_STATE) from the parameters.

Thanks,
Maulik

> 
> Konrad

Re: [PATCH 2/2] regulator: qcom-rpmh: Add support to read regulator settings

[Kamal Wadhwa]

Hi Mark,

On Mon, Jun 23, 2025 at 10:34 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jun 23, 2025 at 10:13:41PM +0530, Kamal Wadhwa wrote:
>
> > To address this issue, enhance the `get_voltage_sel()`,
> > `get_mode()`, and `is_enabled()` callbacks to read the regulator
> > settings directly from the RPMH hardware using the `rpmh_read()`
>
> Two things here.  One is that my understanding was that at least some of
> the firmwares simply do not provide read functionality - this new code
> will turn that into an error if it's the case.  The other is that
> there's an expectation that the read operations will return the value
> that was configured by the host, we might get confused if that's not the
> case.  I'm not sure if there's paths that are currently implemented
> that'd have issues, but it's a concern.

This change should not violate the 2 things you have highlighted.

To elaborate, the regulator status will be read by APPS as ON if the rail was
requested ON by APPS, before reaching kernel stage.  And will *not* be
seen as ON if it’s voted from some other subsystem DSP.

One important note here (about an exception to the above statement),
Internally all rails are ‘assigned' to a subsystem. Even rails shared between 2
or more subsystems are 'internally assigned’ to only one of those subsystems.

Boot loader code initializes the RPMh votes of the 'assigned' subsystem to match
the physical status of each regulator after the power-on sequence completes.
This ensures correct RPMh regulator parameter aggregation when subsystems
issue their own votes.

So, if a rail which is internally assigned to the APPS subsystem, gets
turned ON by
PMIC HW ( power ON sequence), in such cases too,  the rail may be seen
ON from APPS side. This is the exception.

But this exception only applies if default ON rails is ‘internally assigned’ to
APPS subsystem. And will *not* happen if the rail was assigned to some other
subsystem DSP (and was turned ON from PMIC HW). In such a case status
will still be seen as OFF from APPS (as expected), even though they may actually
be ON.


>
> For the enable there's a separate status callback that should be
> implemented, and you could bootstrap the state.  For the voltage
> readback it's a range that's configured so it should be fine to just do
> this I think, though I'd need to go double check the code for keeping
> multiple supplies tied within a range.

Sure, I will bootstrap the state(ON/OFF), and move the read logic to
get_status() op.