On 30.05.2014 22:33, Nishanth Menon wrote:

On 05/30/2014 03:19 PM, Tomasz Figa wrote:

quoted

On 30.05.2014 22:13, Nishanth Menon wrote:

quoted

On 05/30/2014 03:02 PM, Tomasz Figa wrote:

quoted

On 30.05.2014 21:50, Nishanth Menon wrote:

quoted

On 05/30/2014 01:55 PM, Mark Rutland wrote:

quoted

On Fri, May 30, 2014 at 07:05:43PM +0100, Thomas Abraham wrote:

quoted

Hi Mark,

On Fri, May 30, 2014 at 6:38 PM, Mark Rutland [off-list ref] wrote:

quoted

Hi,

Apologies for being somewhat late w.r.t. review on this.

On Fri, May 30, 2014 at 10:01:17AM +0100, Thomas Abraham wrote:

quoted

From: Thomas Abraham <redacted>

Add a new optional boost-frequency binding for specifying the frequencies
usable in boost mode.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <redacted>
Cc: Kumar Gala <redacted>
Signed-off-by: Thomas Abraham <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Nishanth Menon <nm@ti.com>
Acked-by: Lukasz Majewski <redacted>
---
 .../devicetree/bindings/cpufreq/cpufreq-boost.txt  |   38 ++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
new file mode 100644
index 0000000..63ed0fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt

@@ -0,0 +1,38 @@
+* Device tree binding for CPU boost frequency (aka over-clocking)
+
+Certain CPU's can be operated in optional 'boost' mode (or sometimes referred as

Nit: CPUs (we're not greengrocers [1])

quoted

+overclocking) in which the CPU can operate at frequencies which are not
+specified by the manufacturer as CPU's operating frequency.
+
+Optional Properties:
+- boost-frequencies: list of frequencies in KHz to be used only in boost mode.
+  This list should be a subset of frequencies listed in "operating-points"
+  property. Refer to Documentation/devicetree/bindings/power/opp.txt for
+  details about "operating-points" property.

What is 'boost-mode'?

boost-mode activates additional one or more cpu clock speeds (which
are not specified as operating frequency of the cpu by the
manufacturer). The cpu is allowed to operate in these boost mode
speeds when the boost mode is active. The boost mode speeds are
usually undocumented. Some of the chip samples could be clocked in
boost mode speeds and only such samples can be safely operated in
boost mode.

The mechanism of entry into and exit out of boost mode is outside the
scope of this documentation.

quoted

What are the limitations on boost frequencies? When is a CPU expected to
go to these frequencies and for now long? When should I as a dt author
place elements in boost-frequencies?

I will add these details in the next revision of this patch.

Cheers.

quoted

quoted

Why are these in both operating-points and boost-frequencies? It'll be
really easy to accidentally forget to mark something as a
boost-frequency this way. Why not have a boost-points instead?

Does boost-points mean a set of clock speeds which are not listed as
part of operating-points property? If yes, that also is a possible
implementation (it was implemented in one of the earlier version of
this series). Could you confirm that you want the boost mode speeds to
be exclusive of the speeds listed in operating-points?

If these boost mode operating points are not generally advisable for use
as the other operating-points are, then they should IMO been in an
entirely separate property exclusive of (but in the same format as) the
operating-points property, e.g.

operating points = <A B>, <C D>;
boost-points = <E F>;

you are asking boost frequencies to remain for ever tied down to OPP
style description.

What we are trying to describe? "What are my SoC's overclocked
frequencies"? That description can be used even in a system that does
not use OPP style table (say ACPI based OPP tables or whatever
acronymned table).

Tying it down to operating points just because we have it today as a
convenient description, is limiting.

Further, if we decide to educate boost-frequencies to also indicate
how long is it safe? That does indeed belong to boost-frequency
description and not OPP description. Or if we decide to change
operating-points description[1] in the future has an impact on
"boost-points" description, when it should not have.

quoted

Otherwise, without boost-mode support we have to parse the boost mode
table to figure out which points to avoid. Or if someone typos a value

That is OS usage of h/w description - yeah - in anycase, if OS has no
ability to deal with boost-frequencies, it should skip it for sure.

quoted

in either table we might go into a boost mode when we didn't want to!

There are other ways to screw up device with dts typo. you could give
a wrong voltage(extra 0?) and ensure you never use the chip ever
again.. typos are dt bugs, we can do the best to write robust code to
report them.

Typos are not the primary thing to worry about here. Adding boost
frequencies to the list of primary operating points is flawed, because
an OS that has no idea of boost mode will use them as normal operating
points and this is not desired.

That means we have an implementation bug in OS since it does not
consider the complete hardware description that device tree is
providing. An analogy will be a regulator compatible match being used
but regulator-min-microvolt and regulator-max-microvolt being ignored
by OS.

No. The operating points bindings were defined far before this
boost-frequency and so there is no requirement to support the latter.

So, we dont add any new bindings ever again? /me blinks. *IF* we add a
new property in the future, do we expect the new description to be
supported in older kernel(which could not have known about it)? How
far are we taking this ABI thing?
Documentation/devicetree/bindings/ABI.txt states:
3) Bindings can be augmented, but the driver shouldn't break when given
   the old binding. ie. add additional properties, but don't change the
   meaning of an existing property. For drivers, default to the original
   behaviour when a newly added property is missing.

we are not changing the meaning of existing property, we are
augumenting it.

In my opinion, *IF* we are concerned about polluting operating-points
description, why dont we enforce that the boost operating points
should NOT be defined in the current "operating-points" description -
and - just follow what Rob suggested and iMx already does - add such
operating points from platform code.

quoted

quoted

We never said that "operating points" are "primary operating points".
all we said is they are "operating points" for the device - we dont
associate meanings to it. You may add to it[1] in platform code, as we
decided to.

Maybe generic OPP bindings don't state that, but I believe that at least
cpufreq-cpu0 bindings have been defined (and the driver implemented)
this way and changing the semantics now would be breaking DT ABI
compatibility.

Did you mean
Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt which
points to the generic bindings? Quote:
- operating-points: Refer to
Documentation/devicetree/bindings/power/opp.txt
  for details

quoted

quoted

boost-frequencies are describing "overclocked frequencies" - should it
matter if the description of that comes from platform code OR existing
opp tables or what ever? I dont think defining them as "operating
point" style as the only way of describing overclocked frequencies are
the right approach to describing it.

Nobody said that it is the only way. The whole point here is that it
should be separate from the main operating-points property, as boost
operating points are not normal operating points and the OS must be
specifically aware how to handle them.

They are descriptions - I repeat myself when I state that they are
"overclocked frequencies" that happen to map to operating points on
the platforms of concern, but may not necessarily be OPP based on
other platforms which also be able to support "overclocked frequencies".

OK, so you add overclocked frequencies to operating-points property,
boost-frequency property, build a dtb, use it with a kernel that doesn't
support boost-frequency and nicely overheat (and likely destroy) your
board. I don't think this makes too much sense, sorry.

Best regards,
Tomasz