[PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Bryan O'Donoghue]

The relevant documents and the dtsi specify the minimum value at 1.808v not
1.8v.

Prior to this fix we get the following error on boot:
[    1.353540] vrej_l3m_1p8: failed to get the current voltage: -ENOTRECOVERABLE
[    1.353544] qcom-rpmh-regulator 17500000.rsc:regulators-9: ldo3: devm_regulator_register() failed, ret=-131
[    1.353546] qcom-rpmh-regulator: probe of 17500000.rsc:regulators-9 failed with error -131

Fixes: 2544631faa7f ("regulator: qcom-rpmh: add support for pm8010 regulators")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/regulator/qcom-rpmh-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 80e304711345b..767a17fe0d51b 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -757,7 +757,7 @@ static const struct rpmh_vreg_hw_data pmic5_pldo502ln = {
 	.regulator_type = VRM,
 	.ops = &rpmh_regulator_vrm_ops,
 	.voltage_ranges = (struct linear_range[]) {
-		REGULATOR_LINEAR_RANGE(1800000, 0,  2,  200000),
+		REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),
 		REGULATOR_LINEAR_RANGE(2608000, 3,  28, 16000),
 		REGULATOR_LINEAR_RANGE(3104000, 29, 30, 96000),
 		REGULATOR_LINEAR_RANGE(3312000, 31, 31, 0),
-- 
2.43.0

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Abel Vesa]

On 24-02-14 12:16:14, Bryan O'Donoghue wrote:
> The relevant documents and the dtsi specify the minimum value at 1.808v not
> 1.8v.
> 
> Prior to this fix we get the following error on boot:
> [    1.353540] vrej_l3m_1p8: failed to get the current voltage: -ENOTRECOVERABLE
> [    1.353544] qcom-rpmh-regulator 17500000.rsc:regulators-9: ldo3: devm_regulator_register() failed, ret=-131
> [    1.353546] qcom-rpmh-regulator: probe of 17500000.rsc:regulators-9 failed with error -131
> 
> Fixes: 2544631faa7f ("regulator: qcom-rpmh: add support for pm8010 regulators")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
>  drivers/regulator/qcom-rpmh-regulator.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 80e304711345b..767a17fe0d51b 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -757,7 +757,7 @@ static const struct rpmh_vreg_hw_data pmic5_pldo502ln = {
>  	.regulator_type = VRM,
>  	.ops = &rpmh_regulator_vrm_ops,
>  	.voltage_ranges = (struct linear_range[]) {
> -		REGULATOR_LINEAR_RANGE(1800000, 0,  2,  200000),
> +		REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),
>  		REGULATOR_LINEAR_RANGE(2608000, 3,  28, 16000),
>  		REGULATOR_LINEAR_RANGE(3104000, 29, 30, 96000),
>  		REGULATOR_LINEAR_RANGE(3312000, 31, 31, 0),
> -- 
> 2.43.0
> 
> 

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On Wed, Feb 14, 2024 at 12:16:14PM +0000, Bryan O'Donoghue wrote:

>  	.voltage_ranges = (struct linear_range[]) {
> -		REGULATOR_LINEAR_RANGE(1800000, 0,  2,  200000),
> +		REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),

This will also offset all other voltages that get set, is that expected
and desired?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Bryan O'Donoghue]

On 14/02/2024 13:25, Mark Brown wrote:
> On Wed, Feb 14, 2024 at 12:16:14PM +0000, Bryan O'Donoghue wrote:
> 
>>   	.voltage_ranges = (struct linear_range[]) {
>> -		REGULATOR_LINEAR_RANGE(1800000, 0,  2,  200000),
>> +		REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),
> 
> This will also offset all other voltages that get set, is that expected
> and desired?

Yep, looks typo in the original submission.

ldo3, ldo4 and ldo6 should all be 1.808.

---
bod

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On Wed, Feb 14, 2024 at 02:07:13PM +0000, Bryan O'Donoghue wrote:
> On 14/02/2024 13:25, Mark Brown wrote:
> > On Wed, Feb 14, 2024 at 12:16:14PM +0000, Bryan O'Donoghue wrote:

> > >   	.voltage_ranges = (struct linear_range[]) {
> > > -		REGULATOR_LINEAR_RANGE(1800000, 0,  2,  200000),
> > > +		REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),

> > This will also offset all other voltages that get set, is that expected
> > and desired?

> Yep, looks typo in the original submission.

> ldo3, ldo4 and ldo6 should all be 1.808.

Not just that but also note that every voltage step in the range will
have the 8mV offset added.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Bryan O'Donoghue]

On 14/02/2024 14:13, Mark Brown wrote:
> On Wed, Feb 14, 2024 at 02:07:13PM +0000, Bryan O'Donoghue wrote:
>> On 14/02/2024 13:25, Mark Brown wrote:
>>> On Wed, Feb 14, 2024 at 12:16:14PM +0000, Bryan O'Donoghue wrote:
> 
>>>>    	.voltage_ranges = (struct linear_range[]) {
>>>> -		REGULATOR_LINEAR_RANGE(1800000, 0,  2,  200000),
>>>> +		REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),
> 
>>> This will also offset all other voltages that get set, is that expected
>>> and desired?
> 
>> Yep, looks typo in the original submission.
> 
>> ldo3, ldo4 and ldo6 should all be 1.808.
> 
> Not just that but also note that every voltage step in the range will
> have the 8mV offset added.

The documents I have just show sensors attached to ldo3, ldo4 and ldo6 
fixed at 1.808.

I don't think there's any better or different information than a 
+200000uV increment TBH.

---
bod

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Wed, Feb 14, 2024 at 02:44:56PM +0000, Bryan O'Donoghue wrote:
> On 14/02/2024 14:13, Mark Brown wrote:

> > Not just that but also note that every voltage step in the range will
> > have the 8mV offset added.

> The documents I have just show sensors attached to ldo3, ldo4 and ldo6 fixed
> at 1.808.

> I don't think there's any better or different information than a +200000uV
> increment TBH.

This seems like a very surprising and unusual hardware design, the
1.808V voltage is already unusual.  Note that this may break systems
that are trying to set a range of say 1.8-2.0V if they actually need to
set 2V.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Bryan O'Donoghue]

On 14/02/2024 14:52, Mark Brown wrote:
> On Wed, Feb 14, 2024 at 02:44:56PM +0000, Bryan O'Donoghue wrote:
>> On 14/02/2024 14:13, Mark Brown wrote:
> 
>>> Not just that but also note that every voltage step in the range will
>>> have the 8mV offset added.
> 
>> The documents I have just show sensors attached to ldo3, ldo4 and ldo6 fixed
>> at 1.808.
> 
>> I don't think there's any better or different information than a +200000uV
>> increment TBH.
> 
> This seems like a very surprising and unusual hardware design, the
> 1.808V voltage is already unusual.  Note that this may break systems
> that are trying to set a range of say 1.8-2.0V if they actually need to
> set 2V.

Hmm. I'm sure the rail value should be 1.808 its all over the 
documentation for example when we get to index 3 we hit 2608000

REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),
1808000 0
2008000 1
2208000 2
2408000 x
REGULATOR_LINEAR_RANGE(2608000, 3,  28, 16000),

And there are other rails @ 1v8 if 1v8

The one thing I can't easily verify is index 0 = 1808000 and not say 
1800000 or indeed that the increment is 200000 and not say 8000.

I'll see if I can ask around with the hw people and get a more complete 
answer.

Similarly now that you've gotten me digging into this problem, it's not 
clear to me why this regulator isn't just a linear regulator with an 8mv 
increment over a range of indexes.

At least the documentation I'm looking at doesn't elucidate.

I'll dig some more.

---
bod

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Bryan O'Donoghue]

On 14/02/2024 22:47, Bryan O'Donoghue wrote:
> And there are other rails @ 1v8 if 1v8

[sic] If 1v8 exact matters to you

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Fenglin Wu]

