Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage

["Stefan Riedmüller" <s.riedmueller@phytec.de>]

Hi Marco,

On 02.12.19 15:14, Marco Felsch wrote:
> Hi Stefan,
> 
> On 19-12-02 14:55, Stefan Riedmüller wrote:
>> Hi Marco,
>>
>> On 02.12.19 13:42, Marco Felsch wrote:
>>> Hi Stefan,
>>>
>>> On 19-12-02 11:09, Stefan Riedmüller wrote:
>>>> Hi Marco,
>>>>
>>>> your proposed setting is only valid for the LDO enabled case but not for the
>>>> case where the LDO's are in bypass mode. Is that intended? In bypass mode it
>>>> actually needs to be 0.925 V min for ARM and 1.15 V min for SOC.
>>>
>>> The case is that the driver doesn't support the bypass mode currently so
>>> yes it was intended.
>>
>> Ok, I see.
>>
>>>
>>>> Did you experience an issue with the current settings or is this just a
>>>> cosmetic change?
>>>
>>> There is currently no issue because the internally LDO's don't try to
>>> apply such a low voltage value. But I think it isn't a cosmetic change
>>> because this value is wrong. We need to specify the valid voltage range.
>>
>> Please correct me if I'm wrong, but isn't the regulator-min and
>> regulator-max values supposed to reflect the min and max values this
>> regulator can deliver?
> 
> Nope, the constraints are hard coded within the driver e.g. da9062: >
> 8<-----------------------------------------------------------------
> /* Regulator operations */
> 
> /* Current limits array (in uA)
>   * - DA9061_ID_[BUCK1|BUCK3]
>   * - DA9062_ID_[BUCK1|BUCK2|BUCK4]
>   * Entry indexes corresponds to register values.
>   */
> static const unsigned int da9062_buck_a_limits[] = {
> 	500000,  600000,  700000,  800000,  900000, 1000000,
> 	1100000, 1200000,
> 	1300000, 1400000, 1500000, 1600000, 1700000,
> 	1800000, 1900000, 2000000
> };
> 
> /* Current limits array (in uA)
>   * - DA9061_ID_BUCK2
>   * - DA9062_ID_BUCK3
>   * Entry indexes corresponds to register values.
>   */
> static const unsigned int
> da9062_buck_b_limits[] = {
> 	1500000, 1600000, 1700000, 1800000,
> 	1900000, 2000000, 2100000, 2200000,
> 	2300000, 2400000, 2500000,
> 	2600000, 2700000, 2800000,
> 	2900000, 3000000
> };
> 
> 8<-----------------------------------------------------------------
> 

These are the available current limits for the buck regulators. I don't see 
where they correspond to the min/max settable output voltage. Maybe I missed 
something?

The regulator bindings state:
- regulator-min-microvolt: smallest voltage consumers may set 

- regulator-max-microvolt: largest voltage consumers may set

For me that is device depended and not design depended.

What is the scenario you're thinking about which would cause the SOC, as a 
consumer, to request a lower voltage as it needs?

Regards,
Stefan

> So you have to specify the min/max voltage for your design.
> 
> Regards,
>    Marco
> 
>> Maybe your change is better placed in the anatop regulators. Btw they also
>> have a 0.725 V minimum voltage:
>>
>>  From arch/arm/boot/dts/imx6qdl.dtsi:
>>
>>                                  reg_arm: regulator-vddcore {
>>
>>                                          compatible = "fsl,anatop-regulator";
>>                                          regulator-name = "vddarm";
>>
>>                                          regulator-min-microvolt = <725000>;
>>
>>                                          regulator-max-microvolt = <1450000>;
>>                                          regulator-always-on;
>>
>>                                          anatop-reg-offset = <0x140>;
>>
>>                                          anatop-vol-bit-shift = <0>;
>>
>>                                          anatop-vol-bit-width = <5>;
>>
>>                                          anatop-delay-reg-offset = <0x170>;
>>
>>                                          anatop-delay-bit-shift = <24>;
>>
>>                                          anatop-delay-bit-width = <2>;
>>
>>                                          anatop-min-bit-val = <1>;
>>
>>                                          anatop-min-voltage = <725000>;
>>
>>                                          anatop-max-voltage = <1450000>;
>>
>>                                  };
>>
>>
>> Regards,
>> Stefan
>>
>>>
>>> Regards,
>>>     Marco
>>>
>>>> Regards,
>>>> Stefan
>>>>
>>>>
>>>> On 29.11.19 17:48, Marco Felsch wrote:
>>>>> The current set minimum voltage of 730000mV seems to be wrong. I don't
>>>>> know the document which specifies that but the imx6qdl datasheets says
>>>>> that the minimum voltage should be 1.05V for VDD_ARM (LDO enabled, lowest
>>>>> opp) and 1.275V for VDD_SOC (LDO enabled, lowest opp).
>>>>>
>>>>> Fixes: ddec5d1c0047 ("ARM: dts: imx6: Add initial support for phyCORE-i.MX 6 SOM")
>>>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>>>>> ---
>>>>>     arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi | 4 ++--
>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
>>>>> index 6486df3e2942..46d4953c5588 100644
>>>>> --- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
>>>>> +++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
>>>>> @@ -107,14 +107,14 @@
>>>>>     		regulators {
>>>>>     			vdd_arm: buck1 {
>>>>>     				regulator-name = "vdd_arm";
>>>>> -				regulator-min-microvolt = <730000>;
>>>>> +				regulator-min-microvolt = <1050000>;
>>>>>     				regulator-max-microvolt = <1380000>;
>>>>>     				regulator-always-on;
>>>>>     			};
>>>>>     			vdd_soc: buck2 {
>>>>>     				regulator-name = "vdd_soc";
>>>>> -				regulator-min-microvolt = <730000>;
>>>>> +				regulator-min-microvolt = <1275000>;
>>>>>     				regulator-max-microvolt = <1380000>;
>>>>>     				regulator-always-on;
>>>>>     			};
>>>>>
>>>>
>>>
>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel