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.1Hi 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
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
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
© 2016 - 2026 Red Hat, Inc.