On Tue, Sep 10, 2019 at 1:51 PM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote: > > 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. I unrolled the patches to see what a stock kernel does. When I 'cat num_users' it returns 1. Do you know if there is a way to determine who the user is? The stock tree doesn't appear to have any users of this regulator. adam > > BR, > Nikolaus >
