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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 18, 2021 at 8:18 AM Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
>
> On Wed, Aug 18, 2021 at 11:24:28AM +0300, Mikko Perttunen wrote:
> > On 8/18/21 12:20 AM, Rob Herring wrote:
> > > On Wed, Aug 11, 2021 at 01:50:28PM +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@xxxxxxxxxx>
> > > > ---
> > > > 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..571849625da8
> > > > --- /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@xxxxxxxxx>
> > > > +  - Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > > > +
> > > > +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-host1x
> > >
> > > host1x? This will never be true as the schema is only applied to nodes
> > > with the nvdec compatible.
> >
> > Argh, it's a typo indeed. Should be nvidia,tegra194-nvdec.
> >
> > >
> > > > +then:
> > > > +  properties:
> > > > +    nvidia,instance:
> > > > +      items:
> > > > +        - description: 0 for NVDEC0, or 1 for NVDEC1
> > >
> > > What's this for? We generally don't do indices in DT.
> >
> > When programming the hardware through Host1x, we need to know the "class ID"
> > of the hardware, specific to each instance. So we need to know which
> > instance it is. Technically of course we could derive this from the MMIO
> > address but that seems more confusing.
> >
> > >
> > > > +
> > > > +additionalProperties: true
> > >
> > > This should be false or 'unevaluatedProperties: false'
> >
> > I tried that but it resulted in validation failures; please see the
> > discussion in v2.
>
> Rob mentioned that there is now support for unevaluatedProperties in
> dt-schema. I was able to test this, though with only limited success. I
> made the following changes on top of this patch:

Here's a branch that works with current jsonschema master:

https://github.com/robherring/dt-schema/tree/draft2020-12

> --- >8 ---
> diff --git a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> index d2681c98db7e..0bdf05fc8fc7 100644
> --- a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> +++ b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> @@ -73,14 +73,18 @@ if:
>    properties:
>      compatible:
>        contains:
> -        const: nvidia,tegra194-host1x
> +        const: nvidia,tegra194-nvdec
>  then:
>    properties:
>      nvidia,instance:
> +      $ref: /schemas/types.yaml#/definitions/uint32
>        items:
>          - description: 0 for NVDEC0, or 1 for NVDEC1
>
> -additionalProperties: true
> +  required:
> +    - nvidia,instance
> +
> +unevaluatedProperties: false
>
>  examples:
>    - |
> @@ -105,3 +109,28 @@ examples:
>              interconnect-names = "dma-mem", "read-1", "write";
>              iommus = <&smmu TEGRA186_SID_NVDEC>;
>      };
> +
> +  - |
> +    #include <dt-bindings/clock/tegra194-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/memory/tegra194-mc.h>
> +    #include <dt-bindings/power/tegra194-powergate.h>
> +    #include <dt-bindings/reset/tegra194-reset.h>
> +
> +    nvdec@15480000 {
> +            compatible = "nvidia,tegra194-nvdec";
> +            reg = <0x15480000 0x40000>;
> +            clocks = <&bpmp TEGRA194_CLK_NVDEC>;
> +            clock-names = "nvdec";
> +            resets = <&bpmp TEGRA194_RESET_NVDEC>;
> +            reset-names = "nvdec";
> +
> +            nvidia,instance = <0>;
> +
> +            power-domains = <&bpmp TEGRA194_POWER_DOMAIN_NVDECA>;
> +            interconnects = <&mc TEGRA194_MEMORY_CLIENT_NVDECSRD &emc>,
> +                            <&mc TEGRA194_MEMORY_CLIENT_NVDECSRD1 &emc>,
> +                            <&mc TEGRA194_MEMORY_CLIENT_NVDECSWR &emc>;
> +            interconnect-names = "dma-mem", "read-1", "write";
> +            iommus = <&smmu TEGRA194_SID_NVDEC>;
> +    };
> --- >8 ---
>
> As I said, this only works partially. One thing I have to do is comment
> out the whole "if:" block in meta-schemas/base.yaml in order to get rid
> of this error:
>
>         Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml: 'additionalProperties' is a required property
>         hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties"
>         from schema $id: http://devicetree.org/meta-schemas/base.yaml#
>
> which basically makes the whole file invalid. The reason seems to be
> that dt-schema will only allow unevaluatedProperties if there are any
> $ref references in the schema. Although I'm not sure I understand how
> exactly that works because I tried to inject a dummy $ref but that
> didn't fix the above error.

I think we'll end up relaxing this with 'unevaluatedProperties'
supported. Primarily for just this case with a conditionally defined
property.

> Once that's worked around, though, I do get the examples to validate
> with just a small caveat: nvidia,instance is recognized as being
> required in the Tegra194 case (if I remove it from the example, I do get
> an error, as expected), but if I add nvidia,instance to the Tegra186
> example, it doesn't actually produce an error and will just accept it as
> valid, even though the compatible is not nvidia,tegra194-nvdec.
>
> I don't have time at the moment to investigate why that is, but just
> thought I'd report this here in the meantime. Perhaps it's already a
> known issue?
>
> We could potentially side-step this by getting rid of the custom
> nvidia,instance property altogether. Unfortunately of_device_id table
> matching only supports matching by name, but not by unit-address, which
> would've been nice in this case. Matching by base address manually is a
> bit suboptimal, but it's not that bad.
>
> In any case, there are other examples I know of which need this type of
> functionality (a bunch of devices where the number and names of power
> supplies change from one generation to another), so this problem isn't
> going entirely away anyway.

That's pretty common (though I think we get more variation than we
should), but why would you need the instance or base address for this?

Rob



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux