Re: [linux-sunxi] [PATCH v6 8/9] arm64: allwinner: a64: enable AXP803 regulators for Pine64

[Icenowy Zheng]

ä 2017å7æ18æ GMT+08:00 äå10:58:52, Chen-Yu Tsai <wens@xxxxxxxx> åå:
>On Fri, May 19, 2017 at 4:55 PM, Andre Przywara
><andre.przywara@xxxxxxx> wrote:
>> Hi,
>>
>> On 19/05/17 09:29, Icenowy Zheng wrote:
>>>
>>>
>>> ä 2017å5æ19æ GMT+08:00 äå4:27:21, Andre Przywara
><andre.przywara@xxxxxxx> åå:
>>>> Hi,
>>>>
>>>> On 18/05/17 08:16, Icenowy Zheng wrote:
>>>>> Add support of AXP803 regulators in the Pine64 device tree, in
>order
>>>> to
>>>>> enable many future functionalities, e.g. Wi-Fi.
>>>>>
>>>>> Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
>>>>> ---
>>>>> Changes in v6:
>>>>> - Rebased on next-20170517.
>>>>>
>>>>>  .../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 109
>>>> +++++++++++++++++++++
>>>>>  1 file changed, 109 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
>>>> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
>>>>> index 36001884ed33..40921bacb39c 100644
>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
>>>>> @@ -118,6 +118,115 @@
>>>>>     };
>>>>>  };
>>>>>
>>>>> +#include "axp803.dtsi"
>>>>> +
>>>>> +&reg_aldo1 {
>>>>> +   regulator-min-microvolt = <2800000>;
>>>>> +   regulator-max-microvolt = <2800000>;
>>>>> +   regulator-name = "vcc-csi";
>>>>> +};
>>>>> +
>>>>> +&reg_aldo2 {
>>>>> +   regulator-always-on;
>>>>> +   regulator-min-microvolt = <1800000>;
>>>>> +   regulator-max-microvolt = <3300000>;
>>>>> +   regulator-name = "vcc-pl";
>>>>> +};
>>>>> +
>>>>> +&reg_aldo3 {
>>>>> +   regulator-always-on;
>>>>> +   regulator-min-microvolt = <2700000>;
>>>>> +   regulator-max-microvolt = <3300000>;
>>>>> +   regulator-name = "vcc-pll-avcc";
>>>>> +};
>>
>> The schematic puts this at a fixed 3.0V, so why the range here? Are
>we
>> expected to tune the voltage of the PLLs?
>
>Normally these would describe the operating limits. We did this
>in the past for most boards. I'm fine with fixing it to 3.0V
>though.
>
>>
>>>>> +
>>>>> +&reg_dc1sw {
>>>>> +   regulator-name = "vcc-phy";
>>>>> +};
>>>>> +
>>>>> +&reg_dcdc1 {
>>>>> +   regulator-always-on;
>>>>> +   regulator-min-microvolt = <3300000>;
>>>>> +   regulator-max-microvolt = <3300000>;
>>>>> +   regulator-name = "vcc-3v3";
>>>>> +};
>>>>> +
>>>>> +&reg_dcdc2 {
>>>>> +   regulator-always-on;
>>>>> +   regulator-min-microvolt = <1000000>;
>>>>> +   regulator-max-microvolt = <1300000>;
>>>>> +   regulator-name = "vdd-cpux";
>>>>> +};
>>>>> +
>>>>> +/* DCDC3 is polyphased with DCDC2 */
>>>>> +
>>>>> +&reg_dcdc5 {
>>>>> +   regulator-always-on;
>>>>> +   regulator-min-microvolt = <1500000>;
>>>>> +   regulator-max-microvolt = <1500000>;
>>>>> +   regulator-name = "vcc-dram";
>>>>> +};
>>>>
>>>> I think I mentioned this before, but the Pine64 has DDR3L DRAM,
>which
>>>> is
>>>> specified to run at 1.35V (1.36V with the 20mV granularity of the
>AXP).
>>>> The reset value is even (wrongly?) configured to 1.24V.
>>>>
>>>> So is there any reason you set the voltage to 1.5V? Is that what
>the
>>>> BSP
>>>> does? Or did you see any problems with 1.36V?
>>>
>>> I just set it based on the schematics.
>>
>> I wouldn't trust the schematics too much. They are rather generic,
>see
>> the Ethernet page, for instance, showing *different* PHYs, not just
>the
>> ones used.
>> For the DRAM the Pine64 schematic does not even tell the DRAM chip
>used,
>> the name under the chip is just describing the package.
>>
>>> And 1.35v cannot be accurately achieved by dcdc5 and it's a problem
>whether to use 1.34v or 1.36v ;-)
>>
>> Well, as I wrote above, 1.36V is the voltage to go with. I think 10mV
>> more is well within the tolerance ;-)
>>
>>>>> +&reg_dcdc6 {
>>>>> +   regulator-always-on;
>>>>> +   regulator-min-microvolt = <1100000>;
>>>>> +   regulator-max-microvolt = <1100000>;
>>>>> +   regulator-name = "vdd-sys";
>>>>> +};
>>>>> +
>>>>> +&reg_dldo1 {
>>>>> +   regulator-min-microvolt = <3300000>;
>>>>> +   regulator-max-microvolt = <3300000>;
>>>>> +   regulator-name = "vcc-hdmi";
>>>>> +};
>>>>> +
>>>>> +&reg_dldo2 {
>>>>> +   regulator-min-microvolt = <3300000>;
>>>>> +   regulator-max-microvolt = <3300000>;
>>>>> +   regulator-name = "vcc-mipi";
>>>>> +};
>>>>> +
>>>>> +&reg_dldo3 {
>>>>> +   regulator-min-microvolt = <3300000>;
>>>>> +   regulator-max-microvolt = <3300000>;
>>>>> +   regulator-name = "avdd-csi";
>>>>> +};
>>
>> If you stick to the schematic, this should be 2.8V.
>
>Agreed. But...
>
>>>>> +
>>>>> +&reg_dldo4 {
>>>>> +   regulator-min-microvolt = <3300000>;
>>>>> +   regulator-max-microvolt = <3300000>;
>>>>> +   regulator-name = "vcc-wifi";
>>>>> +};
>>>>> +
>>>>> +&reg_eldo1 {
>>>>> +   regulator-min-microvolt = <1800000>;
>>>>> +   regulator-max-microvolt = <1800000>;
>>>>> +   regulator-name = "cpvdd";
>>>>> +};
>>>>> +
>>>>> +&reg_eldo3 {
>>>>> +   regulator-min-microvolt = <1800000>;
>>>>> +   regulator-max-microvolt = <1800000>;
>>>>> +   regulator-name = "vdd-1v8-csi";
>>>>> +};
>>
>> The schematic lists 1.2V/1.5V/1.8V here, so should we have a range?
>
>This and avdd-csi really depends on the camera module used.
>Maybe this should be left to an overlay.

