[RFC] OPP: Redefine bindings to overcome shortcomings

[Viresh Kumar]

On 29 December 2014 at 22:35, Rob Herring <robherring2 at gmail.com> wrote:
>> +  - frequency-kHz: Frequency in kHz
>
> s/kHz/khz/
>
>> +  - voltage-uV: voltage in micro Volts
>
> -microvolt is more consistent with regulator binding.
>
> The names are a bit redundant. perhaps opp-khz and opp-microvolt instead.

All accepted.

>> +Example 1: Simple case of dual-core cortex A9-single cluster, sharing
>> clock line.
>> +
>> +/ {
>> +       cpus {
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>> +               cpu at 0 {
>> +                       compatible = "arm,cortex-a9";
>> +                       reg = <0>;
>> +                       next-level-cache = <&L2>;
>> +                       operating-points-v2 = <&opp0>;
>> +
>> +                       opp0: opp0 {
>
> I don't really like having this under cpu0 when it applies to all
> cpus. I would move it out of /cpus.

That's how I had it initially, but then Arnd didn't like it much.

>> +                               compatible = "linux,cpu-dvfs";
>
> This should not be linux specific. Probably not cpu specific either.

This was just an example of one of the bindings and there will be others
as well.

'linux' here doesn't mean linux specific, but just that its first used by
Linux. That's what my understanding is atleast.

>> +                               clocks = <&clk-controller 0>;
>> +                               clock-names = "cpu";
>
> clocks are an input to the cpu, not really an opp. You could have a an
> OPP which uses a different parent clock, but that is most likely a
> switch within the clock controller rather than 2 clock inputs to the
> cpu.
>
> I think the clock binding for cpus should stand on its own independent of OPPs.
>
>> +                               opp-supply = <&cpu-supply0>;
>
> Same comment as clocks applies here.

Both will be kept in the cpu node where they were initially.

>> +                               voltage-tolerance = <2>; /* percentage */
>> +                               clock-latency = <300000>;
>
> These could be per entry. I'm not sure it is worth the savings to not
> just always specify them per entry.

Done.

> We should append units (-us) to clock-latency unless there is a good
> reason to maintain compatibility.

So you meant something like this:

clock-latency-us = <300000>;

right?

>> +                               opp-list = <&opplist0>;
>
> With the above changes, having this list is unnecessary.

It might be for the use case I mentioned earlier about something
like Krait.

> So, what I envision is like this:
>
> /cpus {
>   cpu at 0 {
>     clocks = <...>;
>     cpu-supply = <...>;
>     operating-point-v2 = <&cpu0-opp>;
>    };
>   cpu at 1 {
>     clocks = <...>;
>     cpu-supply = <...>;
>     operating-point-v2 = <&cpu1-opp>;
>    };
> };

Looks fine..

> cpu0-opp : opp0 {
>   compatible = "operating-point-table";
>   entry0 {
>     opp-khz = <500000>;
>     opp-microvolt = <900000>;
>   };
>   entry1 {
>     opp-khz = <1000000>;
>     opp-microvolt = <1000000>;
>     turbo-mode;
>   };
> };
> cpu1-opp : opp1 {
>   compatible = "operating-point-table";
> ...
> };

What about something like Krait which wants to use exactly same
bindings for all CPUs but want to specify they are controlled separately.

So I had it like:

cpu0-opp : opp0 {
  compatible = "operating-point-table";
  opp-list = <&opplist0>;

  opplist0: opp-list0 {
          entry0 {
            opp-khz = <500000>;
            opp-microvolt = <900000>;
          };
          entry1 {
            opp-khz = <1000000>;
            opp-microvolt = <1000000>;
            turbo-mode;
          };
  };
};

cpu1-opp : opp1 {
  compatible = "operating-point-table";
  opp-list = <&opplist0>;
};

> We need to also consider if this all works for other non-cpu OPPs like
> GPUs or DRAM/bus.

Do you any input here? Or if you know somebody who can give inputs
about them?