2022-09-13 13:51:33

by Mikko Perttunen

[permalink] [raw]
Subject: [PATCH v2 3/8] dt-bindings: Add bindings for Tegra234 NVDEC

From: Mikko Perttunen <[email protected]>

Update NVDEC bindings for Tegra234. This new engine version only has
two memory clients, but now requires three clocks, and as a bigger
change the engine loads firmware from a secure carveout configured by
the bootloader.

For the latter, we need to add a phandle to the memory controller
to query the location of this carveout, and several other properties
containing offsets into the firmware inside the carveout. These
properties are intended to be populated through a device tree overlay
configured at flashing time, so that the values correspond to the
flashed NVDEC firmware.

As the binding was getting large with many conditional properties,
also split the Tegra234 version out into a separate file.

Signed-off-by: Mikko Perttunen <[email protected]>
---
v2:
- Split out into separate file to avoid complexity with
conditionals etc.
---
.../gpu/host1x/nvidia,tegra234-nvdec.yaml | 154 ++++++++++++++++++
1 file changed, 154 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml

diff --git a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
new file mode 100644
index 000000000000..eab0475ca983
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
@@ -0,0 +1,154 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/gpu/host1x/nvidia,tegra234-nvdec.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Device tree binding for NVIDIA Tegra234 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 <[email protected]>
+ - Mikko Perttunen <[email protected]>
+
+properties:
+ $nodename:
+ pattern: "^nvdec@[0-9a-f]*$"
+
+ compatible:
+ enum:
+ - nvidia,tegra234-nvdec
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 3
+
+ clock-names:
+ items:
+ - const: nvdec
+ - const: fuse
+ - const: tsec_pka
+
+ resets:
+ maxItems: 1
+
+ reset-names:
+ items:
+ - const: nvdec
+
+ power-domains:
+ maxItems: 1
+
+ iommus:
+ maxItems: 1
+
+ dma-coherent: true
+
+ interconnects:
+ items:
+ - description: DMA read memory client
+ - description: DMA write memory client
+
+ interconnect-names:
+ items:
+ - const: dma-mem
+ - const: write
+
+ nvidia,memory-controller:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description:
+ phandle to the memory controller for determining carveout information.
+
+ nvidia,bl-manifest-offset:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Offset to bootloader manifest from beginning of firmware. Typically set as
+ part of a device tree overlay corresponding to flashed firmware.
+
+ nvidia,bl-code-offset:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Offset to bootloader code section from beginning of firmware. Typically set as
+ part of a device tree overlay corresponding to flashed firmware.
+
+ nvidia,bl-data-offset:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Offset to bootloader data section from beginning of firmware. Typically set as
+ part of a device tree overlay corresponding to flashed firmware.
+
+ nvidia,os-manifest-offset:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Offset to operating system manifest from beginning of firmware. Typically set as
+ part of a device tree overlay corresponding to flashed firmware.
+
+ nvidia,os-code-offset:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Offset to operating system code section from beginning of firmware. Typically set as
+ part of a device tree overlay corresponding to flashed firmware.
+
+ nvidia,os-data-offset:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Offset to operating system data section from beginning of firmware. Typically set as
+ part of a device tree overlay corresponding to flashed firmware.
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - resets
+ - reset-names
+ - power-domains
+ - nvidia,memory-controller
+ - nvidia,bl-manifest-offset
+ - nvidia,bl-code-offset
+ - nvidia,bl-data-offset
+ - nvidia,os-manifest-offset
+ - nvidia,os-code-offset
+ - nvidia,os-data-offset
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/tegra234-clock.h>
+ #include <dt-bindings/memory/tegra234-mc.h>
+ #include <dt-bindings/power/tegra234-powergate.h>
+ #include <dt-bindings/reset/tegra234-reset.h>
+
+ nvdec@15480000 {
+ compatible = "nvidia,tegra234-nvdec";
+ reg = <0x15480000 0x00040000>;
+ clocks = <&bpmp TEGRA234_CLK_NVDEC>,
+ <&bpmp TEGRA234_CLK_FUSE>,
+ <&bpmp TEGRA234_CLK_TSEC_PKA>;
+ clock-names = "nvdec", "fuse", "tsec_pka";
+ resets = <&bpmp TEGRA234_RESET_NVDEC>;
+ reset-names = "nvdec";
+ power-domains = <&bpmp TEGRA234_POWER_DOMAIN_NVDEC>;
+ interconnects = <&mc TEGRA234_MEMORY_CLIENT_NVDECSRD &emc>,
+ <&mc TEGRA234_MEMORY_CLIENT_NVDECSWR &emc>;
+ interconnect-names = "dma-mem", "write";
+ iommus = <&smmu_niso1 TEGRA234_SID_NVDEC>;
+ dma-coherent;
+
+ nvidia,memory-controller = <&mc>;
+
+ /* Placeholder values, to be replaced with values from overlay */
+ nvidia,bl-manifest-offset = <0>;
+ nvidia,bl-data-offset = <0>;
+ nvidia,bl-code-offset = <0>;
+ nvidia,os-manifest-offset = <0>;
+ nvidia,os-data-offset = <0>;
+ nvidia,os-code-offset = <0>;
+ };
--
2.37.0


