| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4w77t2j65453arqgrtqyuv3bmgelgmecvf26yj7idfwqfumizj@q3dhohaiyaes>
Date: Wed, 22 Oct 2025 11:43:44 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Kamal Wadhwa <kamal.wadhwa@....qualcomm.com>
Cc: Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
David Collins <david.collins@....qualcomm.com>
Subject: Re: [PATCH v2 3/4] regulator: qcom-rpmh: Add support to read
regulator settings
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@....qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@....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
Powered by blists - more mailing lists