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

[Rob Herring]

On Wed, Jun 23, 2021 at 10:32 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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@xxxxxxxxxx>
> > +
> > +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@xxxxxxxxxx>
>
> --
> viresh