by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

On 17.02.2023 15:20, Devi Priya wrote:
> Kernel does not know the initial voltage set by the bootloaders.
> During regulator registration, the voltage variable is just declared
> and it is zero. Based on that, the regulator framework considers current
> the voltage as zero and tries to bring up each regulator to minimum
> the supported voltage.
>
> This introduces a dip in the voltage during kernel boot and gets
> stabilized once the voltage scaling comes into picture.
>
> To avoid the voltage dip, adding support to define the
> bootup voltage set by the boodloaders and based on it, regulator
> framework understands that proper voltage is already set
>
> Co-developed-by: Praveenkumar I <[email protected]>
> Signed-off-by: Praveenkumar I <[email protected]>
> Signed-off-by: Devi Priya <[email protected]>
> ---
Thinking about it again, this seems like something that could be
generalized and introduced into regulator core.. Hardcoding this
will not end well.. Not to mention it'll affect all mp5496-using
boards that are already upstream.

WDYT about regulator-init-microvolts Mark?

Konrad
> Changes in V2:
> - Added the bootup voltages to s2 and l2 regulators
>
> drivers/regulator/qcom_smd-regulator.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> index a40e66cea7e7..5f9fe6b9d368 100644
> --- a/drivers/regulator/qcom_smd-regulator.c
> +++ b/drivers/regulator/qcom_smd-regulator.c
> @@ -800,12 +800,13 @@ struct rpm_regulator_data {
> u32 id;
> const struct regulator_desc *desc;
> const char *supply;
> + int boot_uV; /* To store the bootup voltage set by bootloaders */
> };
>
> static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
> - { "s1", QCOM_SMD_RPM_SMPA, 1, &mp5496_smpa1, "s1" },
> - { "s2", QCOM_SMD_RPM_SMPA, 2, &mp5496_smpa2, "s2" },
> - { "l2", QCOM_SMD_RPM_LDOA, 2, &mp5496_ldoa2, "l2" },
> + { "s1", QCOM_SMD_RPM_SMPA, 1, &mp5496_smpa1, "s1", 875000 },
> + { "s2", QCOM_SMD_RPM_SMPA, 2, &mp5496_smpa2, "s2", 875000 },
> + { "l2", QCOM_SMD_RPM_LDOA, 2, &mp5496_ldoa2, "l2", 2950000 },
> {}
> };
>
> @@ -1388,6 +1389,9 @@ static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev
> vreg->type = rpm_data->type;
> vreg->id = rpm_data->id;
>
> + if (rpm_data->boot_uV)
> + vreg->uV = rpm_data->boot_uV;
> +
> memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc));
> vreg->desc.name = rpm_data->name;
> vreg->desc.supply_name = rpm_data->supply;

2023-02-22 23:01:33

On Mon, Apr 03, 2023 at 07:53:48PM +0200, Konrad Dybcio wrote:
> On 3.04.2023 16:07, Devi Priya wrote:

> > But, when the regulator driver comes up, it tries to bring up the
> > regulators to the minimum supported voltage provided with the
> > regulator-min-microvolt property in the DT.

> Right, that exists..

> Mark, do you think it should be updated such that the requests are
> aggregated before assuming min_uV is "just fine"?

We can't tell if any consumers are ever going to appear, and the
regulator having a voltage outside of the constraints is an urgent
problem we need to fix quickly. Since we try to bring the voltage to
the nearest end of the constraint the driver could always change the
bogus voltage it reports to one that is excessively high, this would
mean the core will try to bring the voltage down to the maximum rather
than up to the minimum. The driver could also look at the constraints
when guessing at the hardware configuration rather than claiming an out
of spec voltage, this would mean we wouldn't need to correct anything.

Attachments:

(No filename) (1.06 kB)
signature.asc (499.00 B)
Download all attachments

2023-04-03 18:28:14

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

On 3.04.2023 20:14, Mark Brown wrote:
> On Mon, Apr 03, 2023 at 07:53:48PM +0200, Konrad Dybcio wrote:
>> On 3.04.2023 16:07, Devi Priya wrote:
>
>>> But, when the regulator driver comes up, it tries to bring up the
>>> regulators to the minimum supported voltage provided with the
>>> regulator-min-microvolt property in the DT.
>
>> Right, that exists..
>
>> Mark, do you think it should be updated such that the requests are
>> aggregated before assuming min_uV is "just fine"?
>
> We can't tell if any consumers are ever going to appear, and the
> regulator having a voltage outside of the constraints is an urgent
> problem we need to fix quickly. Since we try to bring the voltage to
> the nearest end of the constraint the driver could always change the
> bogus voltage it reports to one that is excessively high, this would
> mean the core will try to bring the voltage down to the maximum rather
> than up to the minimum. The driver could also look at the constraints
> when guessing at the hardware configuration rather than claiming an out
> of spec voltage, this would mean we wouldn't need to correct anything.
Hm, all of what you said sounds like a valid concern.. And then we
probably shouldn't shoot up to max by default, as going too low is
not going to cause as much potential irreversible damage as going
too high.. Especially with programmer error..

Too bad Qualcomm's firmware architecture doesn't allow for reading
back the voltage..

Konrad

2023-04-03 18:30:32

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

On Mon, Apr 03, 2023 at 08:21:25PM +0200, Konrad Dybcio wrote:
> On 3.04.2023 20:14, Mark Brown wrote:

> > than up to the minimum. The driver could also look at the constraints
> > when guessing at the hardware configuration rather than claiming an out
> > of spec voltage, this would mean we wouldn't need to correct anything.

> Hm, all of what you said sounds like a valid concern.. And then we
> probably shouldn't shoot up to max by default, as going too low is
> not going to cause as much potential irreversible damage as going
> too high.. Especially with programmer error..

It sounds like the driver should just be reporting a value which is at
least within the constraints.

> Too bad Qualcomm's firmware architecture doesn't allow for reading
> back the voltage..

Right, their interfaces here have some serious drawbacks.

Attachments:

(No filename) (856.00 B)
signature.asc (499.00 B)
Download all attachments