| Message ID | 20210903083155.690022-1-mperttunen@nvidia.com |
|---|---|
| Headers | show |
| Series | NVIDIA Tegra NVDEC support | expand |
On Fri, 03 Sep 2021 11:31:53 +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 > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml:109:1: [warning] too many blank lines (2 > 1) (empty-lines) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1524104 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
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. > + > +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. Rob
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