2022-09-14 12:20:22

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 3/8] dt-bindings: Add bindings for Tegra234 NVDEC

On Tue, Sep 13, 2022 at 04:14:41PM +0300, Mikko Perttunen wrote:
> From: Mikko Perttunen <[email protected]>
>
> Update NVDEC bindings for Tegra234. This new engine version only has
> two memory clients, but now requires three clocks, and as a bigger
> change the engine loads firmware from a secure carveout configured by
> the bootloader.
>
> For the latter, we need to add a phandle to the memory controller
> to query the location of this carveout, and several other properties
> containing offsets into the firmware inside the carveout. These
> properties are intended to be populated through a device tree overlay
> configured at flashing time, so that the values correspond to the
> flashed NVDEC firmware.
>
> As the binding was getting large with many conditional properties,
> also split the Tegra234 version out into a separate file.
>
> Signed-off-by: Mikko Perttunen <[email protected]>
> ---
> v2:
> - Split out into separate file to avoid complexity with
> conditionals etc.
> ---
> .../gpu/host1x/nvidia,tegra234-nvdec.yaml | 154 ++++++++++++++++++
> 1 file changed, 154 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
>
> diff --git a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
> new file mode 100644
> index 000000000000..eab0475ca983
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
> @@ -0,0 +1,154 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/gpu/host1x/nvidia,tegra234-nvdec.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Device tree binding for NVIDIA Tegra234 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 <[email protected]>
> + - Mikko Perttunen <[email protected]>
> +
> +properties:
> + $nodename:
> + pattern: "^nvdec@[0-9a-f]*$"
> +
> + compatible:
> + enum:
> + - nvidia,tegra234-nvdec
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 3
> +
> + clock-names:
> + items:
> + - const: nvdec
> + - const: fuse
> + - const: tsec_pka
> +
> + resets:
> + maxItems: 1
> +
> + reset-names:
> + items:
> + - const: nvdec
> +
> + power-domains:
> + maxItems: 1
> +
> + iommus:
> + maxItems: 1
> +
> + dma-coherent: true
> +
> + interconnects:
> + items:
> + - description: DMA read memory client
> + - description: DMA write memory client
> +
> + interconnect-names:
> + items:
> + - const: dma-mem
> + - const: write
> +
> + nvidia,memory-controller:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description:
> + phandle to the memory controller for determining carveout information.
> +
> + nvidia,bl-manifest-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Offset to bootloader manifest from beginning of firmware. Typically set as
> + part of a device tree overlay corresponding to flashed firmware.
> +
> + nvidia,bl-code-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Offset to bootloader code section from beginning of firmware. Typically set as
> + part of a device tree overlay corresponding to flashed firmware.
> +
> + nvidia,bl-data-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Offset to bootloader data section from beginning of firmware. Typically set as
> + part of a device tree overlay corresponding to flashed firmware.
> +
> + nvidia,os-manifest-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Offset to operating system manifest from beginning of firmware. Typically set as
> + part of a device tree overlay corresponding to flashed firmware.
> +
> + nvidia,os-code-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Offset to operating system code section from beginning of firmware. Typically set as
> + part of a device tree overlay corresponding to flashed firmware.
> +
> + nvidia,os-data-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Offset to operating system data section from beginning of firmware. Typically set as
> + part of a device tree overlay corresponding to flashed firmware.

