On Fri, Sep 3, 2021 at 12:29 PM Mikko Perttunen <cyndis@xxxxxxxx> 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@xxxxxxxxxx> > >> --- > >> 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@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-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
