[ @Jernej, sorry for the private reply earlier. I'll try to address
   everything here. ]

On 26.04.2022 05:05, Samuel Holland wrote:
> On 4/25/22 11:28 AM, Jernej Škrabec wrote:
>> Dne ponedeljek, 25. april 2022 ob 13:01:54 CEST je Harald Geyer 
>> napisal(a):
>>> On 24.04.2022 03:56, Samuel Holland wrote:
>>>> On 4/15/22 11:56 AM, Harald Geyer wrote:
>>>>> Allwinner A64 SoC has separate supplies for PC, PD, PE, PG and 
>>>>> PL.
>>>>>
>>>>> Usually supplies are linked via the 'regulator-name' property of
>>>>> regulator nodes. However when regulators are shared we need to
>>>>> declare the additional links in the pinctrl node.
>>>>>
>>>>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>>>>
>>>> I'm curious if this solved an issue for you, or if this is just 
>>>> for
>>>> accuracy.
>>>> Both of these regulators have the regulator-always-on property, so
>>>> they should have been enabled already.
>>>
>>> You are right, there shouldn't be any change in functionality. It 
>>> is
>>> mostly
>>> for extra correctness. However the pincontrol driver started 
>>> spewing
>>> lot's
>>> of warnings about missing regulator nodes a few versions back. The
>>> visible
>>> effect of this change is to silence those warnings. Also make the 
>>> DTS
>>> more
>>> future proof in case the driver is made even more picky in the 
>>> future.
>>>
>>>> If it's the latter reason, why not add the other
>>>> ports? Regardless:
>>>
>>> PD, PE and PL have dedicated regulators, that can be matched via 
>>> the
>>> 'regulator-name' property. I didn't want to specify the same
>>> information
>>> in two places.
>>
>> "regulator-name" is only a label, while phandle is actual regulator 
>> reference
>> that can be used by the driver.

That is clearly not the whole story, as the driver find's the supply
for the PD bank just fine. And this even isn't an always-on regulator.

See the attached dmesg logs.

>> While DT files reside in Linux kernel source,
>> they are used by other OSes and bootloaders, so you can't really 
>> assume what
>> is good or not just by judging based on Linux behaviour. So please 
>> add PD and
>> PL regulators too.
>
> Yes, agreed, except for VCC-PL, which (as the comments in several 
> devicetrees
> note) will cause a circular dependency when loading drivers.

If this is consensus, then I'm happy to do it that way.

>>> For the PF supply, I couldn't find any connection information in 
>>> the
>>> board schematic. I could have added a dummy regulator. But since 
>>> there
>>> is
>>> only one warning about pf-supply during driver initialization and 
>>> not
>>> the
>>> dozens of warnings I see about PC and PG, I figured, I'd rather not 
>>> add
>>> information of dubious use or qualiy.
>>
>> You mean PE right? There is no PF supply on A64.

I meant PF, but you are right, that this doesn't have a supply on A64
at all. However the driver doesn't seem to know this: It emits a
warning about missing PF supply at startup.

>> Anyway, if it's not on
>> schematic, it can be assumed unconnected and thus you shouldn't 
>> define that
>> property. Messages like "using dummy regulator" are fine in such 
>> cases .
>
> All of the ports without a separate VCC-Px input are powered by
> VCC-IO, which in this case is supplied from DCDC1.

So should I add "vcc-pf-supply = <&reg_dcdc1>;" even though the chip
actually doesn't support a dedicated vcc-pf-supply or should I just
ignore this?

best regards,
Harald


> Regards,
> Samuel
>
>> There is no issue of "dubious quality" if schematic is clear. Also 
>> don't worry
>> about usefulness. DT files are hardware description files. They 
>> should reflect
>> hardware configuration, no matter how useful information seems.
>>
>> FYI, information in this case is useful to the driver. If you check 
>> sunxi
>> pinctrl driver, you can see that port bias is set according to 
>> regulator
>> voltage.
>>
>> Best regards,
>> Jernej
>>
>>>
>>> best regards,
>>> Harald
>>>
>>>
>>>> Reviewed-by: Samuel Holland <samuel@sholland.org>
>>>>
>>>>> ---
>>>>>  arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
>>>>> b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
>>>>> index aff0660b899c..cc316ef2e2d6 100644
>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
>>>>> @@ -197,6 +197,11 @@ &ohci1 {
>>>>>  	status = "okay";
>>>>>  };
>>>>>
>>>>> +&pio {
>>>>> +	vcc-pc-supply = <&reg_dcdc1>;
>>>>> +	vcc-pg-supply = <&reg_aldo2>;
>>>>> +};
>>>>> +
>>>>>  &pwm {
>>>>>  	status = "okay";
>>>>>  };
>>>>>
>>>
>>>
>>
>>