Date:   Tue, 10 Sep 2019 13:30:23 -0500
From:   Adam Ford <aford173@...il.com>
To:     "H. Nikolaus Schaller" <hns@...delico.com>
Cc:     Tony Lindgren <tony@...mide.com>,
        André Roth <neolynx@...il.com>,
        Linux-OMAP <linux-omap@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andreas Kemnade <andreas@...nade.info>,
        Nishanth Menon <nm@...com>
Subject: Re: [Letux-kernel] [RFC PATCH 0/3] Enable 1GHz support on omap36xx

On Tue, Sep 10, 2019 at 11:59 AM H. Nikolaus Schaller <hns@...delico.com> wrote:
>
> Hi Adam,
>
> > Am 09.09.2019 um 21:13 schrieb Adam Ford <aford173@...il.com>:
> >
> > On Mon, Sep 9, 2019 at 1:11 PM H. Nikolaus Schaller <hns@...delico.com> 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.

> >
> > 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.

>
> >
> > 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@...ux:~# cd /sys/bus/platform/devices/483072f0.regulator-abb-mpu/regulator/regulator.3/
> root@...ux:/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@...00000/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@...ux:/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@...00000/regulator-abb-mpu
> OF_COMPATIBLE_0=ti,abb-v1
> OF_COMPATIBLE_N=1

I concur with your findings.

> root@...ux:/sys/bus/platform/devices/483072f0.regulator-abb-mpu/regulator/regulator.3# cd
> root@...ux:~# cpufreq-info
> cpufrequtils 008: cpufreq-info (C) Dominik Brodowski 2004-2009
> Report errors and bugs to cpufreq@...r.kernel.org, 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@...ux:~#
>
> 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.

>
> I just need more time to track it down.
>
> BR and thanks for the description,
> Nikolaus
>

Powered by blists - more mailing lists