I don't think DT is the place for describing your runtime loaded
firmware layout.

Rob

2022-09-14 13:14:42

by Mikko Perttunen

[permalink] [raw]
Subject: Re: [PATCH v2 3/8] dt-bindings: Add bindings for Tegra234 NVDEC

On 9/14/22 15:08, Rob Herring wrote:
> On Tue, Sep 13, 2022 at 04:14:41PM +0300, Mikko Perttunen wrote:
>> From: Mikko Perttunen <[email protected]>
>>
>> Update NVDEC bindings for Tegra234. This new engine version only has
>> two memory clients, but now requires three clocks, and as a bigger
>> change the engine loads firmware from a secure carveout configured by
>> the bootloader.
>>
>> For the latter, we need to add a phandle to the memory controller
>> to query the location of this carveout, and several other properties
>> containing offsets into the firmware inside the carveout. These
>> properties are intended to be populated through a device tree overlay
>> configured at flashing time, so that the values correspond to the
>> flashed NVDEC firmware.
>>
>> As the binding was getting large with many conditional properties,
>> also split the Tegra234 version out into a separate file.
>>
>> Signed-off-by: Mikko Perttunen <[email protected]>
>> ---
>> v2:
>> - Split out into separate file to avoid complexity with
>> conditionals etc.
>> ---
>> .../gpu/host1x/nvidia,tegra234-nvdec.yaml | 154 ++++++++++++++++++
>> 1 file changed, 154 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
>> new file mode 100644
>> index 000000000000..eab0475ca983
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
>> @@ -0,0 +1,154 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/gpu/host1x/nvidia,tegra234-nvdec.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: Device tree binding for NVIDIA Tegra234 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 <[email protected]>
>> + - Mikko Perttunen <[email protected]>
>> +
>> +properties:
>> + $nodename:
>> + pattern: "^nvdec@[0-9a-f]*$"
>> +
>> + compatible:
>> + enum:
>> + - nvidia,tegra234-nvdec
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + clocks:
>> + maxItems: 3
>> +
>> + clock-names:
>> + items:
>> + - const: nvdec
>> + - const: fuse
>> + - const: tsec_pka
>> +
>> + resets:
>> + maxItems: 1
>> +
>> + reset-names:
>> + items:
>> + - const: nvdec
>> +
>> + power-domains:
>> + maxItems: 1
>> +
>> + iommus:
>> + maxItems: 1
>> +
>> + dma-coherent: true
>> +
>> + interconnects:
>> + items:
>> + - description: DMA read memory client
>> + - description: DMA write memory client
>> +
>> + interconnect-names:
>> + items:
>> + - const: dma-mem
>> + - const: write
>> +
>> + nvidia,memory-controller:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description:
>> + phandle to the memory controller for determining carveout information.
>> +
>> + nvidia,bl-manifest-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + Offset to bootloader manifest from beginning of firmware. Typically set as
>> + part of a device tree overlay corresponding to flashed firmware.
>> +
>> + nvidia,bl-code-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + Offset to bootloader code section from beginning of firmware. Typically set as
>> + part of a device tree overlay corresponding to flashed firmware.
>> +
>> + nvidia,bl-data-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + Offset to bootloader data section from beginning of firmware. Typically set as
>> + part of a device tree overlay corresponding to flashed firmware.
>> +
>> + nvidia,os-manifest-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + Offset to operating system manifest from beginning of firmware. Typically set as
>> + part of a device tree overlay corresponding to flashed firmware.
>> +
>> + nvidia,os-code-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + Offset to operating system code section from beginning of firmware. Typically set as
>> + part of a device tree overlay corresponding to flashed firmware.
>> +
>> + nvidia,os-data-offset:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + Offset to operating system data section from beginning of firmware. Typically set as
>> + part of a device tree overlay corresponding to flashed firmware.
>
> I don't think DT is the place for describing your runtime loaded
> firmware layout.
>
> Rob

