On 31 December 2014 at 10:17, Viresh Kumar [off-list ref] wrote:

On 29 December 2014 at 22:35, Rob Herring [off-list ref] wrote:

quoted

quoted

+  - frequency-kHz: Frequency in kHz

s/kHz/khz/

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

+                               opp-supply = <&cpu-supply0>;

Same comment as clocks applies here.

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

quoted

quoted

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

quoted

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?

quoted

quoted

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

quoted

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

quoted

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>;
};

Gentle reminder, so that we can close this long standing issue soon..