[PATCH v6 2/2] Documentation: devicetree: Add boost-frequency binding to list boost mode frequency

[Thomas Abraham]

On Fri, May 30, 2014 at 11:45 PM, Tomasz Figa <tomasz.figa at gmail.com> wrote:
>
>
> On 30.05.2014 20:05, Thomas Abraham wrote:
>> Hi Mark,
>>
>> On Fri, May 30, 2014 at 6:38 PM, Mark Rutland <mark.rutland at arm.com> wrote:
>>> 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:
>>>> From: Thomas Abraham <thomas.ab at samsung.com>
>>>>
>>>> Add a new optional boost-frequency binding for specifying the frequencies
>>>> usable in boost mode.
>>>>
>>>> Cc: Rob Herring <robh+dt at kernel.org>
>>>> Cc: Pawel Moll <pawel.moll at arm.com>
>>>> Cc: Mark Rutland <mark.rutland at arm.com>
>>>> Cc: Ian Campbell <ijc+devicetree at hellion.org.uk>
>>>> Cc: Kumar Gala <galak at codeaurora.org>
>>>> Signed-off-by: Thomas Abraham <thomas.ab at samsung.com>
>>>> Acked-by: Viresh Kumar <viresh.kumar at linaro.org>
>>>> Acked-by: Nishanth Menon <nm at ti.com>
>>>> Acked-by: Lukasz Majewski <l.majewski at samsung.com>
>>>> ---
>>>>  .../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])
>>>
>>>> +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.
>>
>
> IMHO the most important part that I believe should be stated in the
> documentation is that CPU usually can operate in boost mode for limited
> amount of time, which depends on thermal conditions, which makes the
> boost operating points separate from normal ones, which can be used at
> any time.

The factors that allow a CPU to operate in boost mode and the duration
the CPU can operate in boost mode clock speed is a CPU specific
description. This binding does not describe such CPU specific
behavior. This binding only states the additional clock speeds CPU is
allowed to operate in boost mode. The boost mode entry and exit
conditions are implementation specific and not part of the scope of
this binding and documentation.

>
>> The mechanism of entry into and exit out of boost mode is outside the
>> scope of this documentation.
>>
>>>
>>> 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.
>>
>>>
>>> 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?
>
> It seems reasonable to have boost operating points completely separate
> from normal ones, so that a kernel without support for boost mode will
> not use them. Also considering my comment above, logically boost
> operating points are not considered normal operating points, due to
> various constraints that need to be met to use them (i.e. mostly thermal
> conditions).
>
>>
>> Thanks,
>> Thomas.
>>
>>>
>>>> +
>>>> +Example:
>>>> +
>>>> +     cpus {
>>>> +             #address-cells = <1>;
>>>> +             #size-cells = <0>;
>>>> +             cpu at 0 {
>>>> +                     device_type = "cpu";
>>>> +                     compatible = "arm,cortex-a9";
>>>> +                     reg = <0>;
>>>> +
>>>> +                     operating-points = <
>>>> +                             1500000 1350000
>>>> +                             1400000 1287500
>>>> +                             1300000 1250000
>>>> +                             1200000 1187500
>>>> +                             1100000 1137500
>>>> +                             1000000 1087500
>>>> +                     >;
>>>> +                     boost-frequencies = <1500000 1400000>;
>>>
>>> This is more of a general issue, but I hate the whole cpufreq-cpu0 way
>>> of assuming that all CPUs mirror CPU0.
>>>
>>> It would be nicer if either this were dropped in /cpus or repeated
>>> per-cpu.
>
> Would it make any difference?
>
> IMHO, as long as the binding is well defined and doesn't cause
> significant issues, I don't think there is a reason to redesign it and
> add a necessity to support both new and old variants.
>
> Best regards,
> Tomasz