Message-ID: <20210624043240.n6m3cdftz75lhm3t@vireshk-i7>
Date:   Thu, 24 Jun 2021 10:02:40 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Rob Herring <robh@...nel.org>
Cc:     Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Yangtao Li <tiny.windzz@...il.com>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>, linux-pm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        Georgi Djakov <djakov@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Leonard Crestez <leonard.crestez@....com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 2/2] dt-bindings: opp: Convert to DT schema

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@...aro.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 ?

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

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

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

-- 
viresh

Powered by blists - more mailing lists