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 <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 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;
-- 
2.17.1

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 <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---
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;

On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:

> 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?

The overwhelming majority of devices that have variable voltages
support readback, these Qualcomm firmware devices are pretty much
unique in this regard.  We don't want a general property to set a
specific voltage since normally we should be using the
constraints and don't normally need to adjust things immediately
since we can tell what the current voltage is. 

This is pretty much just going to be a device specific bodge,
ideally something that does know what the voltage is would be
able to tell us at runtime but if that's not possible then
there's no good options.  If the initial voltage might vary based
on board then a device specific DT property might be less
terrible, if it's determined by the regulator the current code
seems fine.  Or just leave the current behavour, if the
constraints are accurate then hopefully a temporary dip in
voltage is just inelegant rather than an issue.  Indeed the
current behaviour might well save power if you've got a voltage
range configured and nothing actually ever gets round to setting
the voltage (which is depressingly common, people seem keen on
setting voltage ranges even when the voltage is never varied in
practice).

On 2/23/2023 4:31 AM, Mark Brown wrote:
> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
> 
>> 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?
> 
> The overwhelming majority of devices that have variable voltages
> support readback, these Qualcomm firmware devices are pretty much
> unique in this regard.  We don't want a general property to set a
> specific voltage since normally we should be using the
> constraints and don't normally need to adjust things immediately
> since we can tell what the current voltage is.
> 
> This is pretty much just going to be a device specific bodge,
> ideally something that does know what the voltage is would be
> able to tell us at runtime but if that's not possible then
> there's no good options.  If the initial voltage might vary based
> on board then a device specific DT property might be less
> terrible, if it's determined by the regulator the current code
> seems fine.  Or just leave the current behavour, if the
> constraints are accurate then hopefully a temporary dip in
> voltage is just inelegant rather than an issue.  Indeed the
> current behaviour might well save power if you've got a voltage
> range configured and nothing actually ever gets round to setting
> the voltage (which is depressingly common, people seem keen on
> setting voltage ranges even when the voltage is never varied in
> practice).

Hi Mark, The initial bootup voltage is actually blown into the OTP 
register of the PMIC and it remains the same across boards for IPQ9574 
SoC. Initially the SoC runs at 800MHz with a voltage of 875mV set by the 
bootloaders. As kernel does not know the initial voltage, during 
regulator registration the framework considers the current voltage to be 
zero and tries to bring up the regulator to minimum supported voltage of 
600mV. This causes the dip which might be of concern in SS parts where 
the voltage might be insufficient leading to silent reboots.

Best Regards,
Devi Priya

On 3.03.2023 14:21, Devi Priya wrote:
> 
> 
> On 2/23/2023 4:31 AM, Mark Brown wrote:
>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>
>>> 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?
>>
>> The overwhelming majority of devices that have variable voltages
>> support readback, these Qualcomm firmware devices are pretty much
>> unique in this regard.  We don't want a general property to set a
>> specific voltage since normally we should be using the
>> constraints and don't normally need to adjust things immediately
>> since we can tell what the current voltage is.
>>
>> This is pretty much just going to be a device specific bodge,
>> ideally something that does know what the voltage is would be
>> able to tell us at runtime but if that's not possible then
>> there's no good options.  If the initial voltage might vary based
>> on board then a device specific DT property might be less
>> terrible, if it's determined by the regulator the current code
>> seems fine.  Or just leave the current behavour, if the
>> constraints are accurate then hopefully a temporary dip in
>> voltage is just inelegant rather than an issue.  Indeed the
>> current behaviour might well save power if you've got a voltage
>> range configured and nothing actually ever gets round to setting
>> the voltage (which is depressingly common, people seem keen on
>> setting voltage ranges even when the voltage is never varied in
>> practice).
> 
> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
But what about IPQ6018 which also uses MP5496? That's also gonna
set the voltage on there, it may be too high/low..

 Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
That's an SoC-specific thing, the same regulator can be used with
many different ones. We can't just assume it'll always be like this.
I see the problem, but I believe this is not the correct solution.

Konrad
> 
> Best Regards,
> Devi Priya