Message-ID: <20251107145936.rtbuonpslvixttr4@hu-kamalw-hyd.qualcomm.com>
Date: Fri, 7 Nov 2025 20:29:36 +0530
From: Kamal Wadhwa <kamal.wadhwa@....qualcomm.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....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 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@....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?

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

Powered by blists - more mailing lists