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