The way I see it, from the kernel's point of view it's not runtime
loaded but a contract with the bootloader. Bootloader sets up hardware
in a certain way the kernel doesn't otherwise know so the bootloader
needs to tell the kernel how the hardware is set up.

The fact that the information is supplied through an overlay is
accidental -- equivalently the bootloader that sets up the firmware
could adjust the device tree like we do in other situations, but in this
case an overlay is an easier implementation method.

Thanks,
Mikko

2022-09-14 15:07:41

by Thierry Reding

[permalink] [raw]
Subject: Re: [PATCH v2 3/8] dt-bindings: Add bindings for Tegra234 NVDEC

On Wed, Sep 14, 2022 at 03:19:01PM +0300, Mikko Perttunen wrote:
> On 9/14/22 15:08, Rob Herring wrote:
> > On Tue, Sep 13, 2022 at 04:14:41PM +0300, Mikko Perttunen wrote:
> > > From: Mikko Perttunen <[email protected]>
> > >
> > > Update NVDEC bindings for Tegra234. This new engine version only has
> > > two memory clients, but now requires three clocks, and as a bigger
> > > change the engine loads firmware from a secure carveout configured by
> > > the bootloader.
> > >
> > > For the latter, we need to add a phandle to the memory controller
> > > to query the location of this carveout, and several other properties
> > > containing offsets into the firmware inside the carveout. These
> > > properties are intended to be populated through a device tree overlay
> > > configured at flashing time, so that the values correspond to the
> > > flashed NVDEC firmware.
> > >
> > > As the binding was getting large with many conditional properties,
> > > also split the Tegra234 version out into a separate file.
> > >
> > > Signed-off-by: Mikko Perttunen <[email protected]>
> > > ---
> > > v2:
> > > - Split out into separate file to avoid complexity with
> > > conditionals etc.
> > > ---
> > > .../gpu/host1x/nvidia,tegra234-nvdec.yaml | 154 ++++++++++++++++++
> > > 1 file changed, 154 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
> > > new file mode 100644
> > > index 000000000000..eab0475ca983
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
> > > @@ -0,0 +1,154 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/gpu/host1x/nvidia,tegra234-nvdec.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Device tree binding for NVIDIA Tegra234 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 <[email protected]>
> > > + - Mikko Perttunen <[email protected]>
> > > +
> > > +properties:
> > > + $nodename:
> > > + pattern: "^nvdec@[0-9a-f]*$"
> > > +
> > > + compatible:
> > > + enum:
> > > + - nvidia,tegra234-nvdec
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + clocks:
> > > + maxItems: 3
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: nvdec
> > > + - const: fuse
> > > + - const: tsec_pka
> > > +
> > > + resets:
> > > + maxItems: 1
> > > +
> > > + reset-names:
> > > + items:
> > > + - const: nvdec
> > > +
> > > + power-domains:
> > > + maxItems: 1
> > > +
> > > + iommus:
> > > + maxItems: 1
> > > +
> > > + dma-coherent: true
> > > +
> > > + interconnects:
> > > + items:
> > > + - description: DMA read memory client
> > > + - description: DMA write memory client
> > > +
> > > + interconnect-names:
> > > + items:
> > > + - const: dma-mem
> > > + - const: write
> > > +
> > > + nvidia,memory-controller:
> > > + $ref: /schemas/types.yaml#/definitions/phandle
> > > + description:
> > > + phandle to the memory controller for determining carveout information.
> > > +
> > > + nvidia,bl-manifest-offset:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description:
> > > + Offset to bootloader manifest from beginning of firmware. Typically set as
> > > + part of a device tree overlay corresponding to flashed firmware.
> > > +
> > > + nvidia,bl-code-offset:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description:
> > > + Offset to bootloader code section from beginning of firmware. Typically set as
> > > + part of a device tree overlay corresponding to flashed firmware.
> > > +
> > > + nvidia,bl-data-offset:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description:
> > > + Offset to bootloader data section from beginning of firmware. Typically set as
> > > + part of a device tree overlay corresponding to flashed firmware.
> > > +
> > > + nvidia,os-manifest-offset:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description:
> > > + Offset to operating system manifest from beginning of firmware. Typically set as
> > > + part of a device tree overlay corresponding to flashed firmware.
> > > +
> > > + nvidia,os-code-offset:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description:
> > > + Offset to operating system code section from beginning of firmware. Typically set as
> > > + part of a device tree overlay corresponding to flashed firmware.
> > > +
> > > + nvidia,os-data-offset:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description:
> > > + Offset to operating system data section from beginning of firmware. Typically set as
> > > + part of a device tree overlay corresponding to flashed firmware.
> >
> > I don't think DT is the place for describing your runtime loaded
> > firmware layout.
> >
> > Rob
>
> The way I see it, from the kernel's point of view it's not runtime loaded
> but a contract with the bootloader. Bootloader sets up hardware in a certain
> way the kernel doesn't otherwise know so the bootloader needs to tell the
> kernel how the hardware is set up.
>
> The fact that the information is supplied through an overlay is accidental
> -- equivalently the bootloader that sets up the firmware could adjust the
> device tree like we do in other situations, but in this case an overlay is
> an easier implementation method.

