Re: [PATCH v4 1/3] dt-bindings: Add YAML bindings for NVDEC

[Rob Herring <robh@kernel.org>]

On Fri, Sep 3, 2021 at 12:29 PM Mikko Perttunen <cyndis@kapsi.fi> wrote:
>
> On 9/3/21 7:34 PM, Rob Herring wrote:
> > On Fri, Sep 03, 2021 at 11:31:53AM +0300, Mikko Perttunen wrote:
> >> Add YAML device tree bindings for NVDEC, now in a more appropriate
> >> place compared to the old textual Host1x bindings.
> >>
> >> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> >> ---
> >> v4:
> >> * Fix incorrect compatibility string in 'if' condition
> >> v3:
> >> * Drop host1x bindings
> >> * Change read2 to read-1 in interconnect names
> >> v2:
> >> * Fix issues pointed out in v1
> >> * Add T194 nvidia,instance property
> >> ---
> >>   .../gpu/host1x/nvidia,tegra210-nvdec.yaml     | 109 ++++++++++++++++++
> >>   MAINTAINERS                                   |   1 +
> >>   2 files changed, 110 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> >> new file mode 100644
> >> index 000000000000..33d01c7dc759
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> >> @@ -0,0 +1,109 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: "http://devicetree.org/schemas/gpu/host1x/nvidia,tegra210-nvdec.yaml#"
> >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> >> +
> >> +title: Device tree binding for NVIDIA Tegra NVDEC
> >> +
> >> +description: |
> >> +  NVDEC is the hardware video decoder present on NVIDIA Tegra210
> >> +  and newer chips. It is located on the Host1x bus and typically
> >> +  programmed through Host1x channels.
> >> +
> >> +maintainers:
> >> +  - Thierry Reding <treding@gmail.com>
> >> +  - Mikko Perttunen <mperttunen@nvidia.com>
> >> +
> >> +properties:
> >> +  $nodename:
> >> +    pattern: "^nvdec@[0-9a-f]*$"
> >> +
> >> +  compatible:
> >> +    enum:
> >> +      - nvidia,tegra210-nvdec
> >> +      - nvidia,tegra186-nvdec
> >> +      - nvidia,tegra194-nvdec
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clocks:
> >> +    maxItems: 1
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: nvdec
> >> +
> >> +  resets:
> >> +    maxItems: 1
> >> +
> >> +  reset-names:
> >> +    items:
> >> +      - const: nvdec
> >> +
> >> +  power-domains:
> >> +    maxItems: 1
> >> +
> >> +  iommus:
> >> +    maxItems: 1
> >> +
> >> +  interconnects:
> >> +    items:
> >> +      - description: DMA read memory client
> >> +      - description: DMA read 2 memory client
> >> +      - description: DMA write memory client
> >> +
> >> +  interconnect-names:
> >> +    items:
> >> +      - const: dma-mem
> >> +      - const: read-1
> >> +      - const: write
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - clocks
> >> +  - clock-names
> >> +  - resets
> >> +  - reset-names
> >> +  - power-domains
> >> +
> >> +if:
> >> +  properties:
> >> +    compatible:
> >> +      contains:
> >> +        const: nvidia,tegra194-nvdec
> >> +then:
> >> +  properties:
> >> +    nvidia,instance:
> >> +      items:
> >> +        - description: 0 for NVDEC0, or 1 for NVDEC1
> >
> > I still don't understand what this is needed for. What is the difference
> > between the instances? There must be some reason you care. We should
> > describe that difference, not some made up index.
> >
> > I'm not suggesting using the base address either. That's fragile too.
>
> This device is on the Host1x bus. On that bus, each device has an
> identifier baked into hardware called 'class' that is used when
> accessing devices through some mechanisms (host1x channels). As such,
> when probing the device we need to specify the class of the device to
> the host1x driver so it knows how to talk to it. Those class numbers are
> fixed so we have hardcoded them in the driver, but now that we have two
> NVDECs, we need to distinguish between them so that we can specify the
> correct class for each instance to the host1x driver.

Then why don't you have a property like 'nvidia,host1x-class'
containing the class number?


> >> +additionalProperties: true
> >
> > 'true' here is not allowed unless the schema is not complete and
> > intended to be included in a complete schema or unconditionally applied
> > (i.e. 'select: true'). This case is neither. As pointed out previously,
> > 'unevaluatedProperties' is what you'd want here.
> >
> > However, I looked into supporting defining properties in if/then/else
> > schemas as you have done and I don't think we will support that soon.
> > It's problematic because we can't validate the schema under the if/then
> > completely. The reason is properties under if/then schemas don't have to
> > be complete as we expect a top level definition that is complete (e.g.
> > vendor properties must have 'description'). To solve this, we'd have to
> > only apply meta-schema checks if the property doesn't appear at the top
> > level. That's more complicated than I care to implement ATM.
>
> I see two paths here: either keep 'additionalProperties: true' or remove
> it and have this binding trigger validation failures. Which one do you
> suggest or is there some third option?

Define the property at the top level, then restrict it in the if/then schema:

if:
  properties:
    compatible:
      not:
        contains:
          const: nvidia,tegra194-nvdec
then:
  properties:
    nvidia,instance: false

(Or 'not: {required: [ nvidia,instance ]}' would work here, too)

With that, 'additionalProperties: false' will work.

Rob