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

[Rob Herring]

On Wed, Sep 14, 2022 at 9:32 AM Thierry Reding <thierry.reding@xxxxxxxxx> 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 <mperttunen@xxxxxxxxxx>
> > > >
> > > > 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 <mperttunen@xxxxxxxxxx>
> > > > ---
> > > > 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 <treding@xxxxxxxxx>
> > > > +  - Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > > > +
> > > > +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