diff mbox series

[6/6] regulator: qcom_smd: Add support to define the bootup voltage

Message ID 20230113150310.29709-7-quic_devipriy@quicinc.com
State New
Headers
Series Add regulator support for IPQ9574 SoC |

Commit Message

Devi Priya Jan. 13, 2023, 3:03 p.m. UTC 
  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>
---
 drivers/regulator/qcom_smd-regulator.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Konrad Dybcio Jan. 13, 2023, 3:37 p.m. UTC | #1 
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
>  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;
Devi Priya Jan. 27, 2023, 4:07 p.m. UTC | #2 
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
Konrad Dybcio Jan. 27, 2023, 4:10 p.m. UTC | #3 
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
Devi Priya Jan. 31, 2023, 9:28 a.m. UTC | #4 
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.

>>
>>>>    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
Dmitry Baryshkov Jan. 31, 2023, 9:37 a.m. UTC | #5 
On 13/01/2023 17: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>
> ---
>   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 },

I think this is a peculiarity of the particular board that than a 
property of the PMIC. Please describe this in the board or SoC DTS if 
the value can not be read using the software .

>   	{}
>   };
>   
> @@ -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;
Konrad Dybcio Jan. 31, 2023, 12:44 p.m. UTC | #6 
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
Devi Priya Feb. 2, 2023, 11:09 a.m. UTC | #7 
On 1/31/2023 6:14 PM, Konrad Dybcio wrote:
> 
> 
> 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
The CPR regulator operates in closed loop mode and the RPM can 
independently update the PMIC voltage.
So, Performing an i2c read to the PMIC would introduce conflicts when 
RPM uses the i2c for any of the voltage write or read operations.
>>
>>>>
>>>>>>     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
Devi Priya Feb. 2, 2023, 11:09 a.m. UTC | #8 
On 1/31/2023 3:07 PM, Dmitry Baryshkov wrote:
> On 13/01/2023 17: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>
>> ---
>>   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 },
> 
> I think this is a peculiarity of the particular board that than a 
> property of the PMIC. Please describe this in the board or SoC DTS if 
> the value can not be read using the software .
> 
The bootup voltage is actually blown into the OTP register of the PMIC 
and so, it remains the same across boards for IPQ9574 SoC
>>       {}
>>   };
>> @@ -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
Konrad Dybcio Feb. 2, 2023, 11:43 a.m. UTC | #9 
On 2.02.2023 12:09, Devi Priya wrote:
> 
> 
> On 1/31/2023 6:14 PM, Konrad Dybcio wrote:
>>
>>
>> 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
> The CPR regulator operates in closed loop mode and the RPM can independently update the PMIC voltage.
> So, Performing an i2c read to the PMIC would introduce conflicts when RPM uses the i2c for any of the voltage write or read operations.
So.. are we even going to set voltage from Linux at all, for example
for DCVS? If not, maybe we can simply not register the regulator and
let the non-APSS parts handle it themselves?

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
Devi Priya Feb. 6, 2023, 1:48 p.m. UTC | #10 
On 2/2/2023 5:13 PM, Konrad Dybcio wrote:
> 
> 
> On 2.02.2023 12:09, Devi Priya wrote:
>>
>>
>> On 1/31/2023 6:14 PM, Konrad Dybcio wrote:
>>>
>>>
>>> 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
>> The CPR regulator operates in closed loop mode and the RPM can independently update the PMIC voltage.
>> So, Performing an i2c read to the PMIC would introduce conflicts when RPM uses the i2c for any of the voltage write or read operations.
> So.. are we even going to set voltage from Linux at all, for example
> for DCVS? If not, maybe we can simply not register the regulator and
> let the non-APSS parts handle it themselves?
> 
In IPQ9574, PMIC basically controls three rails. In that, RPM has 
control over two rails (MX and CX) & APSS has control over the APC rail. 
RPM controls the MX and CX rails independently. For APC rail, APSS sends 
the voltage request to RPM via GLINK and RPM takes care of accessing the 
PMIC via I2C for APSS voltage requests & its own requests. This approach 
helps us to avoid arbitration. In this case, if we directly use the I2C 
to read the PMIC we would end up having issues, if RPM is accessing the 
PMIC.
> 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 mbox series

Patch

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;