Re: [PATCH 2/2] dt-bindings: opp: Convert to DT schema

Rob Herring
2021-06-24T14:00:37Z

On Wed, Jun 23, 2021 at 10:32 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Thanks for taking it up :)
>
> On 23-06-21, 17:07, Rob Herring wrote:
> > diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> > +$id: http://devicetree.org/schemas/opp/opp-v2-base.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic OPP (Operating Performance Points) Common Binding
> > +
> > +maintainers:
> > + - Viresh Kumar <viresh.kumar@linaro.org>
> > +
> > +description: |
> > + Devices work at voltage-current-frequency combinations and some implementations
> > + have the liberty of choosing these. These combinations are called Operating
> > + Performance Points aka OPPs. This document defines bindings for these OPPs
> > + applicable across wide range of devices. For illustration purpose, this document
> > + uses CPU as a device.
> > +
> > + This describes the OPPs belonging to a device.
> > +
> > +select: false
> > +
> > +properties:
> > + $nodename:
> > + pattern: '^opp-table(-[a-z0-9]+)?$'
> > +
> > + opp-shared:
> > + description:
> > + Indicates that device nodes using this OPP Table Node's phandle switch
> > + their DVFS state together, i.e. they share clock/voltage/current lines.
> > + Missing property means devices have independent clock/voltage/current
> > + lines, but they share OPP tables.
> > + type: boolean
> > +
> > +patternProperties:
> > + '^opp-?[0-9]+$':
> > + type: object
> > + description:
> > + One or more OPP nodes describing voltage-current-frequency combinations.
> > + Their name isn't significant but their phandle can be used to reference an
> > + OPP. These are mandatory except for the case where the OPP table is
> > + present only to indicate dependency between devices using the opp-shared
> > + property.
> > +
> > + properties:
> > + opp-hz:
> > + description:
> > + Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
> > + required property for all device nodes, unless another "required"
> > + property to uniquely identify the OPP nodes exists. Devices like power
> > + domains must have another (implementation dependent) property.
> > +
> > + opp-peak-kBps:
> > + description:
> > + Peak bandwidth in kilobytes per second, expressed as an array of
> > + 32-bit big-endian integers. Each element of the array represents the
> > + peak bandwidth value of each interconnect path. The number of elements
> > + should match the number of interconnect paths.
> > + minItems: 1
> > + maxItems: 32 # Should be enough
>
> Can we move this down, closer to opp-avg-kBps ?

Sure.

> > +
> > + opp-microvolt:
> > + description: |
> > + Voltage for the OPP
> > +
> > + A single regulator's voltage is specified with an array of size one or three.
> > + Single entry is for target voltage and three entries are for <target min max>
> > + voltages.
> > +
> > + Entries for multiple regulators shall be provided in the same field separated
> > + by angular brackets <>. The OPP binding doesn't provide any provisions to
> > + relate the values to their power supplies or the order in which the supplies
> > + need to be configured and that is left for the implementation specific
> > + binding.
> > +
> > + Entries for all regulators shall be of the same size, i.e. either all use a
> > + single value or triplets.
> > + minItems: 1
> > + maxItems: 8
>
> For consistency with rest of the doc, maybe add
>
> # Should be enough regulators
>
> > + items:
> > + minItems: 1
> > + maxItems: 3
> > +
> > + opp-microamp:
> > + description: |
> > + The maximum current drawn by the device in microamperes considering
> > + system specific parameters (such as transients, process, aging,
> > + maximum operating temperature range etc.) as necessary. This may be
> > + used to set the most efficient regulator operating mode.
> > +
> > + Should only be set if opp-microvolt(-name)? is set for the OPP.
>
> What is the significance of '?' here ?

regex. '?' means optional.

> > + Entries for multiple regulators shall be provided in the same field
> > + separated by angular brackets <>. If current values aren't required
> > + for a regulator, then it shall be filled with 0. If current values
> > + aren't required for any of the regulators, then this field is not
> > + required. The OPP binding doesn't provide any provisions to relate the
> > + values to their power supplies or the order in which the supplies need
> > + to be configured and that is left for the implementation specific
> > + binding.
> > + minItems: 1
> > + maxItems: 8 # Should be enough regulators
> > + items:
> > + minItems: 1
> > + maxItems: 3

Actually, I need to drop these 3 lines as opp-microamp doesn't have a
range like opp-microvolt.

>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> --
> viresh