________________________________________
From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
Sent: 08 May 2023 17:04
To: Peter De Schrijver; thierry.reding@xxxxxxxxx; Jonathan Hunter
Cc: robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-tegra@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Stefan Kristiansson
Subject: Re: [PATCH 4/5] dt-bindings: Add bindings to support DRAM MRQ GSCs
On 08/05/2023 14:20, Peter De Schrijver wrote:
> Add bindings for DRAM MRQ GSC support.
>
> Co-developed-by: Stefan Kristiansson <stefank@xxxxxxxxxx>
> Signed-off-by: Stefan Kristiansson <stefank@xxxxxxxxxx>
> Signed-off-by: Peter De Schrijver <pdeschrijver@xxxxxxxxxx>
> ---
> .../firmware/nvidia,tegra186-bpmp.yaml | 69 ++++++++++++++++++-
> .../nvidia,tegra264-bpmp-shmem.yaml | 40 +++++++++++
> Why touching two files?
Because both are needed to support having MRQ GSCs in DRAM.
> 2 files changed, 106 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
>
> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml
> index 833c07f1685c..d818cfe1d783 100644
> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml
> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml
> @@ -57,8 +57,11 @@ description: |
> "#address-cells" or "#size-cells" property.
>
> The shared memory area for the IPC TX and RX between CPU and BPMP are
> - predefined and work on top of sysram, which is an SRAM inside the
> - chip. See ".../sram/sram.yaml" for the bindings.
> + predefined and work on top of either sysram, which is an SRAM inside the
> + chip, or in normal SDRAM.
> + See ".../sram/sram.yaml" for the bindings for the SRAM case.
> + See "../reserved-memory/nvidia,tegra264-bpmp-shmem.yaml" for bindings for
> + the SDRAM case.
>
> properties:
> compatible:
> @@ -81,6 +84,11 @@ properties:
> minItems: 2
> maxItems: 2
>
> + memory-region:
> + description: phandle to reserved memory region used for IPC between
> + CPU-NS and BPMP.
> + maxItems: 1
> +
> "#clock-cells":
> const: 1
>
> @@ -115,10 +123,16 @@ properties:
>
> additionalProperties: false
>
> +allOf:
> + - oneOf:
Keep just oneOf and drop allOf.
> + - required:
> + - memory-region
> + - required:
> + - shmem
> +
> required:
> - compatible
> - mboxes
> - - shmem
> - "#clock-cells"
> - "#power-domain-cells"
> - "#reset-cells"
> @@ -184,3 +198,52 @@ examples:
> #thermal-sensor-cells = <1>;
> };
> };
> +
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/mailbox/tegra186-hsp.h>
> + #include <dt-bindings/memory/tegra186-mc.h>
> +
> + hsp_top0: hsp@3c00000 {
> + compatible = "nvidia,tegra186-hsp";
> + reg = <0x03c00000 0xa0000>;
> + interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "doorbell";
> + #mbox-cells = <2>;
> Why HSP example is here?
Because it's referred to further down the example.
> + };
> +
> + reserved-memory {
> + dram_cpu_bpmp_mail: shmem@f1be0000 {
> + compatible = "nvidia,tegra264-bpmp-shmem";
> + reg = <0x0 0xf1be0000 0x0 0x2000>;
> + no-map;
> + };
> + };
Drop, fairly obvious and should be in that binding, not here.
> +
> + bpmp {
> + compatible = "nvidia,tegra186-bpmp";
> + interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc>,
> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>,
> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc>,
> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>;
> + interconnect-names = "read", "write", "dma-mem", "dma-write";
> + iommus = <&smmu TEGRA186_SID_BPMP>;
> + mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB
^^^ refers to hsp_top0.
> + TEGRA_HSP_DB_MASTER_BPMP>;
> + memory-region = <&dram_cpu_bpmp_mail>;
> + #clock-cells = <1>;
> + #power-domain-cells = <1>;
> + #reset-cells = <1>;
> +
> + i2c {
> + compatible = "nvidia,tegra186-bpmp-i2c";
> + nvidia,bpmp-bus-id = <5>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + thermal {
> + compatible = "nvidia,tegra186-bpmp-thermal";
> + #thermal-sensor-cells = <1>;
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml b/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
> new file mode 100644
> index 000000000000..6cd9a61cd31f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Tegra CPU-NS - BPMP IPC reserved memory binding
Drop "binding"
> +
> +maintainers:
> + - Peter De Schrijver <pdeschrijver@xxxxxxxxxx>
> +
> +description: |
> + Define a memory region used for communication between CPU-NS and BPMP.
> + Typically this node is created by the bootloader as the physical address
> + has to be known to both CPU-NS and BPMP for correct IPC operation.
> + The memory region is defined using a child node under /reserved-memory.
> + The sub-node is named shmem@<address>.
> +
> Open other files there and implement it similar way. I really wonder why
> this should be done differently than for example other nvidia stuff -
> without reserved-memory schema?
Because up to now, the GSC was kept in sysram which is considered to be a device by the kernel. Now part of the DRAM will be reserved for this, so the kernel needs to know about it.
> +properties:
> + compatible:
> + const: nvidia,tegra264-bpmp-shmem
> +
> + reg:
> + description: The physical address and size of the shared SDRAM region
> +
> +required:
> + - compatible
> + - reg
> + - no-map
> +
Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
> +examples:
> + - |
> + reserved-memory {
> + dram_cpu_bpmp_mail: shmem@f1be0000 {
> + compatible = "nvidia,tegra264-bpmp-shmem";
> + reg = <0x0 0xf1be0000 0x0 0x2000>;
> + no-map;
> + };
> + };
> +...
Best regards,
Krzysztof