Agree.

>
>>
>>>>> +
>>>>> +&reg_fldo1 {
>>>>> +   regulator-min-microvolt = <1200000>;
>>>>> +   regulator-max-microvolt = <1200000>;
>>>>> +   regulator-name = "vcc-1v2-hsic";
>>>>> +};
>>>>> +
>>>>> +&reg_fldo2 {
>>>>> +   regulator-always-on;
>>
>> Why do we need to turn this on? Upstream firmware does not use the
>> arisc, so it can stay off.
>> Also in general I think Linux should not tinker with the management
>> processor at all.
>
>I'm not sure, but I think at least one SoC had failed to work without
>this powered on. If that is the case with the A64, then please leave
>a comment saying so. Otherwise let it stay off.

Yes it failed to work, and this seems to be common among SoCs
with CPUs power domain.

>
>ChenYu
>
>> Cheers,
>> Andre.
>>
>>>>> +   regulator-min-microvolt = <1100000>;
>>>>> +   regulator-max-microvolt = <1100000>;
>>>>> +   regulator-name = "vdd-cpus";
>>>>> +};
>>>>> +
>>>>> +&reg_rtc_ldo {
>>>>> +   regulator-name = "vcc-rtc";
>>>>> +};
>>>>> +
>>>>>  /* On Exp and Euler connectors */
>>>>>  &uart0 {
>>>>>     pinctrl-names = "default";
>>>>>