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

Stefan Riedmüller
2019-12-02T14:30:31Z

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;
>>>>> };
>>>>>
>>>>
>>>
>>