Re: [Letux-kernel] [RFC PATCH 0/3] Enable 1GHz support on omap36xx

[H. Nikolaus Schaller]

Hi,

> Am 10.09.2019 um 20:30 schrieb Adam Ford <aford173@xxxxxxxxx>:
> 
> On Tue, Sep 10, 2019 at 11:59 AM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>> 
>> Hi Adam,
>> 
>>> Am 09.09.2019 um 21:13 schrieb Adam Ford <aford173@xxxxxxxxx>:
>>> 
>>> On Mon, Sep 9, 2019 at 1:11 PM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>>>> 
>>>> Ok, we have to check if the ti,abb-v2 "LDO" driver
>>>> drivers/regulator/ti-abb-regulator.c
>>>> can handle that with a DT entry similar to:
>>>> 
>>>> https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/omap5.dtsi#L365
>>> 
>>> At least for the 3630, the ti-abb-regulator driver is the same driver,
>>> but different structures based on v1, v2, or v3 are used based on
>>> which compatible flag is used.
>>> 
>>> I tried enabling the vbb-supply in the device tree, but the driver
>>> doesn't load it without .multi_regulator being true.
>>> 
>>> cpus {
>>> /* OMAP3630/OMAP37xx variants OPP50 to OPP130 and OPP1G */
>>>    cpu: cpu@0 {
>>>         operating-points-v2 = <&cpu0_opp_table>;
>>>         vbb-supply = <&abb_mpu_iva>;
>> 
>> Oh, that is great that the app_mpu_iva already exists!
>> 
>> So we just need plumbing pieces together.
>> 
>>>         clock-latency = <300000>; /* From omap-cpufreq driver */
>>>    };
>>> };
>>> 
>>> I enabled that in the 3630 structure, but then the opp must list
>>> voltage points for both regulators.
>>> Looking at the entry for abb_mpu_iva, it appears to have voltages that
>>> directly match the OPP table, so I made a duplicate entry:
>>> 
>>> opp-microvolt = <1012500 1012500 1012500>,
>>>                          <1012500 1012500 1012500>;
> 
> Out of curiosity, if we're only going to use one value for the opp
> voltage, do we need to have 3 listed?  when I looked at the driver
> yesterday, it appears to support either 1 or 3 entries per opp.
> If we're going to support two regulators, showing them as
> opp-microvolt = <1012500>, <1012500>; would be cleaner and can fit on
> one line.

Well, IMHO it would be cleaner to specify min and max values as well
since the data sheets define them. It is also not clear if we need
them for AVS or such mechanisms.

> 
>>> 
>>> and similar for 600, 800 and 1000 similar to the way dra7.dtsi does
>> 
>> Yes.
>> 
>>> it, but then I got some nasty errors and crashes.
>> 
>> I have done the same but not (yet) seen a crash or error. Maybe you had
>> a typo?
> 
> Can you send me an updated patch?  I'd like to try to get where you
> are that doesn't crash.

Yes, as soon as I have access.

> 
>> 
>>> 
>>> I started undoing the stuff, and I wanted to see if the abb_mpu_iva
>>> regulator was even running, but I would get -22 errors when I went to
>>> read the voltage.
>>> 
>>> # cat /sys/devices/platform/68000000.ocp/483072f0.regulator-abb-mpu/regulator/regulator.5/microvolts
>>> -22
>> 
>> So it reports wrong voltage settings of -22ÂV...
>> 
>> I have tested and have the same.
>> 
>> root@letux:~# cd /sys/bus/platform/devices/483072f0.regulator-abb-mpu/regulator/regulator.3/
>> root@letux:/sys/bus/platform/devices/483072f0.regulator-abb-mpu/regulator/regulator.3# ls -l
>> total 0
>> lrwxrwxrwx 1 root root    0 Jan  1 00:02 device -> ../../../483072f0.regulator-abb-mpu
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 max_microvolts
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 microvolts
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 min_microvolts
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 name
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 num_users
>> lrwxrwxrwx 1 root root    0 Jan  1 00:02 of_node -> ../../../../../../firmware/devicetree/base/ocp@68000000/regulator-abb-mpu
>> drwxr-xr-x 2 root root    0 Jan  1 00:02 power
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 requested_microamps
>> lrwxrwxrwx 1 root root    0 Jan  1 00:02 subsystem -> ../../../../../../class/regulator
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 suspend_disk_state
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 suspend_mem_state
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 suspend_standby_state
>> -r--r--r-- 1 root root 4096 Jan  1 00:02 type
>> -rw-r--r-- 1 root root 4096 Jan  1 00:02 uevent
>> root@letux:/sys/bus/platform/devices/483072f0.regulator-abb-mpu/regulator/regulator.3# cat *
>> cat: device: Is a directory
>> 1375000
>> -22
>> 1012500
>> abb_mpu_iva
>> 1
>> cat: of_node: Is a directory
>> cat: power: Is a directory
>> 0
>> cat: subsystem: Is a directory
>> disabled
>> disabled
>> disabled
>> voltage
>> OF_NAME=regulator-abb-mpu
>> OF_FULLNAME=/ocp@68000000/regulator-abb-mpu
>> OF_COMPATIBLE_0=ti,abb-v1
>> OF_COMPATIBLE_N=1
> 
> I concur with your findings.
> 
>> root@letux:/sys/bus/platform/devices/483072f0.regulator-abb-mpu/regulator/regulator.3# cd
>> root@letux:~# cpufreq-info
>> cpufrequtils 008: cpufreq-info (C) Dominik Brodowski 2004-2009
>> Report errors and bugs to cpufreq@xxxxxxxxxxxxxxx, please.
>> analyzing CPU 0:
>>  driver: cpufreq-dt
>>  CPUs which run at the same hardware frequency: 0
>>  CPUs which need to have their frequency coordinated by software: 0
>>  maximum transition latency: 300 us.
>>  hardware limits: 300 MHz - 1000 MHz
>>  available frequency steps: 300 MHz, 600 MHz, 800 MHz, 1000 MHz
>>  available cpufreq governors: conservative, userspace, powersave, ondemand, performance
>>  current policy: frequency should be within 300 MHz and 1000 MHz.
>>                  The governor "ondemand" may decide which speed to use
>>                  within this range.
>>  current CPU frequency is 300 MHz (asserted by call to hardware).
>>  cpufreq stats: 300 MHz:31.36%, 600 MHz:4.23%, 800 MHz:3.76%, 1000 MHz:60.65%  (1933)
>> root@letux:~#
>> 
>> So it runs with different OPPs... My chip may also be more robust to wrong ABB FBB setting.
>> 
>>> 
>>> If someone has any suggestions on how to test the abb_mpu_iva driver,
>>> let me know.
>> 
>> Well, next I want to look into the code for cat microvolts and
>> maybe add some printk to understand the result...
>> 
>> A first result is that it comes from
>> 
>>        /* We do not know where the OPP voltage is at the moment */
>>        abb->current_info_idx = -EINVAL;
>> 
>> But this is not treated as an -EINVAL but value of -22 microvolts...
>> Maybe an error check is missing somewhere in the regulator core.
> 
> I assumed this to be -EINVAL, but I'd be happy to be wrong.

It seems that cat microvolts stringifies the int returned from reading
the regulator voltage.

Since it is initialized to -EINVAL it returns "-22" as string instead of
converting into an errno return when reading /sys... So one step is
missing a proper error check.

But that is just a symptom that there is no call to set a good voltage.

BR,
Nikolaus