On 2024/2/15 6:47, Bryan O'Donoghue wrote:
> On 14/02/2024 14:52, Mark Brown wrote:
>> On Wed, Feb 14, 2024 at 02:44:56PM +0000, Bryan O'Donoghue wrote:
>>> On 14/02/2024 14:13, Mark Brown wrote:
>>
>>>> Not just that but also note that every voltage step in the range will
>>>> have the 8mV offset added.
>>
>>> The documents I have just show sensors attached to ldo3, ldo4 and 
>>> ldo6 fixed
>>> at 1.808.
>>
>>> I don't think there's any better or different information than a 
>>> +200000uV
>>> increment TBH.
>>
>> This seems like a very surprising and unusual hardware design, the
>> 1.808V voltage is already unusual.  Note that this may break systems
>> that are trying to set a range of say 1.8-2.0V if they actually need to
>> set 2V.
> 
> Hmm. I'm sure the rail value should be 1.808 its all over the 
> documentation for example when we get to index 3 we hit 2608000
> 
> REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),
> 1808000 0
> 2008000 1
> 2208000 2
> 2408000 x
> REGULATOR_LINEAR_RANGE(2608000, 3,  28, 16000),
> 
> And there are other rails @ 1v8 if 1v8
> 
> The one thing I can't easily verify is index 0 = 1808000 and not say 
> 1800000 or indeed that the increment is 200000 and not say 8000.
> 
> I'll see if I can ask around with the hw people and get a more complete 
> answer.
> 
> Similarly now that you've gotten me digging into this problem, it's not 
> clear to me why this regulator isn't just a linear regulator with an 8mv 
> increment over a range of indexes.
> 
> At least the documentation I'm looking at doesn't elucidate.
> 
> I'll dig some more.
Please see the voltage steps for LDO3/4/6 described in the PM8010 TDOS 
document which is the most authoritative that we used internally for 
PMIC driver development:

