| Message ID | 20230113150310.29709-7-quic_devipriy@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] soc: qcom: smd-rpm: Add IPQ9574 compatible | expand |
On 1/13/2023 9:07 PM, Konrad Dybcio wrote: > > > On 13.01.2023 16:03, 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> >> --- > Or maybe hook it up to the spmi_regulator_common_get_voltage() > from the SPMI regulator driver and read the real voltage instead > of relying on hardcoded values thay may differ between boards? > > Konrad In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM. In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values >> drivers/regulator/qcom_smd-regulator.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c >> index 1eb17d378897..49a36b07397c 100644 >> --- a/drivers/regulator/qcom_smd-regulator.c >> +++ b/drivers/regulator/qcom_smd-regulator.c >> @@ -800,6 +800,7 @@ 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[] = { >> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = { >> }; >> >> static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = { >> - { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" }, >> + { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 }, >> {} >> }; >> >> @@ -1394,6 +1395,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; Best Regards, Devi Priya
On 27.01.2023 17:07, Devi Priya wrote: > > > On 1/13/2023 9:07 PM, Konrad Dybcio wrote: >> >> >> On 13.01.2023 16:03, 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> >>> --- >> Or maybe hook it up to the spmi_regulator_common_get_voltage() >> from the SPMI regulator driver and read the real voltage instead >> of relying on hardcoded values thay may differ between boards? >> >> Konrad > In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM. > In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values Unless something changed, RPM regulator framework is simply a fancy front-end for communicating with the PMIC over SPMI, AFAIK.. Konrad > >>> drivers/regulator/qcom_smd-regulator.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c >>> index 1eb17d378897..49a36b07397c 100644 >>> --- a/drivers/regulator/qcom_smd-regulator.c >>> +++ b/drivers/regulator/qcom_smd-regulator.c >>> @@ -800,6 +800,7 @@ 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[] = { >>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = { >>> }; >>> static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = { >>> - { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" }, >>> + { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 }, >>> {} >>> }; >>> @@ -1394,6 +1395,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; > Best Regards, > Devi Priya
On 31.01.2023 10:28, Devi Priya wrote: > > > On 1/27/2023 9:40 PM, Konrad Dybcio wrote: >> >> >> On 27.01.2023 17:07, Devi Priya wrote: >>> >>> >>> On 1/13/2023 9:07 PM, Konrad Dybcio wrote: >>>> >>>> >>>> On 13.01.2023 16:03, 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> >>>>> --- >>>> Or maybe hook it up to the spmi_regulator_common_get_voltage() >>>> from the SPMI regulator driver and read the real voltage instead >>>> of relying on hardcoded values thay may differ between boards? >>>> >>>> Konrad >>> In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM. >>> In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values >> Unless something changed, RPM regulator framework is simply a >> fancy front-end for communicating with the PMIC over SPMI, AFAIK.. >> >> Konrad > Currently in our driver, the voltage write request will be sent to RPM via GLINK which then writes it to the PMIC over I2C using the below APIs > qcom_rpm_smd_write -> rpmsg_send > In IPQ9574, we do not have SPMI support or the support to readback voltage. Okay, I didn't quite catch that there's *only* an i2c PMIC on this platform.. Looking at the MP5496 datasheet though, reading back the voltage should be possible via simply reading the fields that are used to set it. Konrad > >>> >>>>> drivers/regulator/qcom_smd-regulator.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c >>>>> index 1eb17d378897..49a36b07397c 100644 >>>>> --- a/drivers/regulator/qcom_smd-regulator.c >>>>> +++ b/drivers/regulator/qcom_smd-regulator.c >>>>> @@ -800,6 +800,7 @@ 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[] = { >>>>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = { >>>>> }; >>>>> static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = { >>>>> - { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" }, >>>>> + { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 }, >>>>> {} >>>>> }; >>>>> @@ -1394,6 +1395,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; >>> Best Regards, >>> Devi Priya > Best Regards, > Devi Priya
diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c index 1eb17d378897..49a36b07397c 100644 --- a/drivers/regulator/qcom_smd-regulator.c +++ b/drivers/regulator/qcom_smd-regulator.c @@ -800,6 +800,7 @@ 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[] = { @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = { }; static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = { - { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" }, + { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 }, {} }; @@ -1394,6 +1395,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;