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

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.

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.