Index	Vset (mV)
0	1800
1	2000
2	2200
3	2608
4	2624
5	2640
6	2656
7	2672
8	2688
9	2704
10	2720
11	2736
12	2752
13	2768
14	2784
15	2800
16	2816
17	2832
18	2848
19	2864
20	2880
21	2896
22	2912
23	2928
24	2944
25	2960
26	2976
27	2992
28	3008
29	3104
30	3200
31	3312

And I do see from the document change history that step 0 was changed 
from 1808mV and step 2 was changed from 2512mV, I don't know the reason 
of the change though.

Fenglin

> 
> ---
> bod
> 

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Bryan O'Donoghue]

On 19/02/2024 3:06 a.m., Fenglin Wu wrote:
> 
> 
> On 2024/2/15 6:47, Bryan O'Donoghue wrote:
>> On 14/02/2024 14:52, Mark Brown wrote:
>>> On Wed, Feb 14, 2024 at 02:44:56PM +0000, Bryan O'Donoghue wrote:
>>>> On 14/02/2024 14:13, Mark Brown wrote:
>>>
>>>>> Not just that but also note that every voltage step in the range will
>>>>> have the 8mV offset added.
>>>
>>>> The documents I have just show sensors attached to ldo3, ldo4 and 
>>>> ldo6 fixed
>>>> at 1.808.
>>>
>>>> I don't think there's any better or different information than a 
>>>> +200000uV
>>>> increment TBH.
>>>
>>> This seems like a very surprising and unusual hardware design, the
>>> 1.808V voltage is already unusual.  Note that this may break systems
>>> that are trying to set a range of say 1.8-2.0V if they actually need to
>>> set 2V.
>>
>> Hmm. I'm sure the rail value should be 1.808 its all over the 
>> documentation for example when we get to index 3 we hit 2608000
>>
>> REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),
>> 1808000 0
>> 2008000 1
>> 2208000 2
>> 2408000 x
>> REGULATOR_LINEAR_RANGE(2608000, 3,  28, 16000),
>>
>> And there are other rails @ 1v8 if 1v8
>>
>> The one thing I can't easily verify is index 0 = 1808000 and not say 
>> 1800000 or indeed that the increment is 200000 and not say 8000.
>>
>> I'll see if I can ask around with the hw people and get a more 
>> complete answer.
>>
>> Similarly now that you've gotten me digging into this problem, it's 
>> not clear to me why this regulator isn't just a linear regulator with 
>> an 8mv increment over a range of indexes.
>>
>> At least the documentation I'm looking at doesn't elucidate.
>>
>> I'll dig some more.
> Please see the voltage steps for LDO3/4/6 described in the PM8010 TDOS 
> document which is the most authoritative that we used internally for 
> PMIC driver development:

I will look - however

1. The powertree internal docs for xe801000 show 1.808 rails derived
    from 1.856 rails for camera sensors

2. Publicly available with registration : 80-185821-1
 
https://docs.qualcomm.com/bundle/80-18582-1/resource/80-18582-1_REV_AV_PM8010_Data_Sheet.pdf

    Table 3-7 Linear/low-voltage regulator summary

    Specified programmable range (V)
    ldo3, ldo4, ldo6 = 1.808 to 3.312

3. The pmic ranges I'm looking at on the internal
    show increases of 8000 mv linearly

> And I do see from the document change history that step 0 was changed 
> from 1808mV and step 2 was changed from 2512mV, I don't know the reason 
> of the change though.

Hrmm...

OK, that's enough to investigate further.

---
bod

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Fenglin Wu]

