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

[Adam Ford]

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
>