I think the key bit of information to know here is that the kernel is
not permitted to load this firmware. It's a bootloader early in the boot
process that sets this up in a secure context and then needs to convey
that information to the kernel.

Perhaps a slightly more idiomatic way to pass this information would be
using a reserved memory node? That could span the entirety of the secure
carveout (therefore removing the need to query the memory controller for
that information) and be identified with a compatible string that would
allow custom properties for these various offsets. Yet another way would
be to have each of the bootloader and OS regions (manifest, code and
data) be their own reserved memory regions. But given that this is one
chunk with different regions inside makes that seem excessive.

Rob, do you have any other ideas how this information could be passed to
the kernel if not via DT?

Thierry


Attachments:
(No filename) (6.92 kB)
signature.asc (849.00 B)
Download all attachments

2022-09-14 15:38:01

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 3/8] dt-bindings: Add bindings for Tegra234 NVDEC

On Wed, Sep 14, 2022 at 9:32 AM Thierry Reding <[email protected]> wrote:
>
> On Wed, Sep 14, 2022 at 03:19:01PM +0300, Mikko Perttunen wrote:
> > On 9/14/22 15:08, Rob Herring wrote:
> > > On Tue, Sep 13, 2022 at 04:14:41PM +0300, Mikko Perttunen wrote:
> > > > From: Mikko Perttunen <[email protected]>
> > > >
> > > > Update NVDEC bindings for Tegra234. This new engine version only has
> > > > two memory clients, but now requires three clocks, and as a bigger
> > > > change the engine loads firmware from a secure carveout configured by
> > > > the bootloader.
> > > >
> > > > For the latter, we need to add a phandle to the memory controller
> > > > to query the location of this carveout, and several other properties
> > > > containing offsets into the firmware inside the carveout. These
> > > > properties are intended to be populated through a device tree overlay
> > > > configured at flashing time, so that the values correspond to the
> > > > flashed NVDEC firmware.
> > > >
> > > > As the binding was getting large with many conditional properties,
> > > > also split the Tegra234 version out into a separate file.
> > > >
> > > > Signed-off-by: Mikko Perttunen <[email protected]>
> > > > ---
> > > > v2:
> > > > - Split out into separate file to avoid complexity with
> > > > conditionals etc.
> > > > ---
> > > > .../gpu/host1x/nvidia,tegra234-nvdec.yaml | 154 ++++++++++++++++++
> > > > 1 file changed, 154 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
> > > > new file mode 100644
> > > > index 000000000000..eab0475ca983
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml
> > > > @@ -0,0 +1,154 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: "http://devicetree.org/schemas/gpu/host1x/nvidia,tegra234-nvdec.yaml#"
> > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > > +
> > > > +title: Device tree binding for NVIDIA Tegra234 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 <[email protected]>
> > > > + - Mikko Perttunen <[email protected]>
> > > > +
> > > > +properties:
> > > > + $nodename:
> > > > + pattern: "^nvdec@[0-9a-f]*$"
> > > > +
> > > > + compatible:
> > > > + enum:
> > > > + - nvidia,tegra234-nvdec
> > > > +
> > > > + reg:
> > > > + maxItems: 1
> > > > +
> > > > + clocks:
> > > > + maxItems: 3
> > > > +
> > > > + clock-names:
> > > > + items:
> > > > + - const: nvdec
> > > > + - const: fuse
> > > > + - const: tsec_pka
> > > > +
> > > > + resets:
> > > > + maxItems: 1
> > > > +
> > > > + reset-names:
> > > > + items:
> > > > + - const: nvdec
> > > > +
> > > > + power-domains:
> > > > + maxItems: 1
> > > > +
> > > > + iommus:
> > > > + maxItems: 1
> > > > +
> > > > + dma-coherent: true
> > > > +
> > > > + interconnects:
> > > > + items:
> > > > + - description: DMA read memory client
> > > > + - description: DMA write memory client
> > > > +
> > > > + interconnect-names:
> > > > + items:
> > > > + - const: dma-mem
> > > > + - const: write
> > > > +
> > > > + nvidia,memory-controller:
> > > > + $ref: /schemas/types.yaml#/definitions/phandle
> > > > + description:
> > > > + phandle to the memory controller for determining carveout information.
> > > > +
> > > > + nvidia,bl-manifest-offset:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + description:
> > > > + Offset to bootloader manifest from beginning of firmware. Typically set as
> > > > + part of a device tree overlay corresponding to flashed firmware.
> > > > +
> > > > + nvidia,bl-code-offset:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + description:
> > > > + Offset to bootloader code section from beginning of firmware. Typically set as
> > > > + part of a device tree overlay corresponding to flashed firmware.
> > > > +
> > > > + nvidia,bl-data-offset:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + description:
> > > > + Offset to bootloader data section from beginning of firmware. Typically set as
> > > > + part of a device tree overlay corresponding to flashed firmware.
> > > > +
> > > > + nvidia,os-manifest-offset:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + description:
> > > > + Offset to operating system manifest from beginning of firmware. Typically set as
> > > > + part of a device tree overlay corresponding to flashed firmware.
> > > > +
> > > > + nvidia,os-code-offset:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + description:
> > > > + Offset to operating system code section from beginning of firmware. Typically set as
> > > > + part of a device tree overlay corresponding to flashed firmware.
> > > > +
> > > > + nvidia,os-data-offset:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + description:
> > > > + Offset to operating system data section from beginning of firmware. Typically set as
> > > > + part of a device tree overlay corresponding to flashed firmware.
> > >
> > > I don't think DT is the place for describing your runtime loaded
> > > firmware layout.
> > >
> > > Rob
> >
> > The way I see it, from the kernel's point of view it's not runtime loaded
> > but a contract with the bootloader. Bootloader sets up hardware in a certain
> > way the kernel doesn't otherwise know so the bootloader needs to tell the
> > kernel how the hardware is set up.
> >
> > The fact that the information is supplied through an overlay is accidental
> > -- equivalently the bootloader that sets up the firmware could adjust the
> > device tree like we do in other situations, but in this case an overlay is
> > an easier implementation method.
>
> I think the key bit of information to know here is that the kernel is
> not permitted to load this firmware. It's a bootloader early in the boot
> process that sets this up in a secure context and then needs to convey
> that information to the kernel.
>
> Perhaps a slightly more idiomatic way to pass this information would be
> using a reserved memory node? That could span the entirety of the secure
> carveout (therefore removing the need to query the memory controller for
> that information) and be identified with a compatible string that would
> allow custom properties for these various offsets. Yet another way would
> be to have each of the bootloader and OS regions (manifest, code and
> data) be their own reserved memory regions. But given that this is one
> chunk with different regions inside makes that seem excessive.
>
> Rob, do you have any other ideas how this information could be passed to
> the kernel if not via DT?

Just update the description summarizing the above removing the overlay
aspect as when they are set is more important than how.

Rob