On 2/20/2024 4:20 PM, Bryan O'Donoghue wrote:
> On 19/02/2024 3:06 a.m., Fenglin Wu wrote:
>>
>>
>> On 2024/2/15 6:47, Bryan O'Donoghue wrote:
>>> On 14/02/2024 14:52, Mark Brown wrote:
>>>> On Wed, Feb 14, 2024 at 02:44:56PM +0000, Bryan O'Donoghue wrote:
>>>>> On 14/02/2024 14:13, Mark Brown wrote:
>>>>
>>>>>> Not just that but also note that every voltage step in the range will
>>>>>> have the 8mV offset added.
>>>>
>>>>> The documents I have just show sensors attached to ldo3, ldo4 and 
>>>>> ldo6 fixed
>>>>> at 1.808.
>>>>
>>>>> I don't think there's any better or different information than a 
>>>>> +200000uV
>>>>> increment TBH.
>>>>
>>>> This seems like a very surprising and unusual hardware design, the
>>>> 1.808V voltage is already unusual.  Note that this may break systems
>>>> that are trying to set a range of say 1.8-2.0V if they actually need to
>>>> set 2V.
>>>
>>> Hmm. I'm sure the rail value should be 1.808 its all over the 
>>> documentation for example when we get to index 3 we hit 2608000
>>>
>>> REGULATOR_LINEAR_RANGE(1808000, 0,  2,  200000),
>>> 1808000 0
>>> 2008000 1
>>> 2208000 2
>>> 2408000 x
>>> REGULATOR_LINEAR_RANGE(2608000, 3,  28, 16000),
>>>
>>> And there are other rails @ 1v8 if 1v8
>>>
>>> The one thing I can't easily verify is index 0 = 1808000 and not say 
>>> 1800000 or indeed that the increment is 200000 and not say 8000.
>>>
>>> I'll see if I can ask around with the hw people and get a more 
>>> complete answer.
>>>
>>> Similarly now that you've gotten me digging into this problem, it's 
>>> not clear to me why this regulator isn't just a linear regulator with 
>>> an 8mv increment over a range of indexes.
>>>
>>> At least the documentation I'm looking at doesn't elucidate.
>>>
>>> I'll dig some more.
>> Please see the voltage steps for LDO3/4/6 described in the PM8010 TDOS 
>> document which is the most authoritative that we used internally for 
>> PMIC driver development:
> 
> I will look - however
> 
> 1. The powertree internal docs for xe801000 show 1.808 rails derived
>     from 1.856 rails for camera sensors
> 
> 2. Publicly available with registration : 80-185821-1
> 
> https://docs.qualcomm.com/bundle/80-18582-1/resource/80-18582-1_REV_AV_PM8010_Data_Sheet.pdf
> 
>     Table 3-7 Linear/low-voltage regulator summary
> 
>     Specified programmable range (V)
>     ldo3, ldo4, ldo6 = 1.808 to 3.312
> 
> 3. The pmic ranges I'm looking at on the internal
>     show increases of 8000 mv linearly
> 
>> And I do see from the document change history that step 0 was changed 
>> from 1808mV and step 2 was changed from 2512mV, I don't know the 
>> reason of the change though.
> 
> Hrmm...
> 
> OK, that's enough to investigate further.
> 

Got feedback from the chip designer: "1.808V is the typical digital Vset 
logic output – always round up to the integer multiples of 8mV.
However, 1.8V is a more commonly used output. So we made analog only 
change, move the tap point of the reference generator to 1.8V when it is 
programmed to 1.808V.
If user program it to 1.8V, digital logic will round it up to 1.808V, 
send it to analog, then analog will map it to 1.8V. So the end result is 
the same regardless customer program it to 1.8V or 1.808V from PMIC 
register point of view. "

So, programming it to either 1.8V or 1.808V, the HW will output 1.8V. I 
understand there is a problem for x1e801000 because its AOP side limits 
the voltage range to [1.808V, 1.808V] for LDO3/4/6 power rails, it won't 
work if linux side updates to use 1.8V. Actually the same issue applies 
to SM8550 and SM8650 if you simply update the voltage level to 1.808V, 
because their AOP side limits the voltage ranges for some of these LDOs 
to [1.8V, 1.8V].

One possible fix is just adding 1.808v as another level for these LDOs.

Fenglin

> ---
> bod
> 

Re: [PATCH] regulator: qcom-rpmh: Fix pm8010 pmic5_pldo502ln minimum voltage

[Bryan O'Donoghue]

On 21/02/2024 9:11 a.m., Fenglin Wu wrote:
> 
> So, programming it to either 1.8V or 1.808V, the HW will output 1.8V. I 
> understand there is a problem for x1e801000 because its AOP side limits 
> the voltage range to [1.808V, 1.808V] for LDO3/4/6 power rails, it won't 
> work if linux side updates to use 1.8V. Actually the same issue applies 
> to SM8550 and SM8650 if you simply update the voltage level to 1.808V, 
> because their AOP side limits the voltage ranges for some of these LDOs 
> to [1.8V, 1.8V].

Hmm.

We have no use case for 1.808 to my knowledge - the sensors take 
voltages in the range 1.7 to 1.9 volts.

TBH I think it should work just fine at 1.8v bang on.

I'll test this theory on the reference hardware and repost either a 
patch or further discussion.

But I think we should probably just leave what you've upstreamed and 
request 1.8 v in the dts honestly.

Thanks

---
bod