Re: [PATCH v3 3/4] regulator: qcom-rpmh: readback voltage/bypass/mode/status set during bootup

[Kamal Wadhwa]

On Tue, Apr 07, 2026 at 01:16:41PM +0200, Konrad Dybcio wrote:
> On 4/7/26 6:43 AM, Kamal Wadhwa wrote:
> > Currently, during regulator registration, regulator framework sends an
> > unnecessary `min-microvolts` request for the rpmh-regulator device. This
> > happens because in current design, we do not have a way to readback the
> > voltage settings that was set during the bootloader stage.
> > 
> > Fix this by using the rpmh_read() API to read the regulator voltage
> > settings done during boot and make it available to regulator framework
> > from the very first read after the bootup.
> > 
> > Also use this API to read the status/mode/bypass settings as well. This
> > will provide the regulator framework a sense of the initial settings
> > done by bootloader and thus preventing any redundents writes for any
> > setting post bootup incase the same setting was already applied during
> > bootup.
> > 
> > Signed-off-by: Kamal Wadhwa <kamal.wadhwa@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/regulator/qcom-rpmh-regulator.c | 178 ++++++++++++++++++++++++++++++++
> >  1 file changed, 178 insertions(+)
> > 
> > diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> > index 85fbf10f74bb3393071bc65681356312f27b7527..8e1c576b718b595bbbff7f5fa76de84d4e90f3bb 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
> 
> GENMASK(12, 0)

will fix this in next reversion

> 
> > +
> >  #define RPMH_REGULATOR_REG_ENABLE		0x4
> > +#define RPMH_REGULATOR_ENABLE_MASK		0x1
> 
> BIT(0)

will fix this in next reversion

> 
> > +
> >  #define RPMH_REGULATOR_REG_VRM_MODE		0x8
> > +#define RPMH_REGULATOR_MODE_MASK		0x7
> 
> GENMASK(2, 0)

will fix this in next reversion

> 
> [...]
> 
> >  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) {
> 
> This assumes that the voltage selector can't change due to an
> intervention from say ADSP - is that true, as far as the _read() lets
> us know?
> 
> But I suppose we can't know about __every__ change since they could
> happen without a notification to HLOS and it's probably much saner to
> stick to what Linux believes is set on the hw..
> 
> [...]

We can only read the voltage_selector for APPS, we cannot know
what ADSP or other subsystem may have set on a shared rail. RPMH
firmware guarantees that voltage will be greater-or-equal to the
voltage requested from the APPS subsystem. We cannot know the actual
voltage set on the regulator using the rpmh_read() API.

eg - we may read a regulator as OFF/0V while the actual state of that
regulator may have been ON, due to a request from some other subsystem.

As we are currently relying on the cached value, (after this change)
we can still continue to do same, but only right after boot (when selector
is initialized to -ENOTRECOVERABLE) we are using the read API to know
what was set from APPS during bootloader stage.

> 
> > +	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;
> 
> The 'else' case is unreachable, since you return early if !sts beforehand

Agree, will set to ON always, as we already checked for OFF.

> 
> 
> Otherwise LGTM
Thank you for quick review.

> 
> Konrad

Regards,
Kamal