* [PATCH 1/2] dt-bindings: firmware: Convert Tegra186 BPMP bindings to json-schema @ 2020-01-14 18:15 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-14 18:15 UTC (permalink / raw) To: Rob Herring, Georgi Djakov Cc: Thierry Reding, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> Convert the Tegra186 BPMP bindings from the old free-form text format to a json-schema and fix things up so that existing device trees properly validate. Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> --- .../firmware/nvidia,tegra186-bpmp.txt | 107 --------- .../firmware/nvidia,tegra186-bpmp.yaml | 209 ++++++++++++++++++ 2 files changed, 209 insertions(+), 107 deletions(-) delete mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt create mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt deleted file mode 100644 index e44a13bc06ed..000000000000 --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt +++ /dev/null @@ -1,107 +0,0 @@ -NVIDIA Tegra Boot and Power Management Processor (BPMP) - -The BPMP is a specific processor in Tegra chip, which is designed for -booting process handling and offloading the power management, clock -management, and reset control tasks from the CPU. The binding document -defines the resources that would be used by the BPMP firmware driver, -which can create the interprocessor communication (IPC) between the CPU -and BPMP. - -Required properties: -- compatible - Array of strings - One of: - - "nvidia,tegra186-bpmp" -- mboxes : The phandle of mailbox controller and the mailbox specifier. -- shmem : List of the phandle of the TX and RX shared memory area that - the IPC between CPU and BPMP is based on. -- #clock-cells : Should be 1. -- #power-domain-cells : Should be 1. -- #reset-cells : Should be 1. - -This node is a mailbox consumer. See the following files for details of -the mailbox subsystem, and the specifiers implemented by the relevant -provider(s): - -- .../mailbox/mailbox.txt -- .../mailbox/nvidia,tegra186-hsp.txt - -This node is a clock, power domain, and reset provider. See the following -files for general documentation of those features, and the specifiers -implemented by this node: - -- .../clock/clock-bindings.txt -- <dt-bindings/clock/tegra186-clock.h> -- ../power/power-domain.yaml -- <dt-bindings/power/tegra186-powergate.h> -- .../reset/reset.txt -- <dt-bindings/reset/tegra186-reset.h> - -The BPMP implements some services which must be represented by separate nodes. -For example, it can provide access to certain I2C controllers, and the I2C -bindings represent each I2C controller as a device tree node. Such nodes should -be nested directly inside the main BPMP node. - -Software can determine whether a child node of the BPMP node represents a device -by checking for a compatible property. Any node with a compatible property -represents a device that can be instantiated. Nodes without a compatible -property may be used to provide configuration information regarding the BPMP -itself, although no such configuration nodes are currently defined by this -binding. - -The BPMP firmware defines no single global name-/numbering-space for such -services. Put another way, the numbering scheme for I2C buses is distinct from -the numbering scheme for any other service the BPMP may provide (e.g. a future -hypothetical SPI bus service). As such, child device nodes will have no reg -property, and the BPMP node will have no #address-cells or #size-cells property. - -The shared memory bindings for BPMP ------------------------------------ - -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.txt" for the bindings. - -Example: - -hsp_top0: hsp@3c00000 { - ... - #mbox-cells = <2>; -}; - -sysram@30000000 { - compatible = "nvidia,tegra186-sysram", "mmio-sram"; - reg = <0x0 0x30000000 0x0 0x50000>; - #address-cells = <2>; - #size-cells = <2>; - ranges = <0 0x0 0x0 0x30000000 0x0 0x50000>; - - cpu_bpmp_tx: shmem@4e000 { - compatible = "nvidia,tegra186-bpmp-shmem"; - reg = <0x0 0x4e000 0x0 0x1000>; - label = "cpu-bpmp-tx"; - pool; - }; - - cpu_bpmp_rx: shmem@4f000 { - compatible = "nvidia,tegra186-bpmp-shmem"; - reg = <0x0 0x4f000 0x0 0x1000>; - label = "cpu-bpmp-rx"; - pool; - }; -}; - -bpmp { - compatible = "nvidia,tegra186-bpmp"; - mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; - shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; - #clock-cells = <1>; - #power-domain-cells = <1>; - #reset-cells = <1>; - - i2c { - compatible = "..."; - ... - }; -}; diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml new file mode 100644 index 000000000000..dabf1c1aec2f --- /dev/null +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml @@ -0,0 +1,209 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/firmware/nvidia,tegra186-bpmp.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NVIDIA Tegra186 (and later) Boot and Power Management Processor (BPMP) + +maintainers: + - Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> + - Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> + +description: | + The BPMP is a specific processor in Tegra chip, which is designed for + booting process handling and offloading the power management, clock + management, and reset control tasks from the CPU. The binding document + defines the resources that would be used by the BPMP firmware driver, + which can create the interprocessor communication (IPC) between the + CPU and BPMP. + + The BPMP implements some services which must be represented by separate + nodes. For example, it can provide access to certain I2C controllers, and + the I2C bindings represent each I2C controller as a device tree node. Such + nodes should be nested directly inside the main BPMP node. + + Software can determine whether a child node of the BPMP node represents a + device by checking for a compatible property. Any node with a compatible + property represents a device that can be instantiated. Nodes without a + compatible property may be used to provide configuration information + regarding the BPMP itself, although no such configuration nodes are + currently defined by this binding. + + The BPMP firmware defines no single global name-/numbering-space for such + services. Put another way, the numbering scheme for I2C buses is distinct + from the numbering scheme for any other service the BPMP may provide (e.g. + a future hypothetical SPI bus service). As such, child device nodes will + have no "reg" property, and the BPMP node will have no "#address-cells" or + "#size-cells" property. + +properties: + compatible: + items: + - enum: + - nvidia,tegra186-bpmp + + iommus: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: | + The phandle of the IOMMU and the IOMMU specifier. See ../iommu/iommu.txt + for details. + + mboxes: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: | + The phandle of the mailbox controller and the mailbox specifier. See + ../mailbox/mailbox.txt and ../mailbox/nvidia,tegra186-hsp.txt for + details. + + shmem: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: List of phandles for the TX and RX shared memory areas used + for interprocess communication between the CPU and the BPMP. + + 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. + + "#clock-cells": + const: 1 + + "#power-domain-cells": + const: 1 + + "#reset-cells": + const: 1 + + i2c: + type: object + description: | + The BPMP can provide serialized access to I2C controllers that have + been assigned to it. + + properties: + compatible: + items: + - enum: + - nvidia,tegra186-bpmp-i2c + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + nvidia,bpmp-bus-id: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The bus ID of the I2C controller. + + required: + - compatible + - "#address-cells" + - "#size-cells" + - nvidia,bpmp-bus-id + + patternProperties: + "^.*@[0-9a-f]+$": + type: object + description: I2C slave + properties: + reg: + maxItems: 1 + description: I2C address of the slave + + required: + - reg + + additionalProperties: false + + thermal: + type: object + description: + The BPMP provides functionality that exposes system temperature sensors + and which can be used to trigger a system shutdown if the temperature + for a given zone exceeds the specified thresholds. + + properties: + compatible: + items: + - enum: + - nvidia,tegra186-bpmp-thermal + + "#thermal-sensor-cells": + description: The ID of the thermal zone. + const: 1 + + required: + - compatible + - "#thermal-sensor-cells" + + additionalProperties: false + +required: + - compatible + - mboxes + - shmem + - "#clock-cells" + - "#power-domain-cells" + - "#reset-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/mailbox/tegra186-hsp.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>; + }; + + sysram@30000000 { + compatible = "nvidia,tegra186-sysram", "mmio-sram"; + reg = <0x30000000 0x50000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0x0 0x30000000 0x50000>; + + cpu_bpmp_tx: shmem@4e000 { + compatible = "nvidia,tegra186-bpmp-shmem"; + reg = <0x4e000 0x1000>; + label = "cpu-bpmp-tx"; + pool; + }; + + cpu_bpmp_rx: shmem@4f000 { + compatible = "nvidia,tegra186-bpmp-shmem"; + reg = <0x4f000 0x1000>; + label = "cpu-bpmp-rx"; + pool; + }; + }; + + bpmp { + compatible = "nvidia,tegra186-bpmp"; + iommus = <&smmu TEGRA186_SID_BPMP>; + mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; + shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; + #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>; + status = "disabled"; + }; + + thermal { + compatible = "nvidia,tegra186-bpmp-thermal"; + #thermal-sensor-cells = <1>; + }; + }; -- 2.24.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 1/2] dt-bindings: firmware: Convert Tegra186 BPMP bindings to json-schema @ 2020-01-14 18:15 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-14 18:15 UTC (permalink / raw) To: Rob Herring, Georgi Djakov Cc: Thierry Reding, Jon Hunter, linux-tegra, devicetree From: Thierry Reding <treding@nvidia.com> Convert the Tegra186 BPMP bindings from the old free-form text format to a json-schema and fix things up so that existing device trees properly validate. Signed-off-by: Thierry Reding <treding@nvidia.com> --- .../firmware/nvidia,tegra186-bpmp.txt | 107 --------- .../firmware/nvidia,tegra186-bpmp.yaml | 209 ++++++++++++++++++ 2 files changed, 209 insertions(+), 107 deletions(-) delete mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt create mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt deleted file mode 100644 index e44a13bc06ed..000000000000 --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt +++ /dev/null @@ -1,107 +0,0 @@ -NVIDIA Tegra Boot and Power Management Processor (BPMP) - -The BPMP is a specific processor in Tegra chip, which is designed for -booting process handling and offloading the power management, clock -management, and reset control tasks from the CPU. The binding document -defines the resources that would be used by the BPMP firmware driver, -which can create the interprocessor communication (IPC) between the CPU -and BPMP. - -Required properties: -- compatible - Array of strings - One of: - - "nvidia,tegra186-bpmp" -- mboxes : The phandle of mailbox controller and the mailbox specifier. -- shmem : List of the phandle of the TX and RX shared memory area that - the IPC between CPU and BPMP is based on. -- #clock-cells : Should be 1. -- #power-domain-cells : Should be 1. -- #reset-cells : Should be 1. - -This node is a mailbox consumer. See the following files for details of -the mailbox subsystem, and the specifiers implemented by the relevant -provider(s): - -- .../mailbox/mailbox.txt -- .../mailbox/nvidia,tegra186-hsp.txt - -This node is a clock, power domain, and reset provider. See the following -files for general documentation of those features, and the specifiers -implemented by this node: - -- .../clock/clock-bindings.txt -- <dt-bindings/clock/tegra186-clock.h> -- ../power/power-domain.yaml -- <dt-bindings/power/tegra186-powergate.h> -- .../reset/reset.txt -- <dt-bindings/reset/tegra186-reset.h> - -The BPMP implements some services which must be represented by separate nodes. -For example, it can provide access to certain I2C controllers, and the I2C -bindings represent each I2C controller as a device tree node. Such nodes should -be nested directly inside the main BPMP node. - -Software can determine whether a child node of the BPMP node represents a device -by checking for a compatible property. Any node with a compatible property -represents a device that can be instantiated. Nodes without a compatible -property may be used to provide configuration information regarding the BPMP -itself, although no such configuration nodes are currently defined by this -binding. - -The BPMP firmware defines no single global name-/numbering-space for such -services. Put another way, the numbering scheme for I2C buses is distinct from -the numbering scheme for any other service the BPMP may provide (e.g. a future -hypothetical SPI bus service). As such, child device nodes will have no reg -property, and the BPMP node will have no #address-cells or #size-cells property. - -The shared memory bindings for BPMP ------------------------------------ - -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.txt" for the bindings. - -Example: - -hsp_top0: hsp@3c00000 { - ... - #mbox-cells = <2>; -}; - -sysram@30000000 { - compatible = "nvidia,tegra186-sysram", "mmio-sram"; - reg = <0x0 0x30000000 0x0 0x50000>; - #address-cells = <2>; - #size-cells = <2>; - ranges = <0 0x0 0x0 0x30000000 0x0 0x50000>; - - cpu_bpmp_tx: shmem@4e000 { - compatible = "nvidia,tegra186-bpmp-shmem"; - reg = <0x0 0x4e000 0x0 0x1000>; - label = "cpu-bpmp-tx"; - pool; - }; - - cpu_bpmp_rx: shmem@4f000 { - compatible = "nvidia,tegra186-bpmp-shmem"; - reg = <0x0 0x4f000 0x0 0x1000>; - label = "cpu-bpmp-rx"; - pool; - }; -}; - -bpmp { - compatible = "nvidia,tegra186-bpmp"; - mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; - shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; - #clock-cells = <1>; - #power-domain-cells = <1>; - #reset-cells = <1>; - - i2c { - compatible = "..."; - ... - }; -}; diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml new file mode 100644 index 000000000000..dabf1c1aec2f --- /dev/null +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml @@ -0,0 +1,209 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/firmware/nvidia,tegra186-bpmp.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NVIDIA Tegra186 (and later) Boot and Power Management Processor (BPMP) + +maintainers: + - Thierry Reding <thierry.reding@gmail.com> + - Jon Hunter <jonathanh@nvidia.com> + +description: | + The BPMP is a specific processor in Tegra chip, which is designed for + booting process handling and offloading the power management, clock + management, and reset control tasks from the CPU. The binding document + defines the resources that would be used by the BPMP firmware driver, + which can create the interprocessor communication (IPC) between the + CPU and BPMP. + + The BPMP implements some services which must be represented by separate + nodes. For example, it can provide access to certain I2C controllers, and + the I2C bindings represent each I2C controller as a device tree node. Such + nodes should be nested directly inside the main BPMP node. + + Software can determine whether a child node of the BPMP node represents a + device by checking for a compatible property. Any node with a compatible + property represents a device that can be instantiated. Nodes without a + compatible property may be used to provide configuration information + regarding the BPMP itself, although no such configuration nodes are + currently defined by this binding. + + The BPMP firmware defines no single global name-/numbering-space for such + services. Put another way, the numbering scheme for I2C buses is distinct + from the numbering scheme for any other service the BPMP may provide (e.g. + a future hypothetical SPI bus service). As such, child device nodes will + have no "reg" property, and the BPMP node will have no "#address-cells" or + "#size-cells" property. + +properties: + compatible: + items: + - enum: + - nvidia,tegra186-bpmp + + iommus: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: | + The phandle of the IOMMU and the IOMMU specifier. See ../iommu/iommu.txt + for details. + + mboxes: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: | + The phandle of the mailbox controller and the mailbox specifier. See + ../mailbox/mailbox.txt and ../mailbox/nvidia,tegra186-hsp.txt for + details. + + shmem: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: List of phandles for the TX and RX shared memory areas used + for interprocess communication between the CPU and the BPMP. + + 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. + + "#clock-cells": + const: 1 + + "#power-domain-cells": + const: 1 + + "#reset-cells": + const: 1 + + i2c: + type: object + description: | + The BPMP can provide serialized access to I2C controllers that have + been assigned to it. + + properties: + compatible: + items: + - enum: + - nvidia,tegra186-bpmp-i2c + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + nvidia,bpmp-bus-id: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The bus ID of the I2C controller. + + required: + - compatible + - "#address-cells" + - "#size-cells" + - nvidia,bpmp-bus-id + + patternProperties: + "^.*@[0-9a-f]+$": + type: object + description: I2C slave + properties: + reg: + maxItems: 1 + description: I2C address of the slave + + required: + - reg + + additionalProperties: false + + thermal: + type: object + description: + The BPMP provides functionality that exposes system temperature sensors + and which can be used to trigger a system shutdown if the temperature + for a given zone exceeds the specified thresholds. + + properties: + compatible: + items: + - enum: + - nvidia,tegra186-bpmp-thermal + + "#thermal-sensor-cells": + description: The ID of the thermal zone. + const: 1 + + required: + - compatible + - "#thermal-sensor-cells" + + additionalProperties: false + +required: + - compatible + - mboxes + - shmem + - "#clock-cells" + - "#power-domain-cells" + - "#reset-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/mailbox/tegra186-hsp.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>; + }; + + sysram@30000000 { + compatible = "nvidia,tegra186-sysram", "mmio-sram"; + reg = <0x30000000 0x50000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0x0 0x30000000 0x50000>; + + cpu_bpmp_tx: shmem@4e000 { + compatible = "nvidia,tegra186-bpmp-shmem"; + reg = <0x4e000 0x1000>; + label = "cpu-bpmp-tx"; + pool; + }; + + cpu_bpmp_rx: shmem@4f000 { + compatible = "nvidia,tegra186-bpmp-shmem"; + reg = <0x4f000 0x1000>; + label = "cpu-bpmp-rx"; + pool; + }; + }; + + bpmp { + compatible = "nvidia,tegra186-bpmp"; + iommus = <&smmu TEGRA186_SID_BPMP>; + mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; + shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; + #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>; + status = "disabled"; + }; + + thermal { + compatible = "nvidia,tegra186-bpmp-thermal"; + #thermal-sensor-cells = <1>; + }; + }; -- 2.24.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
[parent not found: <20200114181519.3402385-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-14 18:15 ` Thierry Reding @ 2020-01-14 18:15 ` Thierry Reding -1 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-14 18:15 UTC (permalink / raw) To: Rob Herring, Georgi Djakov Cc: Thierry Reding, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> Document the interconnects property that is used to describe the paths from and to system memory from and to the BPMP. Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> --- Rob, Georgi, after the initial RFC that I did for adding interconnect properties on Tegra, I realized that the description wasn't complete. This is an attempt at a more accurate description, but unfortunately I'm not sure if it's even correct in terms of the interconnect bindings. The problem here is that on Tegra, each device has multiple paths to system memory, and I have no good idea on what to pick as the default. They are all basically the same path, but each provides extra controls to configure the "interconnect". Any ideas on how to resolve this? Let me know if the DT bindings and example don't make things clear enough. Thierry .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml index dabf1c1aec2f..d40fcd836e90 100644 --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml @@ -43,6 +43,24 @@ properties: - enum: - nvidia,tegra186-bpmp + interconnects: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: A list of phandle and specifier pairs that describe the + interconnect paths to and from the BPMP. + + interconnect-names: + $ref: /schemas/types.yaml#/definitions/non-unique-string-array + description: One string for each pair of phandle and specifier in the + "interconnects" property. + # XXX We need at least one of these to be named dma-mem so that the core + # will set the DMA mask based on the DMA parent, but all of these go to + # system memory eventually. + items: + - const: dma-mem + - const: dma-mem + - const: dma-mem + - const: dma-mem + iommus: $ref: /schemas/types.yaml#/definitions/phandle-array description: | @@ -152,8 +170,43 @@ additionalProperties: false examples: - | + #include <dt-bindings/clock/tegra186-clock.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/mailbox/tegra186-hsp.h> + #include <dt-bindings/memory/tegra186-mc.h> + + mc: memory-controller@2c00000 { + compatible = "nvidia,tegra186-mc"; + reg = <0x02c00000 0xb0000>; + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + + #interconnect-cells = <1>; + #address-cells = <2>; + #size-cells = <2>; + + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; + + /* + * Memory clients have access to all 40 bits that the memory + * controller can address. + */ + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; + + #memory-controller-cells = <0>; + + emc: external-memory-controller@2c60000 { + compatible = "nvidia,tegra186-emc"; + reg = <0x0 0x02c60000 0x0 0x50000>; + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&bpmp TEGRA186_CLK_EMC>; + clock-names = "emc"; + + #interconnect-cells = <0>; + + nvidia,bpmp = <&bpmp>; + }; + }; hsp_top0: hsp@3c00000 { compatible = "nvidia,tegra186-hsp"; @@ -187,6 +240,12 @@ examples: bpmp { compatible = "nvidia,tegra186-bpmp"; + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; + iommus = <&smmu TEGRA186_SID_BPMP>; mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; -- 2.24.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-14 18:15 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-14 18:15 UTC (permalink / raw) To: Rob Herring, Georgi Djakov Cc: Thierry Reding, Jon Hunter, linux-tegra, devicetree From: Thierry Reding <treding@nvidia.com> Document the interconnects property that is used to describe the paths from and to system memory from and to the BPMP. Signed-off-by: Thierry Reding <treding@nvidia.com> --- Rob, Georgi, after the initial RFC that I did for adding interconnect properties on Tegra, I realized that the description wasn't complete. This is an attempt at a more accurate description, but unfortunately I'm not sure if it's even correct in terms of the interconnect bindings. The problem here is that on Tegra, each device has multiple paths to system memory, and I have no good idea on what to pick as the default. They are all basically the same path, but each provides extra controls to configure the "interconnect". Any ideas on how to resolve this? Let me know if the DT bindings and example don't make things clear enough. Thierry .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml index dabf1c1aec2f..d40fcd836e90 100644 --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml @@ -43,6 +43,24 @@ properties: - enum: - nvidia,tegra186-bpmp + interconnects: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: A list of phandle and specifier pairs that describe the + interconnect paths to and from the BPMP. + + interconnect-names: + $ref: /schemas/types.yaml#/definitions/non-unique-string-array + description: One string for each pair of phandle and specifier in the + "interconnects" property. + # XXX We need at least one of these to be named dma-mem so that the core + # will set the DMA mask based on the DMA parent, but all of these go to + # system memory eventually. + items: + - const: dma-mem + - const: dma-mem + - const: dma-mem + - const: dma-mem + iommus: $ref: /schemas/types.yaml#/definitions/phandle-array description: | @@ -152,8 +170,43 @@ additionalProperties: false examples: - | + #include <dt-bindings/clock/tegra186-clock.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/mailbox/tegra186-hsp.h> + #include <dt-bindings/memory/tegra186-mc.h> + + mc: memory-controller@2c00000 { + compatible = "nvidia,tegra186-mc"; + reg = <0x02c00000 0xb0000>; + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + + #interconnect-cells = <1>; + #address-cells = <2>; + #size-cells = <2>; + + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; + + /* + * Memory clients have access to all 40 bits that the memory + * controller can address. + */ + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; + + #memory-controller-cells = <0>; + + emc: external-memory-controller@2c60000 { + compatible = "nvidia,tegra186-emc"; + reg = <0x0 0x02c60000 0x0 0x50000>; + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&bpmp TEGRA186_CLK_EMC>; + clock-names = "emc"; + + #interconnect-cells = <0>; + + nvidia,bpmp = <&bpmp>; + }; + }; hsp_top0: hsp@3c00000 { compatible = "nvidia,tegra186-hsp"; @@ -187,6 +240,12 @@ examples: bpmp { compatible = "nvidia,tegra186-bpmp"; + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; + iommus = <&smmu TEGRA186_SID_BPMP>; mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; -- 2.24.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
[parent not found: <20200114181519.3402385-2-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-14 18:15 ` Thierry Reding @ 2020-01-17 15:23 ` Georgi Djakov -1 siblings, 0 replies; 42+ messages in thread From: Georgi Djakov @ 2020-01-17 15:23 UTC (permalink / raw) To: Thierry Reding, Rob Herring Cc: Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA Hi Thierry, Thanks for the patch! On 1/14/20 20:15, Thierry Reding wrote: > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > > Document the interconnects property that is used to describe the paths > from and to system memory from and to the BPMP. > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > --- > Rob, Georgi, > > after the initial RFC that I did for adding interconnect properties on > Tegra, I realized that the description wasn't complete. This is an > attempt at a more accurate description, but unfortunately I'm not sure > if it's even correct in terms of the interconnect bindings. > > The problem here is that on Tegra, each device has multiple paths to > system memory, and I have no good idea on what to pick as the default. > They are all basically the same path, but each provides extra controls > to configure the "interconnect". Are these multiple paths between a device and system memory used simultaneously for load-balancing, or who makes the decision about which path would be used? Is this based on the client/stream ID that you mentioned previously? Looking at the the binding below, it seems to me like there are different master/slave pairs between MC and EMC and each link is used for unidirectional traffic only. In terms of the interconnect API, both read and write paths have the same direction. Is the EMC really an interconnect provider or is it just a slave port? Can we scale both EMC and MC independently? Thanks, Georgi > > Any ideas on how to resolve this? Let me know if the DT bindings and > example don't make things clear enough. > > Thierry > > .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > index dabf1c1aec2f..d40fcd836e90 100644 > --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > @@ -43,6 +43,24 @@ properties: > - enum: > - nvidia,tegra186-bpmp > > + interconnects: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: A list of phandle and specifier pairs that describe the > + interconnect paths to and from the BPMP. > + > + interconnect-names: > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > + description: One string for each pair of phandle and specifier in the > + "interconnects" property. > + # XXX We need at least one of these to be named dma-mem so that the core > + # will set the DMA mask based on the DMA parent, but all of these go to > + # system memory eventually. > + items: > + - const: dma-mem > + - const: dma-mem > + - const: dma-mem > + - const: dma-mem > + > iommus: > $ref: /schemas/types.yaml#/definitions/phandle-array > description: | > @@ -152,8 +170,43 @@ additionalProperties: false > > examples: > - | > + #include <dt-bindings/clock/tegra186-clock.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/mailbox/tegra186-hsp.h> > + #include <dt-bindings/memory/tegra186-mc.h> > + > + mc: memory-controller@2c00000 { > + compatible = "nvidia,tegra186-mc"; > + reg = <0x02c00000 0xb0000>; > + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; > + status = "disabled"; > + > + #interconnect-cells = <1>; > + #address-cells = <2>; > + #size-cells = <2>; > + > + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; > + > + /* > + * Memory clients have access to all 40 bits that the memory > + * controller can address. > + */ > + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; > + > + #memory-controller-cells = <0>; > + > + emc: external-memory-controller@2c60000 { > + compatible = "nvidia,tegra186-emc"; > + reg = <0x0 0x02c60000 0x0 0x50000>; > + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&bpmp TEGRA186_CLK_EMC>; > + clock-names = "emc"; > + > + #interconnect-cells = <0>; > + > + nvidia,bpmp = <&bpmp>; > + }; > + }; > > hsp_top0: hsp@3c00000 { > compatible = "nvidia,tegra186-hsp"; > @@ -187,6 +240,12 @@ examples: > > bpmp { > compatible = "nvidia,tegra186-bpmp"; > + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, > + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, > + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; > + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; > + > iommus = <&smmu TEGRA186_SID_BPMP>; > mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; > shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-17 15:23 ` Georgi Djakov 0 siblings, 0 replies; 42+ messages in thread From: Georgi Djakov @ 2020-01-17 15:23 UTC (permalink / raw) To: Thierry Reding, Rob Herring; +Cc: Jon Hunter, linux-tegra, devicetree Hi Thierry, Thanks for the patch! On 1/14/20 20:15, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Document the interconnects property that is used to describe the paths > from and to system memory from and to the BPMP. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Rob, Georgi, > > after the initial RFC that I did for adding interconnect properties on > Tegra, I realized that the description wasn't complete. This is an > attempt at a more accurate description, but unfortunately I'm not sure > if it's even correct in terms of the interconnect bindings. > > The problem here is that on Tegra, each device has multiple paths to > system memory, and I have no good idea on what to pick as the default. > They are all basically the same path, but each provides extra controls > to configure the "interconnect". Are these multiple paths between a device and system memory used simultaneously for load-balancing, or who makes the decision about which path would be used? Is this based on the client/stream ID that you mentioned previously? Looking at the the binding below, it seems to me like there are different master/slave pairs between MC and EMC and each link is used for unidirectional traffic only. In terms of the interconnect API, both read and write paths have the same direction. Is the EMC really an interconnect provider or is it just a slave port? Can we scale both EMC and MC independently? Thanks, Georgi > > Any ideas on how to resolve this? Let me know if the DT bindings and > example don't make things clear enough. > > Thierry > > .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > index dabf1c1aec2f..d40fcd836e90 100644 > --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > @@ -43,6 +43,24 @@ properties: > - enum: > - nvidia,tegra186-bpmp > > + interconnects: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: A list of phandle and specifier pairs that describe the > + interconnect paths to and from the BPMP. > + > + interconnect-names: > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > + description: One string for each pair of phandle and specifier in the > + "interconnects" property. > + # XXX We need at least one of these to be named dma-mem so that the core > + # will set the DMA mask based on the DMA parent, but all of these go to > + # system memory eventually. > + items: > + - const: dma-mem > + - const: dma-mem > + - const: dma-mem > + - const: dma-mem > + > iommus: > $ref: /schemas/types.yaml#/definitions/phandle-array > description: | > @@ -152,8 +170,43 @@ additionalProperties: false > > examples: > - | > + #include <dt-bindings/clock/tegra186-clock.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/mailbox/tegra186-hsp.h> > + #include <dt-bindings/memory/tegra186-mc.h> > + > + mc: memory-controller@2c00000 { > + compatible = "nvidia,tegra186-mc"; > + reg = <0x02c00000 0xb0000>; > + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; > + status = "disabled"; > + > + #interconnect-cells = <1>; > + #address-cells = <2>; > + #size-cells = <2>; > + > + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; > + > + /* > + * Memory clients have access to all 40 bits that the memory > + * controller can address. > + */ > + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; > + > + #memory-controller-cells = <0>; > + > + emc: external-memory-controller@2c60000 { > + compatible = "nvidia,tegra186-emc"; > + reg = <0x0 0x02c60000 0x0 0x50000>; > + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&bpmp TEGRA186_CLK_EMC>; > + clock-names = "emc"; > + > + #interconnect-cells = <0>; > + > + nvidia,bpmp = <&bpmp>; > + }; > + }; > > hsp_top0: hsp@3c00000 { > compatible = "nvidia,tegra186-hsp"; > @@ -187,6 +240,12 @@ examples: > > bpmp { > compatible = "nvidia,tegra186-bpmp"; > + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, > + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, > + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; > + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; > + > iommus = <&smmu TEGRA186_SID_BPMP>; > mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; > shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; > ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <7aefac6c-092c-b5a6-2fa6-e283d2147fc3-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-17 15:23 ` Georgi Djakov @ 2020-01-20 15:06 ` Thierry Reding -1 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-20 15:06 UTC (permalink / raw) To: Georgi Djakov Cc: Rob Herring, Jon Hunter, Dmitry Osipenko, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 7110 bytes --] On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: > Hi Thierry, > > Thanks for the patch! > > On 1/14/20 20:15, Thierry Reding wrote: > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > > > > Document the interconnects property that is used to describe the paths > > from and to system memory from and to the BPMP. > > > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > > --- > > Rob, Georgi, > > > > after the initial RFC that I did for adding interconnect properties on > > Tegra, I realized that the description wasn't complete. This is an > > attempt at a more accurate description, but unfortunately I'm not sure > > if it's even correct in terms of the interconnect bindings. > > > > The problem here is that on Tegra, each device has multiple paths to > > system memory, and I have no good idea on what to pick as the default. > > They are all basically the same path, but each provides extra controls > > to configure the "interconnect". > > Are these multiple paths between a device and system memory used simultaneously > for load-balancing, or who makes the decision about which path would be used? It varies. The vast majority of these paths are read/write pairs, which can be configured separately. There are also cases where multiple paths are used for load-balancing and I don't think there's any direct software control over which path will be used. A third class is where you have one device, but two read/write pairs, one which is tied to a microcontroller that's part of the device, and another read/write pair that is used for DMA to/from the device. Often in the latter case, the microcontroller memory client interfaces will be used by the microcontroller to read firmware and once the micro- controller has booted up, the DMA memory client interfaces will be used to read/write system memory with bulk data (like frame buffers, etc.). > Is this based on the client/stream ID that you mentioned previously? These are now all what's call memory client IDs, which identify the corresponding interface to the memory controller. Stream IDs are slightly higher-level and typically identify the "module" that uses the SMMU. Generally a stream ID is mapped to one or more memory client IDs. > Looking at the the binding below, it seems to me like there are different > master/slave pairs between MC and EMC and each link is used for > unidirectional traffic only. In terms of the interconnect API, both read > and write paths have the same direction. I'm not sure I understand what you mean by this last sentence. Are you saying that each path in terms of the interconnect API is a always a bidirectional link? > Is the EMC really an interconnect provider or is it just a slave port? Can > we scale both EMC and MC independently? The EMC is the only one where we can scale the frequency, but the MC has various knobs that can be used to fine-tune arbitration, set maximum latency, etc. I vaguely recall Dmitry mentioning that the EMC in early generations of Tegra used to have controls for individual memory clients, but I don't see that in more recent generations. Thierry > > Any ideas on how to resolve this? Let me know if the DT bindings and > > example don't make things clear enough. > > > > Thierry > > > > .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > > index dabf1c1aec2f..d40fcd836e90 100644 > > --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > > +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > > @@ -43,6 +43,24 @@ properties: > > - enum: > > - nvidia,tegra186-bpmp > > > > + interconnects: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + description: A list of phandle and specifier pairs that describe the > > + interconnect paths to and from the BPMP. > > + > > + interconnect-names: > > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > > + description: One string for each pair of phandle and specifier in the > > + "interconnects" property. > > + # XXX We need at least one of these to be named dma-mem so that the core > > + # will set the DMA mask based on the DMA parent, but all of these go to > > + # system memory eventually. > > + items: > > + - const: dma-mem > > + - const: dma-mem > > + - const: dma-mem > > + - const: dma-mem > > + > > iommus: > > $ref: /schemas/types.yaml#/definitions/phandle-array > > description: | > > @@ -152,8 +170,43 @@ additionalProperties: false > > > > examples: > > - | > > + #include <dt-bindings/clock/tegra186-clock.h> > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > #include <dt-bindings/mailbox/tegra186-hsp.h> > > + #include <dt-bindings/memory/tegra186-mc.h> > > + > > + mc: memory-controller@2c00000 { > > + compatible = "nvidia,tegra186-mc"; > > + reg = <0x02c00000 0xb0000>; > > + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; > > + status = "disabled"; > > + > > + #interconnect-cells = <1>; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; > > + > > + /* > > + * Memory clients have access to all 40 bits that the memory > > + * controller can address. > > + */ > > + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; > > + > > + #memory-controller-cells = <0>; > > + > > + emc: external-memory-controller@2c60000 { > > + compatible = "nvidia,tegra186-emc"; > > + reg = <0x0 0x02c60000 0x0 0x50000>; > > + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&bpmp TEGRA186_CLK_EMC>; > > + clock-names = "emc"; > > + > > + #interconnect-cells = <0>; > > + > > + nvidia,bpmp = <&bpmp>; > > + }; > > + }; > > > > hsp_top0: hsp@3c00000 { > > compatible = "nvidia,tegra186-hsp"; > > @@ -187,6 +240,12 @@ examples: > > > > bpmp { > > compatible = "nvidia,tegra186-bpmp"; > > + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, > > + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, > > + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > > + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; > > + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; > > + > > iommus = <&smmu TEGRA186_SID_BPMP>; > > mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; > > shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-20 15:06 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-20 15:06 UTC (permalink / raw) To: Georgi Djakov Cc: Rob Herring, Jon Hunter, Dmitry Osipenko, linux-tegra, devicetree [-- Attachment #1: Type: text/plain, Size: 7052 bytes --] On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: > Hi Thierry, > > Thanks for the patch! > > On 1/14/20 20:15, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Document the interconnects property that is used to describe the paths > > from and to system memory from and to the BPMP. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > Rob, Georgi, > > > > after the initial RFC that I did for adding interconnect properties on > > Tegra, I realized that the description wasn't complete. This is an > > attempt at a more accurate description, but unfortunately I'm not sure > > if it's even correct in terms of the interconnect bindings. > > > > The problem here is that on Tegra, each device has multiple paths to > > system memory, and I have no good idea on what to pick as the default. > > They are all basically the same path, but each provides extra controls > > to configure the "interconnect". > > Are these multiple paths between a device and system memory used simultaneously > for load-balancing, or who makes the decision about which path would be used? It varies. The vast majority of these paths are read/write pairs, which can be configured separately. There are also cases where multiple paths are used for load-balancing and I don't think there's any direct software control over which path will be used. A third class is where you have one device, but two read/write pairs, one which is tied to a microcontroller that's part of the device, and another read/write pair that is used for DMA to/from the device. Often in the latter case, the microcontroller memory client interfaces will be used by the microcontroller to read firmware and once the micro- controller has booted up, the DMA memory client interfaces will be used to read/write system memory with bulk data (like frame buffers, etc.). > Is this based on the client/stream ID that you mentioned previously? These are now all what's call memory client IDs, which identify the corresponding interface to the memory controller. Stream IDs are slightly higher-level and typically identify the "module" that uses the SMMU. Generally a stream ID is mapped to one or more memory client IDs. > Looking at the the binding below, it seems to me like there are different > master/slave pairs between MC and EMC and each link is used for > unidirectional traffic only. In terms of the interconnect API, both read > and write paths have the same direction. I'm not sure I understand what you mean by this last sentence. Are you saying that each path in terms of the interconnect API is a always a bidirectional link? > Is the EMC really an interconnect provider or is it just a slave port? Can > we scale both EMC and MC independently? The EMC is the only one where we can scale the frequency, but the MC has various knobs that can be used to fine-tune arbitration, set maximum latency, etc. I vaguely recall Dmitry mentioning that the EMC in early generations of Tegra used to have controls for individual memory clients, but I don't see that in more recent generations. Thierry > > Any ideas on how to resolve this? Let me know if the DT bindings and > > example don't make things clear enough. > > > > Thierry > > > > .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > > index dabf1c1aec2f..d40fcd836e90 100644 > > --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > > +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > > @@ -43,6 +43,24 @@ properties: > > - enum: > > - nvidia,tegra186-bpmp > > > > + interconnects: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + description: A list of phandle and specifier pairs that describe the > > + interconnect paths to and from the BPMP. > > + > > + interconnect-names: > > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > > + description: One string for each pair of phandle and specifier in the > > + "interconnects" property. > > + # XXX We need at least one of these to be named dma-mem so that the core > > + # will set the DMA mask based on the DMA parent, but all of these go to > > + # system memory eventually. > > + items: > > + - const: dma-mem > > + - const: dma-mem > > + - const: dma-mem > > + - const: dma-mem > > + > > iommus: > > $ref: /schemas/types.yaml#/definitions/phandle-array > > description: | > > @@ -152,8 +170,43 @@ additionalProperties: false > > > > examples: > > - | > > + #include <dt-bindings/clock/tegra186-clock.h> > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > #include <dt-bindings/mailbox/tegra186-hsp.h> > > + #include <dt-bindings/memory/tegra186-mc.h> > > + > > + mc: memory-controller@2c00000 { > > + compatible = "nvidia,tegra186-mc"; > > + reg = <0x02c00000 0xb0000>; > > + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; > > + status = "disabled"; > > + > > + #interconnect-cells = <1>; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; > > + > > + /* > > + * Memory clients have access to all 40 bits that the memory > > + * controller can address. > > + */ > > + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; > > + > > + #memory-controller-cells = <0>; > > + > > + emc: external-memory-controller@2c60000 { > > + compatible = "nvidia,tegra186-emc"; > > + reg = <0x0 0x02c60000 0x0 0x50000>; > > + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&bpmp TEGRA186_CLK_EMC>; > > + clock-names = "emc"; > > + > > + #interconnect-cells = <0>; > > + > > + nvidia,bpmp = <&bpmp>; > > + }; > > + }; > > > > hsp_top0: hsp@3c00000 { > > compatible = "nvidia,tegra186-hsp"; > > @@ -187,6 +240,12 @@ examples: > > > > bpmp { > > compatible = "nvidia,tegra186-bpmp"; > > + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, > > + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, > > + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > > + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; > > + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; > > + > > iommus = <&smmu TEGRA186_SID_BPMP>; > > mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; > > shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-20 15:06 ` Thierry Reding @ 2020-01-21 6:53 ` Dmitry Osipenko -1 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-21 6:53 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 20.01.2020 18:06, Thierry Reding пишет: > On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: >> Hi Thierry, >> >> Thanks for the patch! >> >> On 1/14/20 20:15, Thierry Reding wrote: >>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>> >>> Document the interconnects property that is used to describe the paths >>> from and to system memory from and to the BPMP. >>> >>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>> --- >>> Rob, Georgi, >>> >>> after the initial RFC that I did for adding interconnect properties on >>> Tegra, I realized that the description wasn't complete. This is an >>> attempt at a more accurate description, but unfortunately I'm not sure >>> if it's even correct in terms of the interconnect bindings. >>> >>> The problem here is that on Tegra, each device has multiple paths to >>> system memory, and I have no good idea on what to pick as the default. >>> They are all basically the same path, but each provides extra controls >>> to configure the "interconnect". >> >> Are these multiple paths between a device and system memory used simultaneously >> for load-balancing, or who makes the decision about which path would be used? > > It varies. The vast majority of these paths are read/write pairs, which > can be configured separately. There are also cases where multiple paths > are used for load-balancing and I don't think there's any direct > software control over which path will be used. > > A third class is where you have one device, but two read/write pairs, > one which is tied to a microcontroller that's part of the device, and > another read/write pair that is used for DMA to/from the device. > > Often in the latter case, the microcontroller memory client interfaces > will be used by the microcontroller to read firmware and once the micro- > controller has booted up, the DMA memory client interfaces will be used > to read/write system memory with bulk data (like frame buffers, etc.). > >> Is this based on the client/stream ID that you mentioned previously? > > These are now all what's call memory client IDs, which identify the > corresponding interface to the memory controller. Stream IDs are > slightly higher-level and typically identify the "module" that uses > the SMMU. Generally a stream ID is mapped to one or more memory client > IDs. > >> Looking at the the binding below, it seems to me like there are different >> master/slave pairs between MC and EMC and each link is used for >> unidirectional traffic only. In terms of the interconnect API, both read >> and write paths have the same direction. Yes, that definition should be incorrect. > I'm not sure I understand what you mean by this last sentence. Are you > saying that each path in terms of the interconnect API is a always a > bidirectional link? Please see more below. >> Is the EMC really an interconnect provider or is it just a slave port? Can >> we scale both EMC and MC independently? > > The EMC is the only one where we can scale the frequency, but the MC has > various knobs that can be used to fine-tune arbitration, set maximum > latency, etc. Yes.. EMC controls the total amount of available memory bandwidth, things like DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from one side and DRAM (EMEM) from the other. MC controls allocation of that total bandwidth between the memory clients. It has knobs to prioritize clients, the knobs are per read/write port. MC is facing memory clients from one side and EMC from the other. > I vaguely recall Dmitry mentioning that the EMC in early generations of > Tegra used to have controls for individual memory clients, but I don't > see that in more recent generations. EMC doesn't have direct controls over memory clients on all Tegra SoCs, but it may have some extra knobs for the MC arbitration config. The MC bandwidth allocation logic and hardware programming interface differs among SoC generations, but the basic principle is the same. >>> Any ideas on how to resolve this? Let me know if the DT bindings and >>> example don't make things clear enough. I'm also interested in the answer to this question. A quick thought.. maybe it could be some new ICC DT property which tells that all paths are the "dma-mem": interconnects-all-dma-mem; >>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ >>> 1 file changed, 59 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>> index dabf1c1aec2f..d40fcd836e90 100644 >>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>> @@ -43,6 +43,24 @@ properties: >>> - enum: >>> - nvidia,tegra186-bpmp >>> >>> + interconnects: >>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>> + description: A list of phandle and specifier pairs that describe the >>> + interconnect paths to and from the BPMP. >>> + >>> + interconnect-names: >>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>> + description: One string for each pair of phandle and specifier in the >>> + "interconnects" property. >>> + # XXX We need at least one of these to be named dma-mem so that the core >>> + # will set the DMA mask based on the DMA parent, but all of these go to >>> + # system memory eventually. >>> + items: >>> + - const: dma-mem >>> + - const: dma-mem >>> + - const: dma-mem >>> + - const: dma-mem Names should be unique, otherwise it's not possible to retrieve ICC path other than the first one. >>> iommus: >>> $ref: /schemas/types.yaml#/definitions/phandle-array >>> description: | >>> @@ -152,8 +170,43 @@ additionalProperties: false >>> >>> examples: >>> - | >>> + #include <dt-bindings/clock/tegra186-clock.h> >>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>> #include <dt-bindings/mailbox/tegra186-hsp.h> >>> + #include <dt-bindings/memory/tegra186-mc.h> >>> + >>> + mc: memory-controller@2c00000 { >>> + compatible = "nvidia,tegra186-mc"; >>> + reg = <0x02c00000 0xb0000>; >>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; >>> + status = "disabled"; >>> + >>> + #interconnect-cells = <1>; >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + >>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; >>> + >>> + /* >>> + * Memory clients have access to all 40 bits that the memory >>> + * controller can address. >>> + */ >>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; >>> + >>> + #memory-controller-cells = <0>; >>> + >>> + emc: external-memory-controller@2c60000 { >>> + compatible = "nvidia,tegra186-emc"; >>> + reg = <0x0 0x02c60000 0x0 0x50000>; >>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; >>> + clocks = <&bpmp TEGRA186_CLK_EMC>; >>> + clock-names = "emc"; >>> + >>> + #interconnect-cells = <0>; >>> + >>> + nvidia,bpmp = <&bpmp>; >>> + }; >>> + }; >>> >>> hsp_top0: hsp@3c00000 { >>> compatible = "nvidia,tegra186-hsp"; >>> @@ -187,6 +240,12 @@ examples: >>> >>> bpmp { >>> compatible = "nvidia,tegra186-bpmp"; >>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, >>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, >>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; I don't think this is a correct definition of the ICC paths because the first node-MC_ID pair should define the source, second pair is the final destination. Then the interconnect core builds (by itself) the path from MC client to MC and finally to EMC based on the given source / destination. Please see my v1 patchset for the example. It should look somewhat like this: interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; >>> + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; >>> + >>> iommus = <&smmu TEGRA186_SID_BPMP>; >>> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; >>> shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; >>> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-21 6:53 ` Dmitry Osipenko 0 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-21 6:53 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra, devicetree 20.01.2020 18:06, Thierry Reding пишет: > On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: >> Hi Thierry, >> >> Thanks for the patch! >> >> On 1/14/20 20:15, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> Document the interconnects property that is used to describe the paths >>> from and to system memory from and to the BPMP. >>> >>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>> --- >>> Rob, Georgi, >>> >>> after the initial RFC that I did for adding interconnect properties on >>> Tegra, I realized that the description wasn't complete. This is an >>> attempt at a more accurate description, but unfortunately I'm not sure >>> if it's even correct in terms of the interconnect bindings. >>> >>> The problem here is that on Tegra, each device has multiple paths to >>> system memory, and I have no good idea on what to pick as the default. >>> They are all basically the same path, but each provides extra controls >>> to configure the "interconnect". >> >> Are these multiple paths between a device and system memory used simultaneously >> for load-balancing, or who makes the decision about which path would be used? > > It varies. The vast majority of these paths are read/write pairs, which > can be configured separately. There are also cases where multiple paths > are used for load-balancing and I don't think there's any direct > software control over which path will be used. > > A third class is where you have one device, but two read/write pairs, > one which is tied to a microcontroller that's part of the device, and > another read/write pair that is used for DMA to/from the device. > > Often in the latter case, the microcontroller memory client interfaces > will be used by the microcontroller to read firmware and once the micro- > controller has booted up, the DMA memory client interfaces will be used > to read/write system memory with bulk data (like frame buffers, etc.). > >> Is this based on the client/stream ID that you mentioned previously? > > These are now all what's call memory client IDs, which identify the > corresponding interface to the memory controller. Stream IDs are > slightly higher-level and typically identify the "module" that uses > the SMMU. Generally a stream ID is mapped to one or more memory client > IDs. > >> Looking at the the binding below, it seems to me like there are different >> master/slave pairs between MC and EMC and each link is used for >> unidirectional traffic only. In terms of the interconnect API, both read >> and write paths have the same direction. Yes, that definition should be incorrect. > I'm not sure I understand what you mean by this last sentence. Are you > saying that each path in terms of the interconnect API is a always a > bidirectional link? Please see more below. >> Is the EMC really an interconnect provider or is it just a slave port? Can >> we scale both EMC and MC independently? > > The EMC is the only one where we can scale the frequency, but the MC has > various knobs that can be used to fine-tune arbitration, set maximum > latency, etc. Yes.. EMC controls the total amount of available memory bandwidth, things like DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from one side and DRAM (EMEM) from the other. MC controls allocation of that total bandwidth between the memory clients. It has knobs to prioritize clients, the knobs are per read/write port. MC is facing memory clients from one side and EMC from the other. > I vaguely recall Dmitry mentioning that the EMC in early generations of > Tegra used to have controls for individual memory clients, but I don't > see that in more recent generations. EMC doesn't have direct controls over memory clients on all Tegra SoCs, but it may have some extra knobs for the MC arbitration config. The MC bandwidth allocation logic and hardware programming interface differs among SoC generations, but the basic principle is the same. >>> Any ideas on how to resolve this? Let me know if the DT bindings and >>> example don't make things clear enough. I'm also interested in the answer to this question. A quick thought.. maybe it could be some new ICC DT property which tells that all paths are the "dma-mem": interconnects-all-dma-mem; >>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ >>> 1 file changed, 59 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>> index dabf1c1aec2f..d40fcd836e90 100644 >>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>> @@ -43,6 +43,24 @@ properties: >>> - enum: >>> - nvidia,tegra186-bpmp >>> >>> + interconnects: >>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>> + description: A list of phandle and specifier pairs that describe the >>> + interconnect paths to and from the BPMP. >>> + >>> + interconnect-names: >>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>> + description: One string for each pair of phandle and specifier in the >>> + "interconnects" property. >>> + # XXX We need at least one of these to be named dma-mem so that the core >>> + # will set the DMA mask based on the DMA parent, but all of these go to >>> + # system memory eventually. >>> + items: >>> + - const: dma-mem >>> + - const: dma-mem >>> + - const: dma-mem >>> + - const: dma-mem Names should be unique, otherwise it's not possible to retrieve ICC path other than the first one. >>> iommus: >>> $ref: /schemas/types.yaml#/definitions/phandle-array >>> description: | >>> @@ -152,8 +170,43 @@ additionalProperties: false >>> >>> examples: >>> - | >>> + #include <dt-bindings/clock/tegra186-clock.h> >>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>> #include <dt-bindings/mailbox/tegra186-hsp.h> >>> + #include <dt-bindings/memory/tegra186-mc.h> >>> + >>> + mc: memory-controller@2c00000 { >>> + compatible = "nvidia,tegra186-mc"; >>> + reg = <0x02c00000 0xb0000>; >>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; >>> + status = "disabled"; >>> + >>> + #interconnect-cells = <1>; >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + >>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; >>> + >>> + /* >>> + * Memory clients have access to all 40 bits that the memory >>> + * controller can address. >>> + */ >>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; >>> + >>> + #memory-controller-cells = <0>; >>> + >>> + emc: external-memory-controller@2c60000 { >>> + compatible = "nvidia,tegra186-emc"; >>> + reg = <0x0 0x02c60000 0x0 0x50000>; >>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; >>> + clocks = <&bpmp TEGRA186_CLK_EMC>; >>> + clock-names = "emc"; >>> + >>> + #interconnect-cells = <0>; >>> + >>> + nvidia,bpmp = <&bpmp>; >>> + }; >>> + }; >>> >>> hsp_top0: hsp@3c00000 { >>> compatible = "nvidia,tegra186-hsp"; >>> @@ -187,6 +240,12 @@ examples: >>> >>> bpmp { >>> compatible = "nvidia,tegra186-bpmp"; >>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, >>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, >>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; I don't think this is a correct definition of the ICC paths because the first node-MC_ID pair should define the source, second pair is the final destination. Then the interconnect core builds (by itself) the path from MC client to MC and finally to EMC based on the given source / destination. Please see my v1 patchset for the example. It should look somewhat like this: interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; >>> + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; >>> + >>> iommus = <&smmu TEGRA186_SID_BPMP>; >>> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; >>> shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; >>> ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <57c37b3c-1473-d444-db59-8c6650241188-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-21 6:53 ` Dmitry Osipenko @ 2020-01-21 14:10 ` Thierry Reding -1 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-21 14:10 UTC (permalink / raw) To: Dmitry Osipenko, Rob Herring Cc: Georgi Djakov, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 11793 bytes --] On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: > 20.01.2020 18:06, Thierry Reding пишет: > > On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: > >> Hi Thierry, > >> > >> Thanks for the patch! > >> > >> On 1/14/20 20:15, Thierry Reding wrote: > >>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > >>> > >>> Document the interconnects property that is used to describe the paths > >>> from and to system memory from and to the BPMP. > >>> > >>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > >>> --- > >>> Rob, Georgi, > >>> > >>> after the initial RFC that I did for adding interconnect properties on > >>> Tegra, I realized that the description wasn't complete. This is an > >>> attempt at a more accurate description, but unfortunately I'm not sure > >>> if it's even correct in terms of the interconnect bindings. > >>> > >>> The problem here is that on Tegra, each device has multiple paths to > >>> system memory, and I have no good idea on what to pick as the default. > >>> They are all basically the same path, but each provides extra controls > >>> to configure the "interconnect". > >> > >> Are these multiple paths between a device and system memory used simultaneously > >> for load-balancing, or who makes the decision about which path would be used? > > > > It varies. The vast majority of these paths are read/write pairs, which > > can be configured separately. There are also cases where multiple paths > > are used for load-balancing and I don't think there's any direct > > software control over which path will be used. > > > > A third class is where you have one device, but two read/write pairs, > > one which is tied to a microcontroller that's part of the device, and > > another read/write pair that is used for DMA to/from the device. > > > > Often in the latter case, the microcontroller memory client interfaces > > will be used by the microcontroller to read firmware and once the micro- > > controller has booted up, the DMA memory client interfaces will be used > > to read/write system memory with bulk data (like frame buffers, etc.). > > > >> Is this based on the client/stream ID that you mentioned previously? > > > > These are now all what's call memory client IDs, which identify the > > corresponding interface to the memory controller. Stream IDs are > > slightly higher-level and typically identify the "module" that uses > > the SMMU. Generally a stream ID is mapped to one or more memory client > > IDs. > > > >> Looking at the the binding below, it seems to me like there are different > >> master/slave pairs between MC and EMC and each link is used for > >> unidirectional traffic only. In terms of the interconnect API, both read > >> and write paths have the same direction. > > Yes, that definition should be incorrect. > > > I'm not sure I understand what you mean by this last sentence. Are you > > saying that each path in terms of the interconnect API is a always a > > bidirectional link? > > Please see more below. > > >> Is the EMC really an interconnect provider or is it just a slave port? Can > >> we scale both EMC and MC independently? > > > > The EMC is the only one where we can scale the frequency, but the MC has > > various knobs that can be used to fine-tune arbitration, set maximum > > latency, etc. > > Yes.. > > > EMC controls the total amount of available memory bandwidth, things like > DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from > one side and DRAM (EMEM) from the other. > > > > MC controls allocation of that total bandwidth between the memory > clients. It has knobs to prioritize clients, the knobs are per > read/write port. MC is facing memory clients from one side and EMC from > the other. > > > > I vaguely recall Dmitry mentioning that the EMC in early generations of > > Tegra used to have controls for individual memory clients, but I don't > > see that in more recent generations. > > EMC doesn't have direct controls over memory clients on all Tegra SoCs, > but it may have some extra knobs for the MC arbitration config. > > The MC bandwidth allocation logic and hardware programming interface > differs among SoC generations, but the basic principle is the same. > > >>> Any ideas on how to resolve this? Let me know if the DT bindings and > >>> example don't make things clear enough. > > I'm also interested in the answer to this question. > > A quick thought.. maybe it could be some new ICC DT property which tells > that all paths are the "dma-mem": > > interconnects-all-dma-mem; There could easily be cases where multiple interconnects are to system memory but there are additional ones which aren't, so the above wouldn't be able to represent such cases. > >>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ > >>> 1 file changed, 59 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>> index dabf1c1aec2f..d40fcd836e90 100644 > >>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>> @@ -43,6 +43,24 @@ properties: > >>> - enum: > >>> - nvidia,tegra186-bpmp > >>> > >>> + interconnects: > >>> + $ref: /schemas/types.yaml#/definitions/phandle-array > >>> + description: A list of phandle and specifier pairs that describe the > >>> + interconnect paths to and from the BPMP. > >>> + > >>> + interconnect-names: > >>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > >>> + description: One string for each pair of phandle and specifier in the > >>> + "interconnects" property. > >>> + # XXX We need at least one of these to be named dma-mem so that the core > >>> + # will set the DMA mask based on the DMA parent, but all of these go to > >>> + # system memory eventually. > >>> + items: > >>> + - const: dma-mem > >>> + - const: dma-mem > >>> + - const: dma-mem > >>> + - const: dma-mem > > Names should be unique, otherwise it's not possible to retrieve ICC path > other than the first one. Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure what else to put there and thought this kinda made it clear that it was only half-baked. > >>> iommus: > >>> $ref: /schemas/types.yaml#/definitions/phandle-array > >>> description: | > >>> @@ -152,8 +170,43 @@ additionalProperties: false > >>> > >>> examples: > >>> - | > >>> + #include <dt-bindings/clock/tegra186-clock.h> > >>> #include <dt-bindings/interrupt-controller/arm-gic.h> > >>> #include <dt-bindings/mailbox/tegra186-hsp.h> > >>> + #include <dt-bindings/memory/tegra186-mc.h> > >>> + > >>> + mc: memory-controller@2c00000 { > >>> + compatible = "nvidia,tegra186-mc"; > >>> + reg = <0x02c00000 0xb0000>; > >>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; > >>> + status = "disabled"; > >>> + > >>> + #interconnect-cells = <1>; > >>> + #address-cells = <2>; > >>> + #size-cells = <2>; > >>> + > >>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; > >>> + > >>> + /* > >>> + * Memory clients have access to all 40 bits that the memory > >>> + * controller can address. > >>> + */ > >>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; > >>> + > >>> + #memory-controller-cells = <0>; > >>> + > >>> + emc: external-memory-controller@2c60000 { > >>> + compatible = "nvidia,tegra186-emc"; > >>> + reg = <0x0 0x02c60000 0x0 0x50000>; > >>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; > >>> + clocks = <&bpmp TEGRA186_CLK_EMC>; > >>> + clock-names = "emc"; > >>> + > >>> + #interconnect-cells = <0>; > >>> + > >>> + nvidia,bpmp = <&bpmp>; > >>> + }; > >>> + }; > >>> > >>> hsp_top0: hsp@3c00000 { > >>> compatible = "nvidia,tegra186-hsp"; > >>> @@ -187,6 +240,12 @@ examples: > >>> > >>> bpmp { > >>> compatible = "nvidia,tegra186-bpmp"; > >>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, > >>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, > >>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > >>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; > > I don't think this is a correct definition of the ICC paths because the > first node-MC_ID pair should define the source, second pair is the final > destination. Then the interconnect core builds (by itself) the path from > MC client to MC and finally to EMC based on the given source / > destination. Please see my v1 patchset for the example. Okay, sounds like "source" in this case means the initiator of the transaction and destination is the target of the transaction. I had interpreted the "source" as the "source location" of the transaction (so for reads the source would be the system memory via the EMC, and for writes the source would be the memory client interface). Yeah, I think that makes sense. It was also pointed out to me (offline) that the above doesn't work as intented for the use-case where I really need it. The primary reason why I need these "dma-mem" interconnect paths is so that the memory controller is specified as the "DMA parent" for these devices, which is important so that the DMA masks can be correctly set. Having the &emc reference in the first slot breaks that. Your suggestion makes sense when interpreting the terminology differently and it fixes the DMA parent issue (at least partially). > It should look somewhat like this: > > interconnects = > <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; > > interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always going to be the same and it's arbitrarily defined, so it's effectively useless. But other than that it looks good. I suppose one could argue about the names a bit. Having the "bpmp" prefix for all of them feels a little redundant. They could also be [ "read", "write", "dma-read", "dma-write" ], which would make them a little more contextual, like we do with other clocks. However, like I said before, at least one of these would need to be named "dma-mem" in order for the memory controller to be selected as the DMA parent. But, perhaps we just need to look at this some other way and specify an additional way of specifying the DMA parent for devices that doesn't rely on a "dma-mem" interconnect-names property. Perhaps some new dma-parent property that takes a phandle (with perhaps an optional specifier) would work? I think that may tie in nicely with the memory controller mini-framework that I had proposed a while ago. Rob, any thoughts on that? Thierry > >>> + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; > >>> + > >>> iommus = <&smmu TEGRA186_SID_BPMP>; > >>> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; > >>> shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; > >>> > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-21 14:10 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-21 14:10 UTC (permalink / raw) To: Dmitry Osipenko, Rob Herring Cc: Georgi Djakov, Jon Hunter, linux-tegra, devicetree [-- Attachment #1: Type: text/plain, Size: 11735 bytes --] On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: > 20.01.2020 18:06, Thierry Reding пишет: > > On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: > >> Hi Thierry, > >> > >> Thanks for the patch! > >> > >> On 1/14/20 20:15, Thierry Reding wrote: > >>> From: Thierry Reding <treding@nvidia.com> > >>> > >>> Document the interconnects property that is used to describe the paths > >>> from and to system memory from and to the BPMP. > >>> > >>> Signed-off-by: Thierry Reding <treding@nvidia.com> > >>> --- > >>> Rob, Georgi, > >>> > >>> after the initial RFC that I did for adding interconnect properties on > >>> Tegra, I realized that the description wasn't complete. This is an > >>> attempt at a more accurate description, but unfortunately I'm not sure > >>> if it's even correct in terms of the interconnect bindings. > >>> > >>> The problem here is that on Tegra, each device has multiple paths to > >>> system memory, and I have no good idea on what to pick as the default. > >>> They are all basically the same path, but each provides extra controls > >>> to configure the "interconnect". > >> > >> Are these multiple paths between a device and system memory used simultaneously > >> for load-balancing, or who makes the decision about which path would be used? > > > > It varies. The vast majority of these paths are read/write pairs, which > > can be configured separately. There are also cases where multiple paths > > are used for load-balancing and I don't think there's any direct > > software control over which path will be used. > > > > A third class is where you have one device, but two read/write pairs, > > one which is tied to a microcontroller that's part of the device, and > > another read/write pair that is used for DMA to/from the device. > > > > Often in the latter case, the microcontroller memory client interfaces > > will be used by the microcontroller to read firmware and once the micro- > > controller has booted up, the DMA memory client interfaces will be used > > to read/write system memory with bulk data (like frame buffers, etc.). > > > >> Is this based on the client/stream ID that you mentioned previously? > > > > These are now all what's call memory client IDs, which identify the > > corresponding interface to the memory controller. Stream IDs are > > slightly higher-level and typically identify the "module" that uses > > the SMMU. Generally a stream ID is mapped to one or more memory client > > IDs. > > > >> Looking at the the binding below, it seems to me like there are different > >> master/slave pairs between MC and EMC and each link is used for > >> unidirectional traffic only. In terms of the interconnect API, both read > >> and write paths have the same direction. > > Yes, that definition should be incorrect. > > > I'm not sure I understand what you mean by this last sentence. Are you > > saying that each path in terms of the interconnect API is a always a > > bidirectional link? > > Please see more below. > > >> Is the EMC really an interconnect provider or is it just a slave port? Can > >> we scale both EMC and MC independently? > > > > The EMC is the only one where we can scale the frequency, but the MC has > > various knobs that can be used to fine-tune arbitration, set maximum > > latency, etc. > > Yes.. > > > EMC controls the total amount of available memory bandwidth, things like > DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from > one side and DRAM (EMEM) from the other. > > > > MC controls allocation of that total bandwidth between the memory > clients. It has knobs to prioritize clients, the knobs are per > read/write port. MC is facing memory clients from one side and EMC from > the other. > > > > I vaguely recall Dmitry mentioning that the EMC in early generations of > > Tegra used to have controls for individual memory clients, but I don't > > see that in more recent generations. > > EMC doesn't have direct controls over memory clients on all Tegra SoCs, > but it may have some extra knobs for the MC arbitration config. > > The MC bandwidth allocation logic and hardware programming interface > differs among SoC generations, but the basic principle is the same. > > >>> Any ideas on how to resolve this? Let me know if the DT bindings and > >>> example don't make things clear enough. > > I'm also interested in the answer to this question. > > A quick thought.. maybe it could be some new ICC DT property which tells > that all paths are the "dma-mem": > > interconnects-all-dma-mem; There could easily be cases where multiple interconnects are to system memory but there are additional ones which aren't, so the above wouldn't be able to represent such cases. > >>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ > >>> 1 file changed, 59 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>> index dabf1c1aec2f..d40fcd836e90 100644 > >>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>> @@ -43,6 +43,24 @@ properties: > >>> - enum: > >>> - nvidia,tegra186-bpmp > >>> > >>> + interconnects: > >>> + $ref: /schemas/types.yaml#/definitions/phandle-array > >>> + description: A list of phandle and specifier pairs that describe the > >>> + interconnect paths to and from the BPMP. > >>> + > >>> + interconnect-names: > >>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > >>> + description: One string for each pair of phandle and specifier in the > >>> + "interconnects" property. > >>> + # XXX We need at least one of these to be named dma-mem so that the core > >>> + # will set the DMA mask based on the DMA parent, but all of these go to > >>> + # system memory eventually. > >>> + items: > >>> + - const: dma-mem > >>> + - const: dma-mem > >>> + - const: dma-mem > >>> + - const: dma-mem > > Names should be unique, otherwise it's not possible to retrieve ICC path > other than the first one. Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure what else to put there and thought this kinda made it clear that it was only half-baked. > >>> iommus: > >>> $ref: /schemas/types.yaml#/definitions/phandle-array > >>> description: | > >>> @@ -152,8 +170,43 @@ additionalProperties: false > >>> > >>> examples: > >>> - | > >>> + #include <dt-bindings/clock/tegra186-clock.h> > >>> #include <dt-bindings/interrupt-controller/arm-gic.h> > >>> #include <dt-bindings/mailbox/tegra186-hsp.h> > >>> + #include <dt-bindings/memory/tegra186-mc.h> > >>> + > >>> + mc: memory-controller@2c00000 { > >>> + compatible = "nvidia,tegra186-mc"; > >>> + reg = <0x02c00000 0xb0000>; > >>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; > >>> + status = "disabled"; > >>> + > >>> + #interconnect-cells = <1>; > >>> + #address-cells = <2>; > >>> + #size-cells = <2>; > >>> + > >>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; > >>> + > >>> + /* > >>> + * Memory clients have access to all 40 bits that the memory > >>> + * controller can address. > >>> + */ > >>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; > >>> + > >>> + #memory-controller-cells = <0>; > >>> + > >>> + emc: external-memory-controller@2c60000 { > >>> + compatible = "nvidia,tegra186-emc"; > >>> + reg = <0x0 0x02c60000 0x0 0x50000>; > >>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; > >>> + clocks = <&bpmp TEGRA186_CLK_EMC>; > >>> + clock-names = "emc"; > >>> + > >>> + #interconnect-cells = <0>; > >>> + > >>> + nvidia,bpmp = <&bpmp>; > >>> + }; > >>> + }; > >>> > >>> hsp_top0: hsp@3c00000 { > >>> compatible = "nvidia,tegra186-hsp"; > >>> @@ -187,6 +240,12 @@ examples: > >>> > >>> bpmp { > >>> compatible = "nvidia,tegra186-bpmp"; > >>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, > >>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, > >>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > >>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; > > I don't think this is a correct definition of the ICC paths because the > first node-MC_ID pair should define the source, second pair is the final > destination. Then the interconnect core builds (by itself) the path from > MC client to MC and finally to EMC based on the given source / > destination. Please see my v1 patchset for the example. Okay, sounds like "source" in this case means the initiator of the transaction and destination is the target of the transaction. I had interpreted the "source" as the "source location" of the transaction (so for reads the source would be the system memory via the EMC, and for writes the source would be the memory client interface). Yeah, I think that makes sense. It was also pointed out to me (offline) that the above doesn't work as intented for the use-case where I really need it. The primary reason why I need these "dma-mem" interconnect paths is so that the memory controller is specified as the "DMA parent" for these devices, which is important so that the DMA masks can be correctly set. Having the &emc reference in the first slot breaks that. Your suggestion makes sense when interpreting the terminology differently and it fixes the DMA parent issue (at least partially). > It should look somewhat like this: > > interconnects = > <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; > > interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always going to be the same and it's arbitrarily defined, so it's effectively useless. But other than that it looks good. I suppose one could argue about the names a bit. Having the "bpmp" prefix for all of them feels a little redundant. They could also be [ "read", "write", "dma-read", "dma-write" ], which would make them a little more contextual, like we do with other clocks. However, like I said before, at least one of these would need to be named "dma-mem" in order for the memory controller to be selected as the DMA parent. But, perhaps we just need to look at this some other way and specify an additional way of specifying the DMA parent for devices that doesn't rely on a "dma-mem" interconnect-names property. Perhaps some new dma-parent property that takes a phandle (with perhaps an optional specifier) would work? I think that may tie in nicely with the memory controller mini-framework that I had proposed a while ago. Rob, any thoughts on that? Thierry > >>> + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; > >>> + > >>> iommus = <&smmu TEGRA186_SID_BPMP>; > >>> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; > >>> shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; > >>> > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-21 14:10 ` Thierry Reding @ 2020-01-21 15:18 ` Georgi Djakov -1 siblings, 0 replies; 42+ messages in thread From: Georgi Djakov @ 2020-01-21 15:18 UTC (permalink / raw) To: Thierry Reding, Dmitry Osipenko, Rob Herring Cc: Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA On 1/21/20 16:10, Thierry Reding wrote: > On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: >> 20.01.2020 18:06, Thierry Reding пишет: >>> On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: >>>> Hi Thierry, >>>> >>>> Thanks for the patch! >>>> >>>> On 1/14/20 20:15, Thierry Reding wrote: >>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>>>> >>>>> Document the interconnects property that is used to describe the paths >>>>> from and to system memory from and to the BPMP. >>>>> >>>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>>>> --- >>>>> Rob, Georgi, >>>>> >>>>> after the initial RFC that I did for adding interconnect properties on >>>>> Tegra, I realized that the description wasn't complete. This is an >>>>> attempt at a more accurate description, but unfortunately I'm not sure >>>>> if it's even correct in terms of the interconnect bindings. >>>>> >>>>> The problem here is that on Tegra, each device has multiple paths to >>>>> system memory, and I have no good idea on what to pick as the default. >>>>> They are all basically the same path, but each provides extra controls >>>>> to configure the "interconnect". >>>> >>>> Are these multiple paths between a device and system memory used simultaneously >>>> for load-balancing, or who makes the decision about which path would be used? >>> >>> It varies. The vast majority of these paths are read/write pairs, which >>> can be configured separately. There are also cases where multiple paths >>> are used for load-balancing and I don't think there's any direct >>> software control over which path will be used. >>> >>> A third class is where you have one device, but two read/write pairs, >>> one which is tied to a microcontroller that's part of the device, and >>> another read/write pair that is used for DMA to/from the device. >>> >>> Often in the latter case, the microcontroller memory client interfaces >>> will be used by the microcontroller to read firmware and once the micro- >>> controller has booted up, the DMA memory client interfaces will be used >>> to read/write system memory with bulk data (like frame buffers, etc.). >>> >>>> Is this based on the client/stream ID that you mentioned previously? >>> >>> These are now all what's call memory client IDs, which identify the >>> corresponding interface to the memory controller. Stream IDs are >>> slightly higher-level and typically identify the "module" that uses >>> the SMMU. Generally a stream ID is mapped to one or more memory client >>> IDs. >>> >>>> Looking at the the binding below, it seems to me like there are different >>>> master/slave pairs between MC and EMC and each link is used for >>>> unidirectional traffic only. In terms of the interconnect API, both read >>>> and write paths have the same direction. >> >> Yes, that definition should be incorrect. >> >>> I'm not sure I understand what you mean by this last sentence. Are you >>> saying that each path in terms of the interconnect API is a always a >>> bidirectional link? >> >> Please see more below. >> >>>> Is the EMC really an interconnect provider or is it just a slave port? Can >>>> we scale both EMC and MC independently? >>> >>> The EMC is the only one where we can scale the frequency, but the MC has >>> various knobs that can be used to fine-tune arbitration, set maximum >>> latency, etc. >> >> Yes.. >> >> >> EMC controls the total amount of available memory bandwidth, things like >> DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from >> one side and DRAM (EMEM) from the other. >> Right, so we can use the icc framework here to aggregate the requested bandwidth from all clients and scale the frequency/voltage of EMC. >> >> >> MC controls allocation of that total bandwidth between the memory >> clients. It has knobs to prioritize clients, the knobs are per >> read/write port. MC is facing memory clients from one side and EMC from >> the other. >> Thanks for clarifying! So are these QoS knobs (priority, latency etc.) tuned dynamically during runtime or is it more like a static configuration that is done for example just once during system boot? >> >>> I vaguely recall Dmitry mentioning that the EMC in early generations of >>> Tegra used to have controls for individual memory clients, but I don't >>> see that in more recent generations. >> >> EMC doesn't have direct controls over memory clients on all Tegra SoCs, >> but it may have some extra knobs for the MC arbitration config. >> >> The MC bandwidth allocation logic and hardware programming interface >> differs among SoC generations, but the basic principle is the same. >> >>>>> Any ideas on how to resolve this? Let me know if the DT bindings and >>>>> example don't make things clear enough. >> >> I'm also interested in the answer to this question. >> >> A quick thought.. maybe it could be some new ICC DT property which tells >> that all paths are the "dma-mem": >> >> interconnects-all-dma-mem; > > There could easily be cases where multiple interconnects are to system > memory but there are additional ones which aren't, so the above wouldn't > be able to represent such cases. Yes, true. >>>>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ >>>>> 1 file changed, 59 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>> index dabf1c1aec2f..d40fcd836e90 100644 >>>>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>> @@ -43,6 +43,24 @@ properties: >>>>> - enum: >>>>> - nvidia,tegra186-bpmp >>>>> >>>>> + interconnects: >>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>> + description: A list of phandle and specifier pairs that describe the >>>>> + interconnect paths to and from the BPMP. >>>>> + >>>>> + interconnect-names: >>>>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>>>> + description: One string for each pair of phandle and specifier in the >>>>> + "interconnects" property. >>>>> + # XXX We need at least one of these to be named dma-mem so that the core >>>>> + # will set the DMA mask based on the DMA parent, but all of these go to >>>>> + # system memory eventually. >>>>> + items: >>>>> + - const: dma-mem >>>>> + - const: dma-mem >>>>> + - const: dma-mem >>>>> + - const: dma-mem >> >> Names should be unique, otherwise it's not possible to retrieve ICC path >> other than the first one. > > Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure > what else to put there and thought this kinda made it clear that it was > only half-baked. > >>>>> iommus: >>>>> $ref: /schemas/types.yaml#/definitions/phandle-array >>>>> description: | >>>>> @@ -152,8 +170,43 @@ additionalProperties: false >>>>> >>>>> examples: >>>>> - | >>>>> + #include <dt-bindings/clock/tegra186-clock.h> >>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>> #include <dt-bindings/mailbox/tegra186-hsp.h> >>>>> + #include <dt-bindings/memory/tegra186-mc.h> >>>>> + >>>>> + mc: memory-controller@2c00000 { >>>>> + compatible = "nvidia,tegra186-mc"; >>>>> + reg = <0x02c00000 0xb0000>; >>>>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; >>>>> + status = "disabled"; >>>>> + >>>>> + #interconnect-cells = <1>; >>>>> + #address-cells = <2>; >>>>> + #size-cells = <2>; >>>>> + >>>>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; >>>>> + >>>>> + /* >>>>> + * Memory clients have access to all 40 bits that the memory >>>>> + * controller can address. >>>>> + */ >>>>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; >>>>> + >>>>> + #memory-controller-cells = <0>; >>>>> + >>>>> + emc: external-memory-controller@2c60000 { >>>>> + compatible = "nvidia,tegra186-emc"; >>>>> + reg = <0x0 0x02c60000 0x0 0x50000>; >>>>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; >>>>> + clocks = <&bpmp TEGRA186_CLK_EMC>; >>>>> + clock-names = "emc"; >>>>> + >>>>> + #interconnect-cells = <0>; >>>>> + >>>>> + nvidia,bpmp = <&bpmp>; >>>>> + }; >>>>> + }; >>>>> >>>>> hsp_top0: hsp@3c00000 { >>>>> compatible = "nvidia,tegra186-hsp"; >>>>> @@ -187,6 +240,12 @@ examples: >>>>> >>>>> bpmp { >>>>> compatible = "nvidia,tegra186-bpmp"; >>>>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, >>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, >>>>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; >> >> I don't think this is a correct definition of the ICC paths because the >> first node-MC_ID pair should define the source, second pair is the final >> destination. Then the interconnect core builds (by itself) the path from >> MC client to MC and finally to EMC based on the given source / >> destination. Please see my v1 patchset for the example. > > Okay, sounds like "source" in this case means the initiator of the > transaction and destination is the target of the transaction. I had > interpreted the "source" as the "source location" of the transaction (so > for reads the source would be the system memory via the EMC, and for > writes the source would be the memory client interface). Yes, exactly. Maybe it would be more correct to call these pairs initiator/target or master/slave. > Yeah, I think that makes sense. It was also pointed out to me (offline) > that the above doesn't work as intented for the use-case where I really > need it. The primary reason why I need these "dma-mem" interconnect > paths is so that the memory controller is specified as the "DMA parent" > for these devices, which is important so that the DMA masks can be > correctly set. Having the &emc reference in the first slot breaks that. > Your suggestion makes sense when interpreting the terminology > differently and it fixes the DMA parent issue (at least partially). > >> It should look somewhat like this: >> >> interconnects = >> <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; >> >> interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; This looks better to me. > I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always > going to be the same and it's arbitrarily defined, so it's effectively > useless. But other than that it looks good. Well, in most cases the target would be the EMEM, so that's fine. I have seen that other vendors that may have an additional internal memory, especially dedicated to some DSPs and in such cases the bandwidth needs are different for the two paths (to internal memory and DDR). > I suppose one could argue about the names a bit. Having the "bpmp" > prefix for all of them feels a little redundant. They could also be > [ "read", "write", "dma-read", "dma-write" ], which would make them > a little more contextual, like we do with other clocks. > > However, like I said before, at least one of these would need to be > named "dma-mem" in order for the memory controller to be selected as > the DMA parent. But, perhaps we just need to look at this some other > way and specify an additional way of specifying the DMA parent for > devices that doesn't rely on a "dma-mem" interconnect-names property. > > Perhaps some new dma-parent property that takes a phandle (with perhaps > an optional specifier) would work? I think that may tie in nicely with > the memory controller mini-framework that I had proposed a while ago. > > Rob, any thoughts on that? > > Thierry > >>>>> + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; >>>>> + >>>>> iommus = <&smmu TEGRA186_SID_BPMP>; >>>>> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; >>>>> shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; >>>>> >> Thanks, Georgi ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-21 15:18 ` Georgi Djakov 0 siblings, 0 replies; 42+ messages in thread From: Georgi Djakov @ 2020-01-21 15:18 UTC (permalink / raw) To: Thierry Reding, Dmitry Osipenko, Rob Herring Cc: Jon Hunter, linux-tegra, devicetree On 1/21/20 16:10, Thierry Reding wrote: > On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: >> 20.01.2020 18:06, Thierry Reding пишет: >>> On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: >>>> Hi Thierry, >>>> >>>> Thanks for the patch! >>>> >>>> On 1/14/20 20:15, Thierry Reding wrote: >>>>> From: Thierry Reding <treding@nvidia.com> >>>>> >>>>> Document the interconnects property that is used to describe the paths >>>>> from and to system memory from and to the BPMP. >>>>> >>>>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>>>> --- >>>>> Rob, Georgi, >>>>> >>>>> after the initial RFC that I did for adding interconnect properties on >>>>> Tegra, I realized that the description wasn't complete. This is an >>>>> attempt at a more accurate description, but unfortunately I'm not sure >>>>> if it's even correct in terms of the interconnect bindings. >>>>> >>>>> The problem here is that on Tegra, each device has multiple paths to >>>>> system memory, and I have no good idea on what to pick as the default. >>>>> They are all basically the same path, but each provides extra controls >>>>> to configure the "interconnect". >>>> >>>> Are these multiple paths between a device and system memory used simultaneously >>>> for load-balancing, or who makes the decision about which path would be used? >>> >>> It varies. The vast majority of these paths are read/write pairs, which >>> can be configured separately. There are also cases where multiple paths >>> are used for load-balancing and I don't think there's any direct >>> software control over which path will be used. >>> >>> A third class is where you have one device, but two read/write pairs, >>> one which is tied to a microcontroller that's part of the device, and >>> another read/write pair that is used for DMA to/from the device. >>> >>> Often in the latter case, the microcontroller memory client interfaces >>> will be used by the microcontroller to read firmware and once the micro- >>> controller has booted up, the DMA memory client interfaces will be used >>> to read/write system memory with bulk data (like frame buffers, etc.). >>> >>>> Is this based on the client/stream ID that you mentioned previously? >>> >>> These are now all what's call memory client IDs, which identify the >>> corresponding interface to the memory controller. Stream IDs are >>> slightly higher-level and typically identify the "module" that uses >>> the SMMU. Generally a stream ID is mapped to one or more memory client >>> IDs. >>> >>>> Looking at the the binding below, it seems to me like there are different >>>> master/slave pairs between MC and EMC and each link is used for >>>> unidirectional traffic only. In terms of the interconnect API, both read >>>> and write paths have the same direction. >> >> Yes, that definition should be incorrect. >> >>> I'm not sure I understand what you mean by this last sentence. Are you >>> saying that each path in terms of the interconnect API is a always a >>> bidirectional link? >> >> Please see more below. >> >>>> Is the EMC really an interconnect provider or is it just a slave port? Can >>>> we scale both EMC and MC independently? >>> >>> The EMC is the only one where we can scale the frequency, but the MC has >>> various knobs that can be used to fine-tune arbitration, set maximum >>> latency, etc. >> >> Yes.. >> >> >> EMC controls the total amount of available memory bandwidth, things like >> DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from >> one side and DRAM (EMEM) from the other. >> Right, so we can use the icc framework here to aggregate the requested bandwidth from all clients and scale the frequency/voltage of EMC. >> >> >> MC controls allocation of that total bandwidth between the memory >> clients. It has knobs to prioritize clients, the knobs are per >> read/write port. MC is facing memory clients from one side and EMC from >> the other. >> Thanks for clarifying! So are these QoS knobs (priority, latency etc.) tuned dynamically during runtime or is it more like a static configuration that is done for example just once during system boot? >> >>> I vaguely recall Dmitry mentioning that the EMC in early generations of >>> Tegra used to have controls for individual memory clients, but I don't >>> see that in more recent generations. >> >> EMC doesn't have direct controls over memory clients on all Tegra SoCs, >> but it may have some extra knobs for the MC arbitration config. >> >> The MC bandwidth allocation logic and hardware programming interface >> differs among SoC generations, but the basic principle is the same. >> >>>>> Any ideas on how to resolve this? Let me know if the DT bindings and >>>>> example don't make things clear enough. >> >> I'm also interested in the answer to this question. >> >> A quick thought.. maybe it could be some new ICC DT property which tells >> that all paths are the "dma-mem": >> >> interconnects-all-dma-mem; > > There could easily be cases where multiple interconnects are to system > memory but there are additional ones which aren't, so the above wouldn't > be able to represent such cases. Yes, true. >>>>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ >>>>> 1 file changed, 59 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>> index dabf1c1aec2f..d40fcd836e90 100644 >>>>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>> @@ -43,6 +43,24 @@ properties: >>>>> - enum: >>>>> - nvidia,tegra186-bpmp >>>>> >>>>> + interconnects: >>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>> + description: A list of phandle and specifier pairs that describe the >>>>> + interconnect paths to and from the BPMP. >>>>> + >>>>> + interconnect-names: >>>>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>>>> + description: One string for each pair of phandle and specifier in the >>>>> + "interconnects" property. >>>>> + # XXX We need at least one of these to be named dma-mem so that the core >>>>> + # will set the DMA mask based on the DMA parent, but all of these go to >>>>> + # system memory eventually. >>>>> + items: >>>>> + - const: dma-mem >>>>> + - const: dma-mem >>>>> + - const: dma-mem >>>>> + - const: dma-mem >> >> Names should be unique, otherwise it's not possible to retrieve ICC path >> other than the first one. > > Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure > what else to put there and thought this kinda made it clear that it was > only half-baked. > >>>>> iommus: >>>>> $ref: /schemas/types.yaml#/definitions/phandle-array >>>>> description: | >>>>> @@ -152,8 +170,43 @@ additionalProperties: false >>>>> >>>>> examples: >>>>> - | >>>>> + #include <dt-bindings/clock/tegra186-clock.h> >>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>> #include <dt-bindings/mailbox/tegra186-hsp.h> >>>>> + #include <dt-bindings/memory/tegra186-mc.h> >>>>> + >>>>> + mc: memory-controller@2c00000 { >>>>> + compatible = "nvidia,tegra186-mc"; >>>>> + reg = <0x02c00000 0xb0000>; >>>>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; >>>>> + status = "disabled"; >>>>> + >>>>> + #interconnect-cells = <1>; >>>>> + #address-cells = <2>; >>>>> + #size-cells = <2>; >>>>> + >>>>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; >>>>> + >>>>> + /* >>>>> + * Memory clients have access to all 40 bits that the memory >>>>> + * controller can address. >>>>> + */ >>>>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; >>>>> + >>>>> + #memory-controller-cells = <0>; >>>>> + >>>>> + emc: external-memory-controller@2c60000 { >>>>> + compatible = "nvidia,tegra186-emc"; >>>>> + reg = <0x0 0x02c60000 0x0 0x50000>; >>>>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; >>>>> + clocks = <&bpmp TEGRA186_CLK_EMC>; >>>>> + clock-names = "emc"; >>>>> + >>>>> + #interconnect-cells = <0>; >>>>> + >>>>> + nvidia,bpmp = <&bpmp>; >>>>> + }; >>>>> + }; >>>>> >>>>> hsp_top0: hsp@3c00000 { >>>>> compatible = "nvidia,tegra186-hsp"; >>>>> @@ -187,6 +240,12 @@ examples: >>>>> >>>>> bpmp { >>>>> compatible = "nvidia,tegra186-bpmp"; >>>>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, >>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, >>>>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; >> >> I don't think this is a correct definition of the ICC paths because the >> first node-MC_ID pair should define the source, second pair is the final >> destination. Then the interconnect core builds (by itself) the path from >> MC client to MC and finally to EMC based on the given source / >> destination. Please see my v1 patchset for the example. > > Okay, sounds like "source" in this case means the initiator of the > transaction and destination is the target of the transaction. I had > interpreted the "source" as the "source location" of the transaction (so > for reads the source would be the system memory via the EMC, and for > writes the source would be the memory client interface). Yes, exactly. Maybe it would be more correct to call these pairs initiator/target or master/slave. > Yeah, I think that makes sense. It was also pointed out to me (offline) > that the above doesn't work as intented for the use-case where I really > need it. The primary reason why I need these "dma-mem" interconnect > paths is so that the memory controller is specified as the "DMA parent" > for these devices, which is important so that the DMA masks can be > correctly set. Having the &emc reference in the first slot breaks that. > Your suggestion makes sense when interpreting the terminology > differently and it fixes the DMA parent issue (at least partially). > >> It should look somewhat like this: >> >> interconnects = >> <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; >> >> interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; This looks better to me. > I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always > going to be the same and it's arbitrarily defined, so it's effectively > useless. But other than that it looks good. Well, in most cases the target would be the EMEM, so that's fine. I have seen that other vendors that may have an additional internal memory, especially dedicated to some DSPs and in such cases the bandwidth needs are different for the two paths (to internal memory and DDR). > I suppose one could argue about the names a bit. Having the "bpmp" > prefix for all of them feels a little redundant. They could also be > [ "read", "write", "dma-read", "dma-write" ], which would make them > a little more contextual, like we do with other clocks. > > However, like I said before, at least one of these would need to be > named "dma-mem" in order for the memory controller to be selected as > the DMA parent. But, perhaps we just need to look at this some other > way and specify an additional way of specifying the DMA parent for > devices that doesn't rely on a "dma-mem" interconnect-names property. > > Perhaps some new dma-parent property that takes a phandle (with perhaps > an optional specifier) would work? I think that may tie in nicely with > the memory controller mini-framework that I had proposed a while ago. > > Rob, any thoughts on that? > > Thierry > >>>>> + interconnect-names = "dma-mem", "dma-mem", "dma-mem", "dma-mem"; >>>>> + >>>>> iommus = <&smmu TEGRA186_SID_BPMP>; >>>>> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_BPMP>; >>>>> shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>; >>>>> >> Thanks, Georgi ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <83d94918-bc01-131b-924c-9750767d3b29-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-21 15:18 ` Georgi Djakov @ 2020-01-21 15:54 ` Thierry Reding -1 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-21 15:54 UTC (permalink / raw) To: Georgi Djakov Cc: Dmitry Osipenko, Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 14389 bytes --] On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: > On 1/21/20 16:10, Thierry Reding wrote: > > On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: > >> 20.01.2020 18:06, Thierry Reding пишет: > >>> On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: > >>>> Hi Thierry, > >>>> > >>>> Thanks for the patch! > >>>> > >>>> On 1/14/20 20:15, Thierry Reding wrote: > >>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > >>>>> > >>>>> Document the interconnects property that is used to describe the paths > >>>>> from and to system memory from and to the BPMP. > >>>>> > >>>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > >>>>> --- > >>>>> Rob, Georgi, > >>>>> > >>>>> after the initial RFC that I did for adding interconnect properties on > >>>>> Tegra, I realized that the description wasn't complete. This is an > >>>>> attempt at a more accurate description, but unfortunately I'm not sure > >>>>> if it's even correct in terms of the interconnect bindings. > >>>>> > >>>>> The problem here is that on Tegra, each device has multiple paths to > >>>>> system memory, and I have no good idea on what to pick as the default. > >>>>> They are all basically the same path, but each provides extra controls > >>>>> to configure the "interconnect". > >>>> > >>>> Are these multiple paths between a device and system memory used simultaneously > >>>> for load-balancing, or who makes the decision about which path would be used? > >>> > >>> It varies. The vast majority of these paths are read/write pairs, which > >>> can be configured separately. There are also cases where multiple paths > >>> are used for load-balancing and I don't think there's any direct > >>> software control over which path will be used. > >>> > >>> A third class is where you have one device, but two read/write pairs, > >>> one which is tied to a microcontroller that's part of the device, and > >>> another read/write pair that is used for DMA to/from the device. > >>> > >>> Often in the latter case, the microcontroller memory client interfaces > >>> will be used by the microcontroller to read firmware and once the micro- > >>> controller has booted up, the DMA memory client interfaces will be used > >>> to read/write system memory with bulk data (like frame buffers, etc.). > >>> > >>>> Is this based on the client/stream ID that you mentioned previously? > >>> > >>> These are now all what's call memory client IDs, which identify the > >>> corresponding interface to the memory controller. Stream IDs are > >>> slightly higher-level and typically identify the "module" that uses > >>> the SMMU. Generally a stream ID is mapped to one or more memory client > >>> IDs. > >>> > >>>> Looking at the the binding below, it seems to me like there are different > >>>> master/slave pairs between MC and EMC and each link is used for > >>>> unidirectional traffic only. In terms of the interconnect API, both read > >>>> and write paths have the same direction. > >> > >> Yes, that definition should be incorrect. > >> > >>> I'm not sure I understand what you mean by this last sentence. Are you > >>> saying that each path in terms of the interconnect API is a always a > >>> bidirectional link? > >> > >> Please see more below. > >> > >>>> Is the EMC really an interconnect provider or is it just a slave port? Can > >>>> we scale both EMC and MC independently? > >>> > >>> The EMC is the only one where we can scale the frequency, but the MC has > >>> various knobs that can be used to fine-tune arbitration, set maximum > >>> latency, etc. > >> > >> Yes.. > >> > >> > >> EMC controls the total amount of available memory bandwidth, things like > >> DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from > >> one side and DRAM (EMEM) from the other. > >> > > Right, so we can use the icc framework here to aggregate the requested bandwidth > from all clients and scale the frequency/voltage of EMC. Yeah, that was the plan. Dmitry's patches implement most of that. Note that we're working on this from two sides: on one hand we need to figure out the bindings so that we can set up the interconnect paths, then the memory controller and external memory controller drivers need to be made interconnect providers so that we can actually implement the dynamic scaling (and then finally all memory client drivers need to be updated to actually use these ICC framework to request bandwidth). I'm prioritizing the first issue right now because that's currently a blocker for enabling SMMU support on Tegra194 where we need to set the DMA mask based on the "bus" (i.e. DMA parent, i.e. the MC) because there are additional restrictions that don't exist on prior generations where we can simply set the DMA mask to the device's "native" DMA mask. EMC frequency scaling is slightly lower on my priority list because in most use-cases there is enough bandwidth at the default EMC frequency. > >> MC controls allocation of that total bandwidth between the memory > >> clients. It has knobs to prioritize clients, the knobs are per > >> read/write port. MC is facing memory clients from one side and EMC from > >> the other. > >> > > Thanks for clarifying! So are these QoS knobs (priority, latency etc.) tuned > dynamically during runtime or is it more like a static configuration that is > done for example just once during system boot? The hardware defaults are usually sufficient unless the system is under very high memory pressure. I'm only aware of one case where we actually need to override the hardware default on boot and it's exotic enough to not be upstream yet. Ultimately we want to tune these at runtime, typically together with and depending on the EMC frequency. Under memory pressure you can use the "latency allowance" registers to prioritize memory requests from isochronous clients (like display controllers) to ensure they don't underrun. Given that we only have to tune these under somewhat extreme conditions, I think these are lower priority from an implementation point of view than the EMC frequency scaling, but the registers are based on the memory client IDs, so I think it's convenient to have those be part of the bindings in the first place. > >>> I vaguely recall Dmitry mentioning that the EMC in early generations of > >>> Tegra used to have controls for individual memory clients, but I don't > >>> see that in more recent generations. > >> > >> EMC doesn't have direct controls over memory clients on all Tegra SoCs, > >> but it may have some extra knobs for the MC arbitration config. > >> > >> The MC bandwidth allocation logic and hardware programming interface > >> differs among SoC generations, but the basic principle is the same. > >> > >>>>> Any ideas on how to resolve this? Let me know if the DT bindings and > >>>>> example don't make things clear enough. > >> > >> I'm also interested in the answer to this question. > >> > >> A quick thought.. maybe it could be some new ICC DT property which tells > >> that all paths are the "dma-mem": > >> > >> interconnects-all-dma-mem; > > > > There could easily be cases where multiple interconnects are to system > > memory but there are additional ones which aren't, so the above wouldn't > > be able to represent such cases. > > Yes, true. > > >>>>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ > >>>>> 1 file changed, 59 insertions(+) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>>>> index dabf1c1aec2f..d40fcd836e90 100644 > >>>>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>>>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>>>> @@ -43,6 +43,24 @@ properties: > >>>>> - enum: > >>>>> - nvidia,tegra186-bpmp > >>>>> > >>>>> + interconnects: > >>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array > >>>>> + description: A list of phandle and specifier pairs that describe the > >>>>> + interconnect paths to and from the BPMP. > >>>>> + > >>>>> + interconnect-names: > >>>>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > >>>>> + description: One string for each pair of phandle and specifier in the > >>>>> + "interconnects" property. > >>>>> + # XXX We need at least one of these to be named dma-mem so that the core > >>>>> + # will set the DMA mask based on the DMA parent, but all of these go to > >>>>> + # system memory eventually. > >>>>> + items: > >>>>> + - const: dma-mem > >>>>> + - const: dma-mem > >>>>> + - const: dma-mem > >>>>> + - const: dma-mem > >> > >> Names should be unique, otherwise it's not possible to retrieve ICC path > >> other than the first one. > > > > Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure > > what else to put there and thought this kinda made it clear that it was > > only half-baked. > > > >>>>> iommus: > >>>>> $ref: /schemas/types.yaml#/definitions/phandle-array > >>>>> description: | > >>>>> @@ -152,8 +170,43 @@ additionalProperties: false > >>>>> > >>>>> examples: > >>>>> - | > >>>>> + #include <dt-bindings/clock/tegra186-clock.h> > >>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> > >>>>> #include <dt-bindings/mailbox/tegra186-hsp.h> > >>>>> + #include <dt-bindings/memory/tegra186-mc.h> > >>>>> + > >>>>> + mc: memory-controller@2c00000 { > >>>>> + compatible = "nvidia,tegra186-mc"; > >>>>> + reg = <0x02c00000 0xb0000>; > >>>>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; > >>>>> + status = "disabled"; > >>>>> + > >>>>> + #interconnect-cells = <1>; > >>>>> + #address-cells = <2>; > >>>>> + #size-cells = <2>; > >>>>> + > >>>>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; > >>>>> + > >>>>> + /* > >>>>> + * Memory clients have access to all 40 bits that the memory > >>>>> + * controller can address. > >>>>> + */ > >>>>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; > >>>>> + > >>>>> + #memory-controller-cells = <0>; > >>>>> + > >>>>> + emc: external-memory-controller@2c60000 { > >>>>> + compatible = "nvidia,tegra186-emc"; > >>>>> + reg = <0x0 0x02c60000 0x0 0x50000>; > >>>>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; > >>>>> + clocks = <&bpmp TEGRA186_CLK_EMC>; > >>>>> + clock-names = "emc"; > >>>>> + > >>>>> + #interconnect-cells = <0>; > >>>>> + > >>>>> + nvidia,bpmp = <&bpmp>; > >>>>> + }; > >>>>> + }; > >>>>> > >>>>> hsp_top0: hsp@3c00000 { > >>>>> compatible = "nvidia,tegra186-hsp"; > >>>>> @@ -187,6 +240,12 @@ examples: > >>>>> > >>>>> bpmp { > >>>>> compatible = "nvidia,tegra186-bpmp"; > >>>>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, > >>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, > >>>>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > >>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; > >> > >> I don't think this is a correct definition of the ICC paths because the > >> first node-MC_ID pair should define the source, second pair is the final > >> destination. Then the interconnect core builds (by itself) the path from > >> MC client to MC and finally to EMC based on the given source / > >> destination. Please see my v1 patchset for the example. > > > > Okay, sounds like "source" in this case means the initiator of the > > transaction and destination is the target of the transaction. I had > > interpreted the "source" as the "source location" of the transaction (so > > for reads the source would be the system memory via the EMC, and for > > writes the source would be the memory client interface). > > Yes, exactly. Maybe it would be more correct to call these pairs > initiator/target or master/slave. Do you want me to extend the bindings documentation to mention these alternative names? > > Yeah, I think that makes sense. It was also pointed out to me (offline) > > that the above doesn't work as intented for the use-case where I really > > need it. The primary reason why I need these "dma-mem" interconnect > > paths is so that the memory controller is specified as the "DMA parent" > > for these devices, which is important so that the DMA masks can be > > correctly set. Having the &emc reference in the first slot breaks that. > > Your suggestion makes sense when interpreting the terminology > > differently and it fixes the DMA parent issue (at least partially). > > > >> It should look somewhat like this: > >> > >> interconnects = > >> <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, > >> <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, > >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, > >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; > >> > >> interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; > > This looks better to me. > > > I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always > > going to be the same and it's arbitrarily defined, so it's effectively > > useless. But other than that it looks good. > > Well, in most cases the target would be the EMEM, so that's fine. I have seen > that other vendors that may have an additional internal memory, especially > dedicated to some DSPs and in such cases the bandwidth needs are different for > the two paths (to internal memory and DDR). Most chips have a small internal memory that can be used, though it seldomly is. However, in that case I would expect the target to be a completely different device, so it'd look more like this: interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, ...; I don't think EMEM has any "downstream" other than external memory. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-21 15:54 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-21 15:54 UTC (permalink / raw) To: Georgi Djakov Cc: Dmitry Osipenko, Rob Herring, Jon Hunter, linux-tegra, devicetree [-- Attachment #1: Type: text/plain, Size: 14331 bytes --] On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: > On 1/21/20 16:10, Thierry Reding wrote: > > On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: > >> 20.01.2020 18:06, Thierry Reding пишет: > >>> On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: > >>>> Hi Thierry, > >>>> > >>>> Thanks for the patch! > >>>> > >>>> On 1/14/20 20:15, Thierry Reding wrote: > >>>>> From: Thierry Reding <treding@nvidia.com> > >>>>> > >>>>> Document the interconnects property that is used to describe the paths > >>>>> from and to system memory from and to the BPMP. > >>>>> > >>>>> Signed-off-by: Thierry Reding <treding@nvidia.com> > >>>>> --- > >>>>> Rob, Georgi, > >>>>> > >>>>> after the initial RFC that I did for adding interconnect properties on > >>>>> Tegra, I realized that the description wasn't complete. This is an > >>>>> attempt at a more accurate description, but unfortunately I'm not sure > >>>>> if it's even correct in terms of the interconnect bindings. > >>>>> > >>>>> The problem here is that on Tegra, each device has multiple paths to > >>>>> system memory, and I have no good idea on what to pick as the default. > >>>>> They are all basically the same path, but each provides extra controls > >>>>> to configure the "interconnect". > >>>> > >>>> Are these multiple paths between a device and system memory used simultaneously > >>>> for load-balancing, or who makes the decision about which path would be used? > >>> > >>> It varies. The vast majority of these paths are read/write pairs, which > >>> can be configured separately. There are also cases where multiple paths > >>> are used for load-balancing and I don't think there's any direct > >>> software control over which path will be used. > >>> > >>> A third class is where you have one device, but two read/write pairs, > >>> one which is tied to a microcontroller that's part of the device, and > >>> another read/write pair that is used for DMA to/from the device. > >>> > >>> Often in the latter case, the microcontroller memory client interfaces > >>> will be used by the microcontroller to read firmware and once the micro- > >>> controller has booted up, the DMA memory client interfaces will be used > >>> to read/write system memory with bulk data (like frame buffers, etc.). > >>> > >>>> Is this based on the client/stream ID that you mentioned previously? > >>> > >>> These are now all what's call memory client IDs, which identify the > >>> corresponding interface to the memory controller. Stream IDs are > >>> slightly higher-level and typically identify the "module" that uses > >>> the SMMU. Generally a stream ID is mapped to one or more memory client > >>> IDs. > >>> > >>>> Looking at the the binding below, it seems to me like there are different > >>>> master/slave pairs between MC and EMC and each link is used for > >>>> unidirectional traffic only. In terms of the interconnect API, both read > >>>> and write paths have the same direction. > >> > >> Yes, that definition should be incorrect. > >> > >>> I'm not sure I understand what you mean by this last sentence. Are you > >>> saying that each path in terms of the interconnect API is a always a > >>> bidirectional link? > >> > >> Please see more below. > >> > >>>> Is the EMC really an interconnect provider or is it just a slave port? Can > >>>> we scale both EMC and MC independently? > >>> > >>> The EMC is the only one where we can scale the frequency, but the MC has > >>> various knobs that can be used to fine-tune arbitration, set maximum > >>> latency, etc. > >> > >> Yes.. > >> > >> > >> EMC controls the total amount of available memory bandwidth, things like > >> DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from > >> one side and DRAM (EMEM) from the other. > >> > > Right, so we can use the icc framework here to aggregate the requested bandwidth > from all clients and scale the frequency/voltage of EMC. Yeah, that was the plan. Dmitry's patches implement most of that. Note that we're working on this from two sides: on one hand we need to figure out the bindings so that we can set up the interconnect paths, then the memory controller and external memory controller drivers need to be made interconnect providers so that we can actually implement the dynamic scaling (and then finally all memory client drivers need to be updated to actually use these ICC framework to request bandwidth). I'm prioritizing the first issue right now because that's currently a blocker for enabling SMMU support on Tegra194 where we need to set the DMA mask based on the "bus" (i.e. DMA parent, i.e. the MC) because there are additional restrictions that don't exist on prior generations where we can simply set the DMA mask to the device's "native" DMA mask. EMC frequency scaling is slightly lower on my priority list because in most use-cases there is enough bandwidth at the default EMC frequency. > >> MC controls allocation of that total bandwidth between the memory > >> clients. It has knobs to prioritize clients, the knobs are per > >> read/write port. MC is facing memory clients from one side and EMC from > >> the other. > >> > > Thanks for clarifying! So are these QoS knobs (priority, latency etc.) tuned > dynamically during runtime or is it more like a static configuration that is > done for example just once during system boot? The hardware defaults are usually sufficient unless the system is under very high memory pressure. I'm only aware of one case where we actually need to override the hardware default on boot and it's exotic enough to not be upstream yet. Ultimately we want to tune these at runtime, typically together with and depending on the EMC frequency. Under memory pressure you can use the "latency allowance" registers to prioritize memory requests from isochronous clients (like display controllers) to ensure they don't underrun. Given that we only have to tune these under somewhat extreme conditions, I think these are lower priority from an implementation point of view than the EMC frequency scaling, but the registers are based on the memory client IDs, so I think it's convenient to have those be part of the bindings in the first place. > >>> I vaguely recall Dmitry mentioning that the EMC in early generations of > >>> Tegra used to have controls for individual memory clients, but I don't > >>> see that in more recent generations. > >> > >> EMC doesn't have direct controls over memory clients on all Tegra SoCs, > >> but it may have some extra knobs for the MC arbitration config. > >> > >> The MC bandwidth allocation logic and hardware programming interface > >> differs among SoC generations, but the basic principle is the same. > >> > >>>>> Any ideas on how to resolve this? Let me know if the DT bindings and > >>>>> example don't make things clear enough. > >> > >> I'm also interested in the answer to this question. > >> > >> A quick thought.. maybe it could be some new ICC DT property which tells > >> that all paths are the "dma-mem": > >> > >> interconnects-all-dma-mem; > > > > There could easily be cases where multiple interconnects are to system > > memory but there are additional ones which aren't, so the above wouldn't > > be able to represent such cases. > > Yes, true. > > >>>>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ > >>>>> 1 file changed, 59 insertions(+) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>>>> index dabf1c1aec2f..d40fcd836e90 100644 > >>>>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>>>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > >>>>> @@ -43,6 +43,24 @@ properties: > >>>>> - enum: > >>>>> - nvidia,tegra186-bpmp > >>>>> > >>>>> + interconnects: > >>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array > >>>>> + description: A list of phandle and specifier pairs that describe the > >>>>> + interconnect paths to and from the BPMP. > >>>>> + > >>>>> + interconnect-names: > >>>>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > >>>>> + description: One string for each pair of phandle and specifier in the > >>>>> + "interconnects" property. > >>>>> + # XXX We need at least one of these to be named dma-mem so that the core > >>>>> + # will set the DMA mask based on the DMA parent, but all of these go to > >>>>> + # system memory eventually. > >>>>> + items: > >>>>> + - const: dma-mem > >>>>> + - const: dma-mem > >>>>> + - const: dma-mem > >>>>> + - const: dma-mem > >> > >> Names should be unique, otherwise it's not possible to retrieve ICC path > >> other than the first one. > > > > Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure > > what else to put there and thought this kinda made it clear that it was > > only half-baked. > > > >>>>> iommus: > >>>>> $ref: /schemas/types.yaml#/definitions/phandle-array > >>>>> description: | > >>>>> @@ -152,8 +170,43 @@ additionalProperties: false > >>>>> > >>>>> examples: > >>>>> - | > >>>>> + #include <dt-bindings/clock/tegra186-clock.h> > >>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> > >>>>> #include <dt-bindings/mailbox/tegra186-hsp.h> > >>>>> + #include <dt-bindings/memory/tegra186-mc.h> > >>>>> + > >>>>> + mc: memory-controller@2c00000 { > >>>>> + compatible = "nvidia,tegra186-mc"; > >>>>> + reg = <0x02c00000 0xb0000>; > >>>>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; > >>>>> + status = "disabled"; > >>>>> + > >>>>> + #interconnect-cells = <1>; > >>>>> + #address-cells = <2>; > >>>>> + #size-cells = <2>; > >>>>> + > >>>>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; > >>>>> + > >>>>> + /* > >>>>> + * Memory clients have access to all 40 bits that the memory > >>>>> + * controller can address. > >>>>> + */ > >>>>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; > >>>>> + > >>>>> + #memory-controller-cells = <0>; > >>>>> + > >>>>> + emc: external-memory-controller@2c60000 { > >>>>> + compatible = "nvidia,tegra186-emc"; > >>>>> + reg = <0x0 0x02c60000 0x0 0x50000>; > >>>>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; > >>>>> + clocks = <&bpmp TEGRA186_CLK_EMC>; > >>>>> + clock-names = "emc"; > >>>>> + > >>>>> + #interconnect-cells = <0>; > >>>>> + > >>>>> + nvidia,bpmp = <&bpmp>; > >>>>> + }; > >>>>> + }; > >>>>> > >>>>> hsp_top0: hsp@3c00000 { > >>>>> compatible = "nvidia,tegra186-hsp"; > >>>>> @@ -187,6 +240,12 @@ examples: > >>>>> > >>>>> bpmp { > >>>>> compatible = "nvidia,tegra186-bpmp"; > >>>>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, > >>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, > >>>>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > >>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; > >> > >> I don't think this is a correct definition of the ICC paths because the > >> first node-MC_ID pair should define the source, second pair is the final > >> destination. Then the interconnect core builds (by itself) the path from > >> MC client to MC and finally to EMC based on the given source / > >> destination. Please see my v1 patchset for the example. > > > > Okay, sounds like "source" in this case means the initiator of the > > transaction and destination is the target of the transaction. I had > > interpreted the "source" as the "source location" of the transaction (so > > for reads the source would be the system memory via the EMC, and for > > writes the source would be the memory client interface). > > Yes, exactly. Maybe it would be more correct to call these pairs > initiator/target or master/slave. Do you want me to extend the bindings documentation to mention these alternative names? > > Yeah, I think that makes sense. It was also pointed out to me (offline) > > that the above doesn't work as intented for the use-case where I really > > need it. The primary reason why I need these "dma-mem" interconnect > > paths is so that the memory controller is specified as the "DMA parent" > > for these devices, which is important so that the DMA masks can be > > correctly set. Having the &emc reference in the first slot breaks that. > > Your suggestion makes sense when interpreting the terminology > > differently and it fixes the DMA parent issue (at least partially). > > > >> It should look somewhat like this: > >> > >> interconnects = > >> <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, > >> <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, > >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, > >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; > >> > >> interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; > > This looks better to me. > > > I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always > > going to be the same and it's arbitrarily defined, so it's effectively > > useless. But other than that it looks good. > > Well, in most cases the target would be the EMEM, so that's fine. I have seen > that other vendors that may have an additional internal memory, especially > dedicated to some DSPs and in such cases the bandwidth needs are different for > the two paths (to internal memory and DDR). Most chips have a small internal memory that can be used, though it seldomly is. However, in that case I would expect the target to be a completely different device, so it'd look more like this: interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, ...; I don't think EMEM has any "downstream" other than external memory. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-21 15:54 ` Thierry Reding @ 2020-01-21 20:12 ` Dmitry Osipenko -1 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-21 20:12 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 21.01.2020 18:54, Thierry Reding пишет: > On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >> On 1/21/20 16:10, Thierry Reding wrote: >>> On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: >>>> 20.01.2020 18:06, Thierry Reding пишет: >>>>> On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: >>>>>> Hi Thierry, >>>>>> >>>>>> Thanks for the patch! >>>>>> >>>>>> On 1/14/20 20:15, Thierry Reding wrote: >>>>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>>>>>> >>>>>>> Document the interconnects property that is used to describe the paths >>>>>>> from and to system memory from and to the BPMP. >>>>>>> >>>>>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>>>>>> --- >>>>>>> Rob, Georgi, >>>>>>> >>>>>>> after the initial RFC that I did for adding interconnect properties on >>>>>>> Tegra, I realized that the description wasn't complete. This is an >>>>>>> attempt at a more accurate description, but unfortunately I'm not sure >>>>>>> if it's even correct in terms of the interconnect bindings. >>>>>>> >>>>>>> The problem here is that on Tegra, each device has multiple paths to >>>>>>> system memory, and I have no good idea on what to pick as the default. >>>>>>> They are all basically the same path, but each provides extra controls >>>>>>> to configure the "interconnect". >>>>>> >>>>>> Are these multiple paths between a device and system memory used simultaneously >>>>>> for load-balancing, or who makes the decision about which path would be used? >>>>> >>>>> It varies. The vast majority of these paths are read/write pairs, which >>>>> can be configured separately. There are also cases where multiple paths >>>>> are used for load-balancing and I don't think there's any direct >>>>> software control over which path will be used. >>>>> >>>>> A third class is where you have one device, but two read/write pairs, >>>>> one which is tied to a microcontroller that's part of the device, and >>>>> another read/write pair that is used for DMA to/from the device. >>>>> >>>>> Often in the latter case, the microcontroller memory client interfaces >>>>> will be used by the microcontroller to read firmware and once the micro- >>>>> controller has booted up, the DMA memory client interfaces will be used >>>>> to read/write system memory with bulk data (like frame buffers, etc.). >>>>> >>>>>> Is this based on the client/stream ID that you mentioned previously? >>>>> >>>>> These are now all what's call memory client IDs, which identify the >>>>> corresponding interface to the memory controller. Stream IDs are >>>>> slightly higher-level and typically identify the "module" that uses >>>>> the SMMU. Generally a stream ID is mapped to one or more memory client >>>>> IDs. >>>>> >>>>>> Looking at the the binding below, it seems to me like there are different >>>>>> master/slave pairs between MC and EMC and each link is used for >>>>>> unidirectional traffic only. In terms of the interconnect API, both read >>>>>> and write paths have the same direction. >>>> >>>> Yes, that definition should be incorrect. >>>> >>>>> I'm not sure I understand what you mean by this last sentence. Are you >>>>> saying that each path in terms of the interconnect API is a always a >>>>> bidirectional link? >>>> >>>> Please see more below. >>>> >>>>>> Is the EMC really an interconnect provider or is it just a slave port? Can >>>>>> we scale both EMC and MC independently? >>>>> >>>>> The EMC is the only one where we can scale the frequency, but the MC has >>>>> various knobs that can be used to fine-tune arbitration, set maximum >>>>> latency, etc. >>>> >>>> Yes.. >>>> >>>> >>>> EMC controls the total amount of available memory bandwidth, things like >>>> DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from >>>> one side and DRAM (EMEM) from the other. >>>> >> >> Right, so we can use the icc framework here to aggregate the requested bandwidth >> from all clients and scale the frequency/voltage of EMC. > > Yeah, that was the plan. Dmitry's patches implement most of that. Note > that we're working on this from two sides: on one hand we need to figure > out the bindings so that we can set up the interconnect paths, then the > memory controller and external memory controller drivers need to be made > interconnect providers so that we can actually implement the dynamic > scaling (and then finally all memory client drivers need to be updated > to actually use these ICC framework to request bandwidth). > > I'm prioritizing the first issue right now because that's currently a > blocker for enabling SMMU support on Tegra194 where we need to set the > DMA mask based on the "bus" (i.e. DMA parent, i.e. the MC) because there > are additional restrictions that don't exist on prior generations where > we can simply set the DMA mask to the device's "native" DMA mask. > > EMC frequency scaling is slightly lower on my priority list because in > most use-cases there is enough bandwidth at the default EMC frequency. > >>>> MC controls allocation of that total bandwidth between the memory >>>> clients. It has knobs to prioritize clients, the knobs are per >>>> read/write port. MC is facing memory clients from one side and EMC from >>>> the other. >>>> >> >> Thanks for clarifying! So are these QoS knobs (priority, latency etc.) tuned >> dynamically during runtime or is it more like a static configuration that is >> done for example just once during system boot? > > The hardware defaults are usually sufficient unless the system is under > very high memory pressure. I'm only aware of one case where we actually > need to override the hardware default on boot and it's exotic enough to > not be upstream yet. > > Ultimately we want to tune these at runtime, typically together with and > depending on the EMC frequency. Under memory pressure you can use the > "latency allowance" registers to prioritize memory requests from > isochronous clients (like display controllers) to ensure they don't > underrun. Perhaps USB could be one example of a memory client that may need ISO transfers for multimedia things (live audio/video), while ISO not needed for file transfers. > Given that we only have to tune these under somewhat extreme conditions, > I think these are lower priority from an implementation point of view > than the EMC frequency scaling, but the registers are based on the > memory client IDs, so I think it's convenient to have those be part of > the bindings in the first place. > >>>>> I vaguely recall Dmitry mentioning that the EMC in early generations of >>>>> Tegra used to have controls for individual memory clients, but I don't >>>>> see that in more recent generations. >>>> >>>> EMC doesn't have direct controls over memory clients on all Tegra SoCs, >>>> but it may have some extra knobs for the MC arbitration config. >>>> >>>> The MC bandwidth allocation logic and hardware programming interface >>>> differs among SoC generations, but the basic principle is the same. >>>> >>>>>>> Any ideas on how to resolve this? Let me know if the DT bindings and >>>>>>> example don't make things clear enough. >>>> >>>> I'm also interested in the answer to this question. >>>> >>>> A quick thought.. maybe it could be some new ICC DT property which tells >>>> that all paths are the "dma-mem": >>>> >>>> interconnects-all-dma-mem; >>> >>> There could easily be cases where multiple interconnects are to system >>> memory but there are additional ones which aren't, so the above wouldn't >>> be able to represent such cases. >> >> Yes, true. Sure, but then that property shouldn't be used. Anyways, I think Thierry's suggestion about the generic memory controller API sounds more attractive. >>>>>>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ >>>>>>> 1 file changed, 59 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>> index dabf1c1aec2f..d40fcd836e90 100644 >>>>>>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>> @@ -43,6 +43,24 @@ properties: >>>>>>> - enum: >>>>>>> - nvidia,tegra186-bpmp >>>>>>> >>>>>>> + interconnects: >>>>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>>> + description: A list of phandle and specifier pairs that describe the >>>>>>> + interconnect paths to and from the BPMP. >>>>>>> + >>>>>>> + interconnect-names: >>>>>>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>>>>>> + description: One string for each pair of phandle and specifier in the >>>>>>> + "interconnects" property. >>>>>>> + # XXX We need at least one of these to be named dma-mem so that the core >>>>>>> + # will set the DMA mask based on the DMA parent, but all of these go to >>>>>>> + # system memory eventually. >>>>>>> + items: >>>>>>> + - const: dma-mem >>>>>>> + - const: dma-mem >>>>>>> + - const: dma-mem >>>>>>> + - const: dma-mem >>>> >>>> Names should be unique, otherwise it's not possible to retrieve ICC path >>>> other than the first one. >>> >>> Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure >>> what else to put there and thought this kinda made it clear that it was >>> only half-baked. >>> >>>>>>> iommus: >>>>>>> $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>>> description: | >>>>>>> @@ -152,8 +170,43 @@ additionalProperties: false >>>>>>> >>>>>>> examples: >>>>>>> - | >>>>>>> + #include <dt-bindings/clock/tegra186-clock.h> >>>>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>>>> #include <dt-bindings/mailbox/tegra186-hsp.h> >>>>>>> + #include <dt-bindings/memory/tegra186-mc.h> >>>>>>> + >>>>>>> + mc: memory-controller@2c00000 { >>>>>>> + compatible = "nvidia,tegra186-mc"; >>>>>>> + reg = <0x02c00000 0xb0000>; >>>>>>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; >>>>>>> + status = "disabled"; >>>>>>> + >>>>>>> + #interconnect-cells = <1>; >>>>>>> + #address-cells = <2>; >>>>>>> + #size-cells = <2>; >>>>>>> + >>>>>>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; >>>>>>> + >>>>>>> + /* >>>>>>> + * Memory clients have access to all 40 bits that the memory >>>>>>> + * controller can address. >>>>>>> + */ >>>>>>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; >>>>>>> + >>>>>>> + #memory-controller-cells = <0>; >>>>>>> + >>>>>>> + emc: external-memory-controller@2c60000 { >>>>>>> + compatible = "nvidia,tegra186-emc"; >>>>>>> + reg = <0x0 0x02c60000 0x0 0x50000>; >>>>>>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; >>>>>>> + clocks = <&bpmp TEGRA186_CLK_EMC>; >>>>>>> + clock-names = "emc"; >>>>>>> + >>>>>>> + #interconnect-cells = <0>; >>>>>>> + >>>>>>> + nvidia,bpmp = <&bpmp>; >>>>>>> + }; >>>>>>> + }; >>>>>>> >>>>>>> hsp_top0: hsp@3c00000 { >>>>>>> compatible = "nvidia,tegra186-hsp"; >>>>>>> @@ -187,6 +240,12 @@ examples: >>>>>>> >>>>>>> bpmp { >>>>>>> compatible = "nvidia,tegra186-bpmp"; >>>>>>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, >>>>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, >>>>>>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >>>>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; >>>> >>>> I don't think this is a correct definition of the ICC paths because the >>>> first node-MC_ID pair should define the source, second pair is the final >>>> destination. Then the interconnect core builds (by itself) the path from >>>> MC client to MC and finally to EMC based on the given source / >>>> destination. Please see my v1 patchset for the example. >>> >>> Okay, sounds like "source" in this case means the initiator of the >>> transaction and destination is the target of the transaction. I had >>> interpreted the "source" as the "source location" of the transaction (so >>> for reads the source would be the system memory via the EMC, and for >>> writes the source would be the memory client interface). >> >> Yes, exactly. Maybe it would be more correct to call these pairs >> initiator/target or master/slave. > > Do you want me to extend the bindings documentation to mention these > alternative names? > >>> Yeah, I think that makes sense. It was also pointed out to me (offline) >>> that the above doesn't work as intented for the use-case where I really >>> need it. The primary reason why I need these "dma-mem" interconnect >>> paths is so that the memory controller is specified as the "DMA parent" >>> for these devices, which is important so that the DMA masks can be >>> correctly set. Having the &emc reference in the first slot breaks that. >>> Your suggestion makes sense when interpreting the terminology >>> differently and it fixes the DMA parent issue (at least partially). >>> >>>> It should look somewhat like this: >>>> >>>> interconnects = >>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, >>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, >>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, >>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; >>>> >>>> interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; >> >> This looks better to me. >> >>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>> going to be the same and it's arbitrarily defined, so it's effectively >>> useless. But other than that it looks good. >> >> Well, in most cases the target would be the EMEM, so that's fine. I have seen >> that other vendors that may have an additional internal memory, especially >> dedicated to some DSPs and in such cases the bandwidth needs are different for >> the two paths (to internal memory and DDR). > > Most chips have a small internal memory that can be used, though it > seldomly is. However, in that case I would expect the target to be a > completely different device, so it'd look more like this: > > interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, > ...; > > I don't think EMEM has any "downstream" other than external memory. The node ID should be mandatory in terms of interconnect, even if it's a single node. EMC (provider) != EMEM (endpoint). ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-21 20:12 ` Dmitry Osipenko 0 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-21 20:12 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra, devicetree 21.01.2020 18:54, Thierry Reding пишет: > On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >> On 1/21/20 16:10, Thierry Reding wrote: >>> On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: >>>> 20.01.2020 18:06, Thierry Reding пишет: >>>>> On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: >>>>>> Hi Thierry, >>>>>> >>>>>> Thanks for the patch! >>>>>> >>>>>> On 1/14/20 20:15, Thierry Reding wrote: >>>>>>> From: Thierry Reding <treding@nvidia.com> >>>>>>> >>>>>>> Document the interconnects property that is used to describe the paths >>>>>>> from and to system memory from and to the BPMP. >>>>>>> >>>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>>>>>> --- >>>>>>> Rob, Georgi, >>>>>>> >>>>>>> after the initial RFC that I did for adding interconnect properties on >>>>>>> Tegra, I realized that the description wasn't complete. This is an >>>>>>> attempt at a more accurate description, but unfortunately I'm not sure >>>>>>> if it's even correct in terms of the interconnect bindings. >>>>>>> >>>>>>> The problem here is that on Tegra, each device has multiple paths to >>>>>>> system memory, and I have no good idea on what to pick as the default. >>>>>>> They are all basically the same path, but each provides extra controls >>>>>>> to configure the "interconnect". >>>>>> >>>>>> Are these multiple paths between a device and system memory used simultaneously >>>>>> for load-balancing, or who makes the decision about which path would be used? >>>>> >>>>> It varies. The vast majority of these paths are read/write pairs, which >>>>> can be configured separately. There are also cases where multiple paths >>>>> are used for load-balancing and I don't think there's any direct >>>>> software control over which path will be used. >>>>> >>>>> A third class is where you have one device, but two read/write pairs, >>>>> one which is tied to a microcontroller that's part of the device, and >>>>> another read/write pair that is used for DMA to/from the device. >>>>> >>>>> Often in the latter case, the microcontroller memory client interfaces >>>>> will be used by the microcontroller to read firmware and once the micro- >>>>> controller has booted up, the DMA memory client interfaces will be used >>>>> to read/write system memory with bulk data (like frame buffers, etc.). >>>>> >>>>>> Is this based on the client/stream ID that you mentioned previously? >>>>> >>>>> These are now all what's call memory client IDs, which identify the >>>>> corresponding interface to the memory controller. Stream IDs are >>>>> slightly higher-level and typically identify the "module" that uses >>>>> the SMMU. Generally a stream ID is mapped to one or more memory client >>>>> IDs. >>>>> >>>>>> Looking at the the binding below, it seems to me like there are different >>>>>> master/slave pairs between MC and EMC and each link is used for >>>>>> unidirectional traffic only. In terms of the interconnect API, both read >>>>>> and write paths have the same direction. >>>> >>>> Yes, that definition should be incorrect. >>>> >>>>> I'm not sure I understand what you mean by this last sentence. Are you >>>>> saying that each path in terms of the interconnect API is a always a >>>>> bidirectional link? >>>> >>>> Please see more below. >>>> >>>>>> Is the EMC really an interconnect provider or is it just a slave port? Can >>>>>> we scale both EMC and MC independently? >>>>> >>>>> The EMC is the only one where we can scale the frequency, but the MC has >>>>> various knobs that can be used to fine-tune arbitration, set maximum >>>>> latency, etc. >>>> >>>> Yes.. >>>> >>>> >>>> EMC controls the total amount of available memory bandwidth, things like >>>> DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from >>>> one side and DRAM (EMEM) from the other. >>>> >> >> Right, so we can use the icc framework here to aggregate the requested bandwidth >> from all clients and scale the frequency/voltage of EMC. > > Yeah, that was the plan. Dmitry's patches implement most of that. Note > that we're working on this from two sides: on one hand we need to figure > out the bindings so that we can set up the interconnect paths, then the > memory controller and external memory controller drivers need to be made > interconnect providers so that we can actually implement the dynamic > scaling (and then finally all memory client drivers need to be updated > to actually use these ICC framework to request bandwidth). > > I'm prioritizing the first issue right now because that's currently a > blocker for enabling SMMU support on Tegra194 where we need to set the > DMA mask based on the "bus" (i.e. DMA parent, i.e. the MC) because there > are additional restrictions that don't exist on prior generations where > we can simply set the DMA mask to the device's "native" DMA mask. > > EMC frequency scaling is slightly lower on my priority list because in > most use-cases there is enough bandwidth at the default EMC frequency. > >>>> MC controls allocation of that total bandwidth between the memory >>>> clients. It has knobs to prioritize clients, the knobs are per >>>> read/write port. MC is facing memory clients from one side and EMC from >>>> the other. >>>> >> >> Thanks for clarifying! So are these QoS knobs (priority, latency etc.) tuned >> dynamically during runtime or is it more like a static configuration that is >> done for example just once during system boot? > > The hardware defaults are usually sufficient unless the system is under > very high memory pressure. I'm only aware of one case where we actually > need to override the hardware default on boot and it's exotic enough to > not be upstream yet. > > Ultimately we want to tune these at runtime, typically together with and > depending on the EMC frequency. Under memory pressure you can use the > "latency allowance" registers to prioritize memory requests from > isochronous clients (like display controllers) to ensure they don't > underrun. Perhaps USB could be one example of a memory client that may need ISO transfers for multimedia things (live audio/video), while ISO not needed for file transfers. > Given that we only have to tune these under somewhat extreme conditions, > I think these are lower priority from an implementation point of view > than the EMC frequency scaling, but the registers are based on the > memory client IDs, so I think it's convenient to have those be part of > the bindings in the first place. > >>>>> I vaguely recall Dmitry mentioning that the EMC in early generations of >>>>> Tegra used to have controls for individual memory clients, but I don't >>>>> see that in more recent generations. >>>> >>>> EMC doesn't have direct controls over memory clients on all Tegra SoCs, >>>> but it may have some extra knobs for the MC arbitration config. >>>> >>>> The MC bandwidth allocation logic and hardware programming interface >>>> differs among SoC generations, but the basic principle is the same. >>>> >>>>>>> Any ideas on how to resolve this? Let me know if the DT bindings and >>>>>>> example don't make things clear enough. >>>> >>>> I'm also interested in the answer to this question. >>>> >>>> A quick thought.. maybe it could be some new ICC DT property which tells >>>> that all paths are the "dma-mem": >>>> >>>> interconnects-all-dma-mem; >>> >>> There could easily be cases where multiple interconnects are to system >>> memory but there are additional ones which aren't, so the above wouldn't >>> be able to represent such cases. >> >> Yes, true. Sure, but then that property shouldn't be used. Anyways, I think Thierry's suggestion about the generic memory controller API sounds more attractive. >>>>>>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ >>>>>>> 1 file changed, 59 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>> index dabf1c1aec2f..d40fcd836e90 100644 >>>>>>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>> @@ -43,6 +43,24 @@ properties: >>>>>>> - enum: >>>>>>> - nvidia,tegra186-bpmp >>>>>>> >>>>>>> + interconnects: >>>>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>>> + description: A list of phandle and specifier pairs that describe the >>>>>>> + interconnect paths to and from the BPMP. >>>>>>> + >>>>>>> + interconnect-names: >>>>>>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>>>>>> + description: One string for each pair of phandle and specifier in the >>>>>>> + "interconnects" property. >>>>>>> + # XXX We need at least one of these to be named dma-mem so that the core >>>>>>> + # will set the DMA mask based on the DMA parent, but all of these go to >>>>>>> + # system memory eventually. >>>>>>> + items: >>>>>>> + - const: dma-mem >>>>>>> + - const: dma-mem >>>>>>> + - const: dma-mem >>>>>>> + - const: dma-mem >>>> >>>> Names should be unique, otherwise it's not possible to retrieve ICC path >>>> other than the first one. >>> >>> Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure >>> what else to put there and thought this kinda made it clear that it was >>> only half-baked. >>> >>>>>>> iommus: >>>>>>> $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>>> description: | >>>>>>> @@ -152,8 +170,43 @@ additionalProperties: false >>>>>>> >>>>>>> examples: >>>>>>> - | >>>>>>> + #include <dt-bindings/clock/tegra186-clock.h> >>>>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>>>> #include <dt-bindings/mailbox/tegra186-hsp.h> >>>>>>> + #include <dt-bindings/memory/tegra186-mc.h> >>>>>>> + >>>>>>> + mc: memory-controller@2c00000 { >>>>>>> + compatible = "nvidia,tegra186-mc"; >>>>>>> + reg = <0x02c00000 0xb0000>; >>>>>>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; >>>>>>> + status = "disabled"; >>>>>>> + >>>>>>> + #interconnect-cells = <1>; >>>>>>> + #address-cells = <2>; >>>>>>> + #size-cells = <2>; >>>>>>> + >>>>>>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; >>>>>>> + >>>>>>> + /* >>>>>>> + * Memory clients have access to all 40 bits that the memory >>>>>>> + * controller can address. >>>>>>> + */ >>>>>>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; >>>>>>> + >>>>>>> + #memory-controller-cells = <0>; >>>>>>> + >>>>>>> + emc: external-memory-controller@2c60000 { >>>>>>> + compatible = "nvidia,tegra186-emc"; >>>>>>> + reg = <0x0 0x02c60000 0x0 0x50000>; >>>>>>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; >>>>>>> + clocks = <&bpmp TEGRA186_CLK_EMC>; >>>>>>> + clock-names = "emc"; >>>>>>> + >>>>>>> + #interconnect-cells = <0>; >>>>>>> + >>>>>>> + nvidia,bpmp = <&bpmp>; >>>>>>> + }; >>>>>>> + }; >>>>>>> >>>>>>> hsp_top0: hsp@3c00000 { >>>>>>> compatible = "nvidia,tegra186-hsp"; >>>>>>> @@ -187,6 +240,12 @@ examples: >>>>>>> >>>>>>> bpmp { >>>>>>> compatible = "nvidia,tegra186-bpmp"; >>>>>>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, >>>>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, >>>>>>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >>>>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; >>>> >>>> I don't think this is a correct definition of the ICC paths because the >>>> first node-MC_ID pair should define the source, second pair is the final >>>> destination. Then the interconnect core builds (by itself) the path from >>>> MC client to MC and finally to EMC based on the given source / >>>> destination. Please see my v1 patchset for the example. >>> >>> Okay, sounds like "source" in this case means the initiator of the >>> transaction and destination is the target of the transaction. I had >>> interpreted the "source" as the "source location" of the transaction (so >>> for reads the source would be the system memory via the EMC, and for >>> writes the source would be the memory client interface). >> >> Yes, exactly. Maybe it would be more correct to call these pairs >> initiator/target or master/slave. > > Do you want me to extend the bindings documentation to mention these > alternative names? > >>> Yeah, I think that makes sense. It was also pointed out to me (offline) >>> that the above doesn't work as intented for the use-case where I really >>> need it. The primary reason why I need these "dma-mem" interconnect >>> paths is so that the memory controller is specified as the "DMA parent" >>> for these devices, which is important so that the DMA masks can be >>> correctly set. Having the &emc reference in the first slot breaks that. >>> Your suggestion makes sense when interpreting the terminology >>> differently and it fixes the DMA parent issue (at least partially). >>> >>>> It should look somewhat like this: >>>> >>>> interconnects = >>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, >>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, >>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, >>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; >>>> >>>> interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; >> >> This looks better to me. >> >>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>> going to be the same and it's arbitrarily defined, so it's effectively >>> useless. But other than that it looks good. >> >> Well, in most cases the target would be the EMEM, so that's fine. I have seen >> that other vendors that may have an additional internal memory, especially >> dedicated to some DSPs and in such cases the bandwidth needs are different for >> the two paths (to internal memory and DDR). > > Most chips have a small internal memory that can be used, though it > seldomly is. However, in that case I would expect the target to be a > completely different device, so it'd look more like this: > > interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, > ...; > > I don't think EMEM has any "downstream" other than external memory. The node ID should be mandatory in terms of interconnect, even if it's a single node. EMC (provider) != EMEM (endpoint). ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <ffc22502-0e7e-522c-543d-0e74cc25f4b1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-21 20:12 ` Dmitry Osipenko @ 2020-01-26 21:56 ` Dmitry Osipenko -1 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-26 21:56 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 21.01.2020 23:12, Dmitry Osipenko пишет: > 21.01.2020 18:54, Thierry Reding пишет: >> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>> On 1/21/20 16:10, Thierry Reding wrote: >>>> On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: >>>>> 20.01.2020 18:06, Thierry Reding пишет: >>>>>> On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: >>>>>>> Hi Thierry, >>>>>>> >>>>>>> Thanks for the patch! >>>>>>> >>>>>>> On 1/14/20 20:15, Thierry Reding wrote: >>>>>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>>>>>>> >>>>>>>> Document the interconnects property that is used to describe the paths >>>>>>>> from and to system memory from and to the BPMP. >>>>>>>> >>>>>>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>>>>>>> --- >>>>>>>> Rob, Georgi, >>>>>>>> >>>>>>>> after the initial RFC that I did for adding interconnect properties on >>>>>>>> Tegra, I realized that the description wasn't complete. This is an >>>>>>>> attempt at a more accurate description, but unfortunately I'm not sure >>>>>>>> if it's even correct in terms of the interconnect bindings. >>>>>>>> >>>>>>>> The problem here is that on Tegra, each device has multiple paths to >>>>>>>> system memory, and I have no good idea on what to pick as the default. >>>>>>>> They are all basically the same path, but each provides extra controls >>>>>>>> to configure the "interconnect". >>>>>>> >>>>>>> Are these multiple paths between a device and system memory used simultaneously >>>>>>> for load-balancing, or who makes the decision about which path would be used? >>>>>> >>>>>> It varies. The vast majority of these paths are read/write pairs, which >>>>>> can be configured separately. There are also cases where multiple paths >>>>>> are used for load-balancing and I don't think there's any direct >>>>>> software control over which path will be used. >>>>>> >>>>>> A third class is where you have one device, but two read/write pairs, >>>>>> one which is tied to a microcontroller that's part of the device, and >>>>>> another read/write pair that is used for DMA to/from the device. >>>>>> >>>>>> Often in the latter case, the microcontroller memory client interfaces >>>>>> will be used by the microcontroller to read firmware and once the micro- >>>>>> controller has booted up, the DMA memory client interfaces will be used >>>>>> to read/write system memory with bulk data (like frame buffers, etc.). >>>>>> >>>>>>> Is this based on the client/stream ID that you mentioned previously? >>>>>> >>>>>> These are now all what's call memory client IDs, which identify the >>>>>> corresponding interface to the memory controller. Stream IDs are >>>>>> slightly higher-level and typically identify the "module" that uses >>>>>> the SMMU. Generally a stream ID is mapped to one or more memory client >>>>>> IDs. >>>>>> >>>>>>> Looking at the the binding below, it seems to me like there are different >>>>>>> master/slave pairs between MC and EMC and each link is used for >>>>>>> unidirectional traffic only. In terms of the interconnect API, both read >>>>>>> and write paths have the same direction. >>>>> >>>>> Yes, that definition should be incorrect. >>>>> >>>>>> I'm not sure I understand what you mean by this last sentence. Are you >>>>>> saying that each path in terms of the interconnect API is a always a >>>>>> bidirectional link? >>>>> >>>>> Please see more below. >>>>> >>>>>>> Is the EMC really an interconnect provider or is it just a slave port? Can >>>>>>> we scale both EMC and MC independently? >>>>>> >>>>>> The EMC is the only one where we can scale the frequency, but the MC has >>>>>> various knobs that can be used to fine-tune arbitration, set maximum >>>>>> latency, etc. >>>>> >>>>> Yes.. >>>>> >>>>> >>>>> EMC controls the total amount of available memory bandwidth, things like >>>>> DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from >>>>> one side and DRAM (EMEM) from the other. >>>>> >>> >>> Right, so we can use the icc framework here to aggregate the requested bandwidth >>> from all clients and scale the frequency/voltage of EMC. >> >> Yeah, that was the plan. Dmitry's patches implement most of that. Note >> that we're working on this from two sides: on one hand we need to figure >> out the bindings so that we can set up the interconnect paths, then the >> memory controller and external memory controller drivers need to be made >> interconnect providers so that we can actually implement the dynamic >> scaling (and then finally all memory client drivers need to be updated >> to actually use these ICC framework to request bandwidth). >> >> I'm prioritizing the first issue right now because that's currently a >> blocker for enabling SMMU support on Tegra194 where we need to set the >> DMA mask based on the "bus" (i.e. DMA parent, i.e. the MC) because there >> are additional restrictions that don't exist on prior generations where >> we can simply set the DMA mask to the device's "native" DMA mask. >> >> EMC frequency scaling is slightly lower on my priority list because in >> most use-cases there is enough bandwidth at the default EMC frequency. >> >>>>> MC controls allocation of that total bandwidth between the memory >>>>> clients. It has knobs to prioritize clients, the knobs are per >>>>> read/write port. MC is facing memory clients from one side and EMC from >>>>> the other. >>>>> >>> >>> Thanks for clarifying! So are these QoS knobs (priority, latency etc.) tuned >>> dynamically during runtime or is it more like a static configuration that is >>> done for example just once during system boot? >> >> The hardware defaults are usually sufficient unless the system is under >> very high memory pressure. I'm only aware of one case where we actually >> need to override the hardware default on boot and it's exotic enough to >> not be upstream yet. >> >> Ultimately we want to tune these at runtime, typically together with and >> depending on the EMC frequency. Under memory pressure you can use the >> "latency allowance" registers to prioritize memory requests from >> isochronous clients (like display controllers) to ensure they don't >> underrun. > > Perhaps USB could be one example of a memory client that may need ISO > transfers for multimedia things (live audio/video), while ISO not needed > for file transfers. > >> Given that we only have to tune these under somewhat extreme conditions, >> I think these are lower priority from an implementation point of view >> than the EMC frequency scaling, but the registers are based on the >> memory client IDs, so I think it's convenient to have those be part of >> the bindings in the first place. >> >>>>>> I vaguely recall Dmitry mentioning that the EMC in early generations of >>>>>> Tegra used to have controls for individual memory clients, but I don't >>>>>> see that in more recent generations. >>>>> >>>>> EMC doesn't have direct controls over memory clients on all Tegra SoCs, >>>>> but it may have some extra knobs for the MC arbitration config. >>>>> >>>>> The MC bandwidth allocation logic and hardware programming interface >>>>> differs among SoC generations, but the basic principle is the same. >>>>> >>>>>>>> Any ideas on how to resolve this? Let me know if the DT bindings and >>>>>>>> example don't make things clear enough. >>>>> >>>>> I'm also interested in the answer to this question. >>>>> >>>>> A quick thought.. maybe it could be some new ICC DT property which tells >>>>> that all paths are the "dma-mem": >>>>> >>>>> interconnects-all-dma-mem; >>>> >>>> There could easily be cases where multiple interconnects are to system >>>> memory but there are additional ones which aren't, so the above wouldn't >>>> be able to represent such cases. >>> >>> Yes, true. > > Sure, but then that property shouldn't be used. > > Anyways, I think Thierry's suggestion about the generic memory > controller API sounds more attractive. > >>>>>>>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ >>>>>>>> 1 file changed, 59 insertions(+) >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>>> index dabf1c1aec2f..d40fcd836e90 100644 >>>>>>>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>>> @@ -43,6 +43,24 @@ properties: >>>>>>>> - enum: >>>>>>>> - nvidia,tegra186-bpmp >>>>>>>> >>>>>>>> + interconnects: >>>>>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>>>> + description: A list of phandle and specifier pairs that describe the >>>>>>>> + interconnect paths to and from the BPMP. >>>>>>>> + >>>>>>>> + interconnect-names: >>>>>>>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>>>>>>> + description: One string for each pair of phandle and specifier in the >>>>>>>> + "interconnects" property. >>>>>>>> + # XXX We need at least one of these to be named dma-mem so that the core >>>>>>>> + # will set the DMA mask based on the DMA parent, but all of these go to >>>>>>>> + # system memory eventually. >>>>>>>> + items: >>>>>>>> + - const: dma-mem >>>>>>>> + - const: dma-mem >>>>>>>> + - const: dma-mem >>>>>>>> + - const: dma-mem >>>>> >>>>> Names should be unique, otherwise it's not possible to retrieve ICC path >>>>> other than the first one. >>>> >>>> Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure >>>> what else to put there and thought this kinda made it clear that it was >>>> only half-baked. >>>> >>>>>>>> iommus: >>>>>>>> $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>>>> description: | >>>>>>>> @@ -152,8 +170,43 @@ additionalProperties: false >>>>>>>> >>>>>>>> examples: >>>>>>>> - | >>>>>>>> + #include <dt-bindings/clock/tegra186-clock.h> >>>>>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>>>>> #include <dt-bindings/mailbox/tegra186-hsp.h> >>>>>>>> + #include <dt-bindings/memory/tegra186-mc.h> >>>>>>>> + >>>>>>>> + mc: memory-controller@2c00000 { >>>>>>>> + compatible = "nvidia,tegra186-mc"; >>>>>>>> + reg = <0x02c00000 0xb0000>; >>>>>>>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; >>>>>>>> + status = "disabled"; >>>>>>>> + >>>>>>>> + #interconnect-cells = <1>; >>>>>>>> + #address-cells = <2>; >>>>>>>> + #size-cells = <2>; >>>>>>>> + >>>>>>>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * Memory clients have access to all 40 bits that the memory >>>>>>>> + * controller can address. >>>>>>>> + */ >>>>>>>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; >>>>>>>> + >>>>>>>> + #memory-controller-cells = <0>; >>>>>>>> + >>>>>>>> + emc: external-memory-controller@2c60000 { >>>>>>>> + compatible = "nvidia,tegra186-emc"; >>>>>>>> + reg = <0x0 0x02c60000 0x0 0x50000>; >>>>>>>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; >>>>>>>> + clocks = <&bpmp TEGRA186_CLK_EMC>; >>>>>>>> + clock-names = "emc"; >>>>>>>> + >>>>>>>> + #interconnect-cells = <0>; >>>>>>>> + >>>>>>>> + nvidia,bpmp = <&bpmp>; >>>>>>>> + }; >>>>>>>> + }; >>>>>>>> >>>>>>>> hsp_top0: hsp@3c00000 { >>>>>>>> compatible = "nvidia,tegra186-hsp"; >>>>>>>> @@ -187,6 +240,12 @@ examples: >>>>>>>> >>>>>>>> bpmp { >>>>>>>> compatible = "nvidia,tegra186-bpmp"; >>>>>>>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, >>>>>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, >>>>>>>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >>>>>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; >>>>> >>>>> I don't think this is a correct definition of the ICC paths because the >>>>> first node-MC_ID pair should define the source, second pair is the final >>>>> destination. Then the interconnect core builds (by itself) the path from >>>>> MC client to MC and finally to EMC based on the given source / >>>>> destination. Please see my v1 patchset for the example. >>>> >>>> Okay, sounds like "source" in this case means the initiator of the >>>> transaction and destination is the target of the transaction. I had >>>> interpreted the "source" as the "source location" of the transaction (so >>>> for reads the source would be the system memory via the EMC, and for >>>> writes the source would be the memory client interface). >>> >>> Yes, exactly. Maybe it would be more correct to call these pairs >>> initiator/target or master/slave. >> >> Do you want me to extend the bindings documentation to mention these >> alternative names? >> >>>> Yeah, I think that makes sense. It was also pointed out to me (offline) >>>> that the above doesn't work as intented for the use-case where I really >>>> need it. The primary reason why I need these "dma-mem" interconnect >>>> paths is so that the memory controller is specified as the "DMA parent" >>>> for these devices, which is important so that the DMA masks can be >>>> correctly set. Having the &emc reference in the first slot breaks that. >>>> Your suggestion makes sense when interpreting the terminology >>>> differently and it fixes the DMA parent issue (at least partially). >>>> >>>>> It should look somewhat like this: >>>>> >>>>> interconnects = >>>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, >>>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, >>>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, >>>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; >>>>> >>>>> interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; >>> >>> This looks better to me. >>> >>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>> going to be the same and it's arbitrarily defined, so it's effectively >>>> useless. But other than that it looks good. >>> >>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>> that other vendors that may have an additional internal memory, especially >>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>> the two paths (to internal memory and DDR). >> >> Most chips have a small internal memory that can be used, though it >> seldomly is. However, in that case I would expect the target to be a >> completely different device, so it'd look more like this: >> >> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >> ...; >> >> I don't think EMEM has any "downstream" other than external memory. > > The node ID should be mandatory in terms of interconnect, even if it's a > single node. EMC (provider) != EMEM (endpoint). > Thinking a bit more about how to define the ICC, I'm now leaning to a variant like this: interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; interconnect-names = "dma-mem", "read", "write", "dma-read", "dma-write"; Looks like there is a problem with having a full MC-EMEM path being defined for each memory client.. it's not very practical in terms of memory frequency scaling. Take Display Controller for example, it has a memory client for each display (overlay) plane. If planes are not overlapping on the displayed area, then the required total memory bandwidth equals to the peak bandwidth selected among of the visible planes. But if planes are overlapping, then the bandwidths of each overlapped plane are accumulated because overlapping planes will issue read request simultaneously for the overlapping areas. The Memory Controller doesn't have any knowledge about the Display Controller's specifics. Thus in the end it should be a responsibility of Display Controller's driver to calculate the required bandwidth for the hardware unit, since only the driver has all required knowledge about planes overlapping state and whatnot. The similar applies to multimedia things, like GPU or Video Decoder. They have multiple memory clients and (I'm pretty sure that) nobody is going to calculate memory bandwidth requirements for every client, it's simply impractical. So, I'm suggesting that we should have a single "dma-mem" ICC path for every hardware unit. The rest of the ICC paths could be memory_client -> memory_controller paths, providing knobs for things like MC arbitration (latency) configuration for each memory client. I think this variant of description is actually closer to the hardware, since the client's arbitration configuration ends in the Memory Controller. Please let me know what do you think about the above, thanks in advance. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-26 21:56 ` Dmitry Osipenko 0 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-26 21:56 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra, devicetree 21.01.2020 23:12, Dmitry Osipenko пишет: > 21.01.2020 18:54, Thierry Reding пишет: >> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>> On 1/21/20 16:10, Thierry Reding wrote: >>>> On Tue, Jan 21, 2020 at 09:53:48AM +0300, Dmitry Osipenko wrote: >>>>> 20.01.2020 18:06, Thierry Reding пишет: >>>>>> On Fri, Jan 17, 2020 at 05:23:43PM +0200, Georgi Djakov wrote: >>>>>>> Hi Thierry, >>>>>>> >>>>>>> Thanks for the patch! >>>>>>> >>>>>>> On 1/14/20 20:15, Thierry Reding wrote: >>>>>>>> From: Thierry Reding <treding@nvidia.com> >>>>>>>> >>>>>>>> Document the interconnects property that is used to describe the paths >>>>>>>> from and to system memory from and to the BPMP. >>>>>>>> >>>>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>>>>>>> --- >>>>>>>> Rob, Georgi, >>>>>>>> >>>>>>>> after the initial RFC that I did for adding interconnect properties on >>>>>>>> Tegra, I realized that the description wasn't complete. This is an >>>>>>>> attempt at a more accurate description, but unfortunately I'm not sure >>>>>>>> if it's even correct in terms of the interconnect bindings. >>>>>>>> >>>>>>>> The problem here is that on Tegra, each device has multiple paths to >>>>>>>> system memory, and I have no good idea on what to pick as the default. >>>>>>>> They are all basically the same path, but each provides extra controls >>>>>>>> to configure the "interconnect". >>>>>>> >>>>>>> Are these multiple paths between a device and system memory used simultaneously >>>>>>> for load-balancing, or who makes the decision about which path would be used? >>>>>> >>>>>> It varies. The vast majority of these paths are read/write pairs, which >>>>>> can be configured separately. There are also cases where multiple paths >>>>>> are used for load-balancing and I don't think there's any direct >>>>>> software control over which path will be used. >>>>>> >>>>>> A third class is where you have one device, but two read/write pairs, >>>>>> one which is tied to a microcontroller that's part of the device, and >>>>>> another read/write pair that is used for DMA to/from the device. >>>>>> >>>>>> Often in the latter case, the microcontroller memory client interfaces >>>>>> will be used by the microcontroller to read firmware and once the micro- >>>>>> controller has booted up, the DMA memory client interfaces will be used >>>>>> to read/write system memory with bulk data (like frame buffers, etc.). >>>>>> >>>>>>> Is this based on the client/stream ID that you mentioned previously? >>>>>> >>>>>> These are now all what's call memory client IDs, which identify the >>>>>> corresponding interface to the memory controller. Stream IDs are >>>>>> slightly higher-level and typically identify the "module" that uses >>>>>> the SMMU. Generally a stream ID is mapped to one or more memory client >>>>>> IDs. >>>>>> >>>>>>> Looking at the the binding below, it seems to me like there are different >>>>>>> master/slave pairs between MC and EMC and each link is used for >>>>>>> unidirectional traffic only. In terms of the interconnect API, both read >>>>>>> and write paths have the same direction. >>>>> >>>>> Yes, that definition should be incorrect. >>>>> >>>>>> I'm not sure I understand what you mean by this last sentence. Are you >>>>>> saying that each path in terms of the interconnect API is a always a >>>>>> bidirectional link? >>>>> >>>>> Please see more below. >>>>> >>>>>>> Is the EMC really an interconnect provider or is it just a slave port? Can >>>>>>> we scale both EMC and MC independently? >>>>>> >>>>>> The EMC is the only one where we can scale the frequency, but the MC has >>>>>> various knobs that can be used to fine-tune arbitration, set maximum >>>>>> latency, etc. >>>>> >>>>> Yes.. >>>>> >>>>> >>>>> EMC controls the total amount of available memory bandwidth, things like >>>>> DRAM timing and EMC-DRAM channel's performance. EMC is facing MC from >>>>> one side and DRAM (EMEM) from the other. >>>>> >>> >>> Right, so we can use the icc framework here to aggregate the requested bandwidth >>> from all clients and scale the frequency/voltage of EMC. >> >> Yeah, that was the plan. Dmitry's patches implement most of that. Note >> that we're working on this from two sides: on one hand we need to figure >> out the bindings so that we can set up the interconnect paths, then the >> memory controller and external memory controller drivers need to be made >> interconnect providers so that we can actually implement the dynamic >> scaling (and then finally all memory client drivers need to be updated >> to actually use these ICC framework to request bandwidth). >> >> I'm prioritizing the first issue right now because that's currently a >> blocker for enabling SMMU support on Tegra194 where we need to set the >> DMA mask based on the "bus" (i.e. DMA parent, i.e. the MC) because there >> are additional restrictions that don't exist on prior generations where >> we can simply set the DMA mask to the device's "native" DMA mask. >> >> EMC frequency scaling is slightly lower on my priority list because in >> most use-cases there is enough bandwidth at the default EMC frequency. >> >>>>> MC controls allocation of that total bandwidth between the memory >>>>> clients. It has knobs to prioritize clients, the knobs are per >>>>> read/write port. MC is facing memory clients from one side and EMC from >>>>> the other. >>>>> >>> >>> Thanks for clarifying! So are these QoS knobs (priority, latency etc.) tuned >>> dynamically during runtime or is it more like a static configuration that is >>> done for example just once during system boot? >> >> The hardware defaults are usually sufficient unless the system is under >> very high memory pressure. I'm only aware of one case where we actually >> need to override the hardware default on boot and it's exotic enough to >> not be upstream yet. >> >> Ultimately we want to tune these at runtime, typically together with and >> depending on the EMC frequency. Under memory pressure you can use the >> "latency allowance" registers to prioritize memory requests from >> isochronous clients (like display controllers) to ensure they don't >> underrun. > > Perhaps USB could be one example of a memory client that may need ISO > transfers for multimedia things (live audio/video), while ISO not needed > for file transfers. > >> Given that we only have to tune these under somewhat extreme conditions, >> I think these are lower priority from an implementation point of view >> than the EMC frequency scaling, but the registers are based on the >> memory client IDs, so I think it's convenient to have those be part of >> the bindings in the first place. >> >>>>>> I vaguely recall Dmitry mentioning that the EMC in early generations of >>>>>> Tegra used to have controls for individual memory clients, but I don't >>>>>> see that in more recent generations. >>>>> >>>>> EMC doesn't have direct controls over memory clients on all Tegra SoCs, >>>>> but it may have some extra knobs for the MC arbitration config. >>>>> >>>>> The MC bandwidth allocation logic and hardware programming interface >>>>> differs among SoC generations, but the basic principle is the same. >>>>> >>>>>>>> Any ideas on how to resolve this? Let me know if the DT bindings and >>>>>>>> example don't make things clear enough. >>>>> >>>>> I'm also interested in the answer to this question. >>>>> >>>>> A quick thought.. maybe it could be some new ICC DT property which tells >>>>> that all paths are the "dma-mem": >>>>> >>>>> interconnects-all-dma-mem; >>>> >>>> There could easily be cases where multiple interconnects are to system >>>> memory but there are additional ones which aren't, so the above wouldn't >>>> be able to represent such cases. >>> >>> Yes, true. > > Sure, but then that property shouldn't be used. > > Anyways, I think Thierry's suggestion about the generic memory > controller API sounds more attractive. > >>>>>>>> .../firmware/nvidia,tegra186-bpmp.yaml | 59 +++++++++++++++++++ >>>>>>>> 1 file changed, 59 insertions(+) >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>>> index dabf1c1aec2f..d40fcd836e90 100644 >>>>>>>> --- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml >>>>>>>> @@ -43,6 +43,24 @@ properties: >>>>>>>> - enum: >>>>>>>> - nvidia,tegra186-bpmp >>>>>>>> >>>>>>>> + interconnects: >>>>>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>>>> + description: A list of phandle and specifier pairs that describe the >>>>>>>> + interconnect paths to and from the BPMP. >>>>>>>> + >>>>>>>> + interconnect-names: >>>>>>>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>>>>>>> + description: One string for each pair of phandle and specifier in the >>>>>>>> + "interconnects" property. >>>>>>>> + # XXX We need at least one of these to be named dma-mem so that the core >>>>>>>> + # will set the DMA mask based on the DMA parent, but all of these go to >>>>>>>> + # system memory eventually. >>>>>>>> + items: >>>>>>>> + - const: dma-mem >>>>>>>> + - const: dma-mem >>>>>>>> + - const: dma-mem >>>>>>>> + - const: dma-mem >>>>> >>>>> Names should be unique, otherwise it's not possible to retrieve ICC path >>>>> other than the first one. >>>> >>>> Yeah, I know, that's why there's an XXX comment. =) I just wasn't sure >>>> what else to put there and thought this kinda made it clear that it was >>>> only half-baked. >>>> >>>>>>>> iommus: >>>>>>>> $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>>>> description: | >>>>>>>> @@ -152,8 +170,43 @@ additionalProperties: false >>>>>>>> >>>>>>>> examples: >>>>>>>> - | >>>>>>>> + #include <dt-bindings/clock/tegra186-clock.h> >>>>>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>>>>> #include <dt-bindings/mailbox/tegra186-hsp.h> >>>>>>>> + #include <dt-bindings/memory/tegra186-mc.h> >>>>>>>> + >>>>>>>> + mc: memory-controller@2c00000 { >>>>>>>> + compatible = "nvidia,tegra186-mc"; >>>>>>>> + reg = <0x02c00000 0xb0000>; >>>>>>>> + interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; >>>>>>>> + status = "disabled"; >>>>>>>> + >>>>>>>> + #interconnect-cells = <1>; >>>>>>>> + #address-cells = <2>; >>>>>>>> + #size-cells = <2>; >>>>>>>> + >>>>>>>> + ranges = <0x02c00000 0x0 0x02c00000 0x0 0xb0000>; >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * Memory clients have access to all 40 bits that the memory >>>>>>>> + * controller can address. >>>>>>>> + */ >>>>>>>> + dma-ranges = <0x0 0x0 0x0 0x100 0x0>; >>>>>>>> + >>>>>>>> + #memory-controller-cells = <0>; >>>>>>>> + >>>>>>>> + emc: external-memory-controller@2c60000 { >>>>>>>> + compatible = "nvidia,tegra186-emc"; >>>>>>>> + reg = <0x0 0x02c60000 0x0 0x50000>; >>>>>>>> + interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; >>>>>>>> + clocks = <&bpmp TEGRA186_CLK_EMC>; >>>>>>>> + clock-names = "emc"; >>>>>>>> + >>>>>>>> + #interconnect-cells = <0>; >>>>>>>> + >>>>>>>> + nvidia,bpmp = <&bpmp>; >>>>>>>> + }; >>>>>>>> + }; >>>>>>>> >>>>>>>> hsp_top0: hsp@3c00000 { >>>>>>>> compatible = "nvidia,tegra186-hsp"; >>>>>>>> @@ -187,6 +240,12 @@ examples: >>>>>>>> >>>>>>>> bpmp { >>>>>>>> compatible = "nvidia,tegra186-bpmp"; >>>>>>>> + interconnects = <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPR>, >>>>>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>, >>>>>>>> + <&emc &mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >>>>>>>> + <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>; >>>>> >>>>> I don't think this is a correct definition of the ICC paths because the >>>>> first node-MC_ID pair should define the source, second pair is the final >>>>> destination. Then the interconnect core builds (by itself) the path from >>>>> MC client to MC and finally to EMC based on the given source / >>>>> destination. Please see my v1 patchset for the example. >>>> >>>> Okay, sounds like "source" in this case means the initiator of the >>>> transaction and destination is the target of the transaction. I had >>>> interpreted the "source" as the "source location" of the transaction (so >>>> for reads the source would be the system memory via the EMC, and for >>>> writes the source would be the memory client interface). >>> >>> Yes, exactly. Maybe it would be more correct to call these pairs >>> initiator/target or master/slave. >> >> Do you want me to extend the bindings documentation to mention these >> alternative names? >> >>>> Yeah, I think that makes sense. It was also pointed out to me (offline) >>>> that the above doesn't work as intented for the use-case where I really >>>> need it. The primary reason why I need these "dma-mem" interconnect >>>> paths is so that the memory controller is specified as the "DMA parent" >>>> for these devices, which is important so that the DMA masks can be >>>> correctly set. Having the &emc reference in the first slot breaks that. >>>> Your suggestion makes sense when interpreting the terminology >>>> differently and it fixes the DMA parent issue (at least partially). >>>> >>>>> It should look somewhat like this: >>>>> >>>>> interconnects = >>>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc TEGRA_ICC_EMEM>, >>>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc TEGRA_ICC_EMEM>, >>>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc TEGRA_ICC_EMEM>, >>>>> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc TEGRA_ICC_EMEM>; >>>>> >>>>> interconnect-names = "bpmpr", "bpmpw", "bpmpdmar", "bpmpdmaw"; >>> >>> This looks better to me. >>> >>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>> going to be the same and it's arbitrarily defined, so it's effectively >>>> useless. But other than that it looks good. >>> >>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>> that other vendors that may have an additional internal memory, especially >>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>> the two paths (to internal memory and DDR). >> >> Most chips have a small internal memory that can be used, though it >> seldomly is. However, in that case I would expect the target to be a >> completely different device, so it'd look more like this: >> >> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >> ...; >> >> I don't think EMEM has any "downstream" other than external memory. > > The node ID should be mandatory in terms of interconnect, even if it's a > single node. EMC (provider) != EMEM (endpoint). > Thinking a bit more about how to define the ICC, I'm now leaning to a variant like this: interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; interconnect-names = "dma-mem", "read", "write", "dma-read", "dma-write"; Looks like there is a problem with having a full MC-EMEM path being defined for each memory client.. it's not very practical in terms of memory frequency scaling. Take Display Controller for example, it has a memory client for each display (overlay) plane. If planes are not overlapping on the displayed area, then the required total memory bandwidth equals to the peak bandwidth selected among of the visible planes. But if planes are overlapping, then the bandwidths of each overlapped plane are accumulated because overlapping planes will issue read request simultaneously for the overlapping areas. The Memory Controller doesn't have any knowledge about the Display Controller's specifics. Thus in the end it should be a responsibility of Display Controller's driver to calculate the required bandwidth for the hardware unit, since only the driver has all required knowledge about planes overlapping state and whatnot. The similar applies to multimedia things, like GPU or Video Decoder. They have multiple memory clients and (I'm pretty sure that) nobody is going to calculate memory bandwidth requirements for every client, it's simply impractical. So, I'm suggesting that we should have a single "dma-mem" ICC path for every hardware unit. The rest of the ICC paths could be memory_client -> memory_controller paths, providing knobs for things like MC arbitration (latency) configuration for each memory client. I think this variant of description is actually closer to the hardware, since the client's arbitration configuration ends in the Memory Controller. Please let me know what do you think about the above, thanks in advance. ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <853bb7bd-8e04-38ac-d0d6-a958135a49be-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-26 21:56 ` Dmitry Osipenko @ 2020-01-26 22:03 ` Dmitry Osipenko -1 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-26 22:03 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 27.01.2020 00:56, Dmitry Osipenko пишет: [snip] > Thinking a bit more about how to define the ICC, I'm now leaning to a > variant like this: > > interconnects = > <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; I forgot that each ICC path should have SRC and DST, but you got the idea. This should be a more correct variant: <&mc TEGRA186_MEMORY_CLIENT_BPMPR &mc TEGRA_ICC_MC>, <&mc TEGRA186_MEMORY_CLIENT_BPMPW &mc TEGRA_ICC_MC>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &mc TEGRA_ICC_MC>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &mc TEGRA_ICC_MC>; > > interconnect-names = "dma-mem", "read", "write", "dma-read", "dma-write"; ... ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-26 22:03 ` Dmitry Osipenko 0 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-26 22:03 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra, devicetree 27.01.2020 00:56, Dmitry Osipenko пишет: [snip] > Thinking a bit more about how to define the ICC, I'm now leaning to a > variant like this: > > interconnects = > <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; I forgot that each ICC path should have SRC and DST, but you got the idea. This should be a more correct variant: <&mc TEGRA186_MEMORY_CLIENT_BPMPR &mc TEGRA_ICC_MC>, <&mc TEGRA186_MEMORY_CLIENT_BPMPW &mc TEGRA_ICC_MC>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &mc TEGRA_ICC_MC>, <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &mc TEGRA_ICC_MC>; > > interconnect-names = "dma-mem", "read", "write", "dma-read", "dma-write"; ... ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <f915949a-b46e-26fe-f103-fbc8d1fa3bb1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-26 22:03 ` Dmitry Osipenko @ 2020-01-27 12:52 ` Thierry Reding -1 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-27 12:52 UTC (permalink / raw) To: Dmitry Osipenko Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 1146 bytes --] On Mon, Jan 27, 2020 at 01:03:57AM +0300, Dmitry Osipenko wrote: > 27.01.2020 00:56, Dmitry Osipenko пишет: > [snip] > > Thinking a bit more about how to define the ICC, I'm now leaning to a > > variant like this: > > > > interconnects = > > <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, > > > <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, > > <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, > > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; > > I forgot that each ICC path should have SRC and DST, but you got the idea. > > This should be a more correct variant: > > <&mc TEGRA186_MEMORY_CLIENT_BPMPR &mc TEGRA_ICC_MC>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPW &mc TEGRA_ICC_MC>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &mc TEGRA_ICC_MC>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &mc TEGRA_ICC_MC>; This seems wrong again, because now we need to make up this TEGRA_ICC_MC ID that doesn't exist anywhere in the hardware. So we're no longer providing a hardware description, but instead are building hints for a use by a Linux-specific framework into the DT. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-27 12:52 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-27 12:52 UTC (permalink / raw) To: Dmitry Osipenko Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra, devicetree [-- Attachment #1: Type: text/plain, Size: 1146 bytes --] On Mon, Jan 27, 2020 at 01:03:57AM +0300, Dmitry Osipenko wrote: > 27.01.2020 00:56, Dmitry Osipenko пишет: > [snip] > > Thinking a bit more about how to define the ICC, I'm now leaning to a > > variant like this: > > > > interconnects = > > <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, > > > <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, > > <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, > > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; > > I forgot that each ICC path should have SRC and DST, but you got the idea. > > This should be a more correct variant: > > <&mc TEGRA186_MEMORY_CLIENT_BPMPR &mc TEGRA_ICC_MC>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPW &mc TEGRA_ICC_MC>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &mc TEGRA_ICC_MC>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &mc TEGRA_ICC_MC>; This seems wrong again, because now we need to make up this TEGRA_ICC_MC ID that doesn't exist anywhere in the hardware. So we're no longer providing a hardware description, but instead are building hints for a use by a Linux-specific framework into the DT. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-26 21:56 ` Dmitry Osipenko @ 2020-01-27 12:49 ` Thierry Reding -1 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-27 12:49 UTC (permalink / raw) To: Dmitry Osipenko Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 3276 bytes --] On Mon, Jan 27, 2020 at 12:56:24AM +0300, Dmitry Osipenko wrote: [...] > Thinking a bit more about how to define the ICC, I'm now leaning to a > variant like this: > > interconnects = > <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; > > interconnect-names = "dma-mem", "read", "write", "dma-read", "dma-write"; > > Looks like there is a problem with having a full MC-EMEM path being > defined for each memory client.. it's not very practical in terms of > memory frequency scaling. > > Take Display Controller for example, it has a memory client for each > display (overlay) plane. If planes are not overlapping on the displayed > area, then the required total memory bandwidth equals to the peak > bandwidth selected among of the visible planes. But if planes are > overlapping, then the bandwidths of each overlapped plane are > accumulated because overlapping planes will issue read request > simultaneously for the overlapping areas. > > The Memory Controller doesn't have any knowledge about the Display > Controller's specifics. Thus in the end it should be a responsibility of > Display Controller's driver to calculate the required bandwidth for the > hardware unit, since only the driver has all required knowledge about > planes overlapping state and whatnot. I agree that the device-specific knowledge should live in the device- specific drivers. However, what you're doing above is basically putting the OS-specific knowledge into the device tree. The memory client interfaces are a real thing in hardware that can be described using the corresponding IDs. But there is no such thing as the "BPMP" memory client. Rather it's composed of the other four. So I think a better thing to do would be for the consumer driver to deal with all of that. If looking at only bandwidth, the consumer driver can simply pick any one of the clients/paths for any of the bandwidth requests and for per-interface settings like latency allowance the consumer would choose the appropriate path. > The similar applies to multimedia things, like GPU or Video Decoder. > They have multiple memory clients and (I'm pretty sure that) nobody is > going to calculate memory bandwidth requirements for every client, it's > simply impractical. > > So, I'm suggesting that we should have a single "dma-mem" ICC path for > every hardware unit. > > The rest of the ICC paths could be memory_client -> memory_controller > paths, providing knobs for things like MC arbitration (latency) > configuration for each memory client. I think this variant of > description is actually closer to the hardware, since the client's > arbitration configuration ends in the Memory Controller. Not necessarily. The target of the access doesn't always have to be the EMC. It could equally well be IRAM, in which case there are additional controls that need to be programmed within the MC to allow the memory client to access IRAM. If you don't have a phandle to IRAM in the interconnect properties, there's no way to make this distinction. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-27 12:49 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-27 12:49 UTC (permalink / raw) To: Dmitry Osipenko Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra, devicetree [-- Attachment #1: Type: text/plain, Size: 3276 bytes --] On Mon, Jan 27, 2020 at 12:56:24AM +0300, Dmitry Osipenko wrote: [...] > Thinking a bit more about how to define the ICC, I'm now leaning to a > variant like this: > > interconnects = > <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, > <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; > > interconnect-names = "dma-mem", "read", "write", "dma-read", "dma-write"; > > Looks like there is a problem with having a full MC-EMEM path being > defined for each memory client.. it's not very practical in terms of > memory frequency scaling. > > Take Display Controller for example, it has a memory client for each > display (overlay) plane. If planes are not overlapping on the displayed > area, then the required total memory bandwidth equals to the peak > bandwidth selected among of the visible planes. But if planes are > overlapping, then the bandwidths of each overlapped plane are > accumulated because overlapping planes will issue read request > simultaneously for the overlapping areas. > > The Memory Controller doesn't have any knowledge about the Display > Controller's specifics. Thus in the end it should be a responsibility of > Display Controller's driver to calculate the required bandwidth for the > hardware unit, since only the driver has all required knowledge about > planes overlapping state and whatnot. I agree that the device-specific knowledge should live in the device- specific drivers. However, what you're doing above is basically putting the OS-specific knowledge into the device tree. The memory client interfaces are a real thing in hardware that can be described using the corresponding IDs. But there is no such thing as the "BPMP" memory client. Rather it's composed of the other four. So I think a better thing to do would be for the consumer driver to deal with all of that. If looking at only bandwidth, the consumer driver can simply pick any one of the clients/paths for any of the bandwidth requests and for per-interface settings like latency allowance the consumer would choose the appropriate path. > The similar applies to multimedia things, like GPU or Video Decoder. > They have multiple memory clients and (I'm pretty sure that) nobody is > going to calculate memory bandwidth requirements for every client, it's > simply impractical. > > So, I'm suggesting that we should have a single "dma-mem" ICC path for > every hardware unit. > > The rest of the ICC paths could be memory_client -> memory_controller > paths, providing knobs for things like MC arbitration (latency) > configuration for each memory client. I think this variant of > description is actually closer to the hardware, since the client's > arbitration configuration ends in the Memory Controller. Not necessarily. The target of the access doesn't always have to be the EMC. It could equally well be IRAM, in which case there are additional controls that need to be programmed within the MC to allow the memory client to access IRAM. If you don't have a phandle to IRAM in the interconnect properties, there's no way to make this distinction. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-27 12:49 ` Thierry Reding @ 2020-02-05 21:34 ` Dmitry Osipenko -1 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-02-05 21:34 UTC (permalink / raw) To: Thierry Reding Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 27.01.2020 15:49, Thierry Reding пишет: > On Mon, Jan 27, 2020 at 12:56:24AM +0300, Dmitry Osipenko wrote: > [...] >> Thinking a bit more about how to define the ICC, I'm now leaning to a >> variant like this: >> >> interconnects = >> <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; >> >> interconnect-names = "dma-mem", "read", "write", "dma-read", "dma-write"; >> >> Looks like there is a problem with having a full MC-EMEM path being >> defined for each memory client.. it's not very practical in terms of >> memory frequency scaling. >> >> Take Display Controller for example, it has a memory client for each >> display (overlay) plane. If planes are not overlapping on the displayed >> area, then the required total memory bandwidth equals to the peak >> bandwidth selected among of the visible planes. But if planes are >> overlapping, then the bandwidths of each overlapped plane are >> accumulated because overlapping planes will issue read request >> simultaneously for the overlapping areas. >> >> The Memory Controller doesn't have any knowledge about the Display >> Controller's specifics. Thus in the end it should be a responsibility of >> Display Controller's driver to calculate the required bandwidth for the >> hardware unit, since only the driver has all required knowledge about >> planes overlapping state and whatnot. > > I agree that the device-specific knowledge should live in the device- > specific drivers. However, what you're doing above is basically putting > the OS-specific knowledge into the device tree. > > The memory client interfaces are a real thing in hardware that can be > described using the corresponding IDs. But there is no such thing as the > "BPMP" memory client. Rather it's composed of the other four. > > So I think a better thing to do would be for the consumer driver to deal > with all of that. If looking at only bandwidth, the consumer driver can > simply pick any one of the clients/paths for any of the bandwidth > requests and for per-interface settings like latency allowance the > consumer would choose the appropriate path. Will be good if we could avoid doing things like that because it doesn't sound very nice :) Although, it should work. On older Tegra SoCs Memory Controller has a hardware ID for each of the clients module and we're using these IDs already for the MC resets. Don't you think that we could use these IDs for ICC? Are you sure that newer SoCs do not have these IDs too or maybe they are kept private now? >> The similar applies to multimedia things, like GPU or Video Decoder. >> They have multiple memory clients and (I'm pretty sure that) nobody is >> going to calculate memory bandwidth requirements for every client, it's >> simply impractical. >> >> So, I'm suggesting that we should have a single "dma-mem" ICC path for >> every hardware unit. >> >> The rest of the ICC paths could be memory_client -> memory_controller >> paths, providing knobs for things like MC arbitration (latency) >> configuration for each memory client. I think this variant of >> description is actually closer to the hardware, since the client's >> arbitration configuration ends in the Memory Controller. > > Not necessarily. The target of the access doesn't always have to be the > EMC. It could equally well be IRAM, in which case there are additional > controls that need to be programmed within the MC to allow the memory > client to access IRAM. If you don't have a phandle to IRAM in the > interconnect properties, there's no way to make this distinction. Could you please clarify what do you mean by the "memory client" here? Do you mean the whole hardware module/unit or each memory client of the hardware module that needs to be programmed for the IRAM accessing? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-02-05 21:34 ` Dmitry Osipenko 0 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-02-05 21:34 UTC (permalink / raw) To: Thierry Reding Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra, devicetree 27.01.2020 15:49, Thierry Reding пишет: > On Mon, Jan 27, 2020 at 12:56:24AM +0300, Dmitry Osipenko wrote: > [...] >> Thinking a bit more about how to define the ICC, I'm now leaning to a >> variant like this: >> >> interconnects = >> <&mc TEGRA186_MEMORY_CLIENT_BPMP &emc TEGRA_ICC_EMEM>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPR>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPW>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR>, >> <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW>; >> >> interconnect-names = "dma-mem", "read", "write", "dma-read", "dma-write"; >> >> Looks like there is a problem with having a full MC-EMEM path being >> defined for each memory client.. it's not very practical in terms of >> memory frequency scaling. >> >> Take Display Controller for example, it has a memory client for each >> display (overlay) plane. If planes are not overlapping on the displayed >> area, then the required total memory bandwidth equals to the peak >> bandwidth selected among of the visible planes. But if planes are >> overlapping, then the bandwidths of each overlapped plane are >> accumulated because overlapping planes will issue read request >> simultaneously for the overlapping areas. >> >> The Memory Controller doesn't have any knowledge about the Display >> Controller's specifics. Thus in the end it should be a responsibility of >> Display Controller's driver to calculate the required bandwidth for the >> hardware unit, since only the driver has all required knowledge about >> planes overlapping state and whatnot. > > I agree that the device-specific knowledge should live in the device- > specific drivers. However, what you're doing above is basically putting > the OS-specific knowledge into the device tree. > > The memory client interfaces are a real thing in hardware that can be > described using the corresponding IDs. But there is no such thing as the > "BPMP" memory client. Rather it's composed of the other four. > > So I think a better thing to do would be for the consumer driver to deal > with all of that. If looking at only bandwidth, the consumer driver can > simply pick any one of the clients/paths for any of the bandwidth > requests and for per-interface settings like latency allowance the > consumer would choose the appropriate path. Will be good if we could avoid doing things like that because it doesn't sound very nice :) Although, it should work. On older Tegra SoCs Memory Controller has a hardware ID for each of the clients module and we're using these IDs already for the MC resets. Don't you think that we could use these IDs for ICC? Are you sure that newer SoCs do not have these IDs too or maybe they are kept private now? >> The similar applies to multimedia things, like GPU or Video Decoder. >> They have multiple memory clients and (I'm pretty sure that) nobody is >> going to calculate memory bandwidth requirements for every client, it's >> simply impractical. >> >> So, I'm suggesting that we should have a single "dma-mem" ICC path for >> every hardware unit. >> >> The rest of the ICC paths could be memory_client -> memory_controller >> paths, providing knobs for things like MC arbitration (latency) >> configuration for each memory client. I think this variant of >> description is actually closer to the hardware, since the client's >> arbitration configuration ends in the Memory Controller. > > Not necessarily. The target of the access doesn't always have to be the > EMC. It could equally well be IRAM, in which case there are additional > controls that need to be programmed within the MC to allow the memory > client to access IRAM. If you don't have a phandle to IRAM in the > interconnect properties, there's no way to make this distinction. Could you please clarify what do you mean by the "memory client" here? Do you mean the whole hardware module/unit or each memory client of the hardware module that needs to be programmed for the IRAM accessing? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-21 20:12 ` Dmitry Osipenko @ 2020-01-27 12:21 ` Thierry Reding -1 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-27 12:21 UTC (permalink / raw) To: Dmitry Osipenko Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 1887 bytes --] On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: > 21.01.2020 18:54, Thierry Reding пишет: > > On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: > >> On 1/21/20 16:10, Thierry Reding wrote: [...] > >>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always > >>> going to be the same and it's arbitrarily defined, so it's effectively > >>> useless. But other than that it looks good. > >> > >> Well, in most cases the target would be the EMEM, so that's fine. I have seen > >> that other vendors that may have an additional internal memory, especially > >> dedicated to some DSPs and in such cases the bandwidth needs are different for > >> the two paths (to internal memory and DDR). > > > > Most chips have a small internal memory that can be used, though it > > seldomly is. However, in that case I would expect the target to be a > > completely different device, so it'd look more like this: > > > > interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, > > ...; > > > > I don't think EMEM has any "downstream" other than external memory. > > The node ID should be mandatory in terms of interconnect, even if it's a > single node. EMC (provider) != EMEM (endpoint). I don't understand why. An ID only makes sense if you've got multiple endpoints. For example, a regulator is a provider with a single endpoint so we don't specify an ID. By its very definition an ID is used to identify something and we use it with a phandle to create a unique pair that identifies a resource within whatever the phandle represents, with the goal to differentiate it from other resources within the same provider. However, if there's only one such resource, the ID becomes redundant because the phandle without an ID is already unique and there's no need to differentiate with an extra ID. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-27 12:21 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-27 12:21 UTC (permalink / raw) To: Dmitry Osipenko Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra, devicetree [-- Attachment #1: Type: text/plain, Size: 1887 bytes --] On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: > 21.01.2020 18:54, Thierry Reding пишет: > > On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: > >> On 1/21/20 16:10, Thierry Reding wrote: [...] > >>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always > >>> going to be the same and it's arbitrarily defined, so it's effectively > >>> useless. But other than that it looks good. > >> > >> Well, in most cases the target would be the EMEM, so that's fine. I have seen > >> that other vendors that may have an additional internal memory, especially > >> dedicated to some DSPs and in such cases the bandwidth needs are different for > >> the two paths (to internal memory and DDR). > > > > Most chips have a small internal memory that can be used, though it > > seldomly is. However, in that case I would expect the target to be a > > completely different device, so it'd look more like this: > > > > interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, > > ...; > > > > I don't think EMEM has any "downstream" other than external memory. > > The node ID should be mandatory in terms of interconnect, even if it's a > single node. EMC (provider) != EMEM (endpoint). I don't understand why. An ID only makes sense if you've got multiple endpoints. For example, a regulator is a provider with a single endpoint so we don't specify an ID. By its very definition an ID is used to identify something and we use it with a phandle to create a unique pair that identifies a resource within whatever the phandle represents, with the goal to differentiate it from other resources within the same provider. However, if there's only one such resource, the ID becomes redundant because the phandle without an ID is already unique and there's no need to differentiate with an extra ID. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-27 12:21 ` Thierry Reding @ 2020-01-28 19:27 ` Dmitry Osipenko -1 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-28 19:27 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 27.01.2020 15:21, Thierry Reding пишет: > On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: >> 21.01.2020 18:54, Thierry Reding пишет: >>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>>> On 1/21/20 16:10, Thierry Reding wrote: > [...] >>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>>> going to be the same and it's arbitrarily defined, so it's effectively >>>>> useless. But other than that it looks good. >>>> >>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>>> that other vendors that may have an additional internal memory, especially >>>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>>> the two paths (to internal memory and DDR). >>> >>> Most chips have a small internal memory that can be used, though it >>> seldomly is. However, in that case I would expect the target to be a >>> completely different device, so it'd look more like this: >>> >>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >>> ...; >>> >>> I don't think EMEM has any "downstream" other than external memory. >> >> The node ID should be mandatory in terms of interconnect, even if it's a >> single node. EMC (provider) != EMEM (endpoint). > > I don't understand why. An ID only makes sense if you've got multiple > endpoints. For example, a regulator is a provider with a single endpoint > so we don't specify an ID. Because this is how ICC binding is defined, unless I'm missing something. > By its very definition an ID is used to identify something and we use it > with a phandle to create a unique pair that identifies a resource within > whatever the phandle represents, with the goal to differentiate it from > other resources within the same provider. However, if there's only one > such resource, the ID becomes redundant because the phandle without an > ID is already unique and there's no need to differentiate with an extra > ID. Georgi, do you think it is possible to support what Thierry is asking for? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-28 19:27 ` Dmitry Osipenko 0 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-28 19:27 UTC (permalink / raw) To: Thierry Reding, Georgi Djakov Cc: Rob Herring, Jon Hunter, linux-tegra, devicetree 27.01.2020 15:21, Thierry Reding пишет: > On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: >> 21.01.2020 18:54, Thierry Reding пишет: >>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>>> On 1/21/20 16:10, Thierry Reding wrote: > [...] >>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>>> going to be the same and it's arbitrarily defined, so it's effectively >>>>> useless. But other than that it looks good. >>>> >>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>>> that other vendors that may have an additional internal memory, especially >>>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>>> the two paths (to internal memory and DDR). >>> >>> Most chips have a small internal memory that can be used, though it >>> seldomly is. However, in that case I would expect the target to be a >>> completely different device, so it'd look more like this: >>> >>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >>> ...; >>> >>> I don't think EMEM has any "downstream" other than external memory. >> >> The node ID should be mandatory in terms of interconnect, even if it's a >> single node. EMC (provider) != EMEM (endpoint). > > I don't understand why. An ID only makes sense if you've got multiple > endpoints. For example, a regulator is a provider with a single endpoint > so we don't specify an ID. Because this is how ICC binding is defined, unless I'm missing something. > By its very definition an ID is used to identify something and we use it > with a phandle to create a unique pair that identifies a resource within > whatever the phandle represents, with the goal to differentiate it from > other resources within the same provider. However, if there's only one > such resource, the ID becomes redundant because the phandle without an > ID is already unique and there's no need to differentiate with an extra > ID. Georgi, do you think it is possible to support what Thierry is asking for? ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <d56618e1-8940-65ae-381e-796e44bcf537-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-28 19:27 ` Dmitry Osipenko @ 2020-01-29 9:36 ` Thierry Reding -1 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-29 9:36 UTC (permalink / raw) To: Dmitry Osipenko Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 2200 bytes --] On Tue, Jan 28, 2020 at 10:27:00PM +0300, Dmitry Osipenko wrote: > 27.01.2020 15:21, Thierry Reding пишет: > > On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: > >> 21.01.2020 18:54, Thierry Reding пишет: > >>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: > >>>> On 1/21/20 16:10, Thierry Reding wrote: > > [...] > >>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always > >>>>> going to be the same and it's arbitrarily defined, so it's effectively > >>>>> useless. But other than that it looks good. > >>>> > >>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen > >>>> that other vendors that may have an additional internal memory, especially > >>>> dedicated to some DSPs and in such cases the bandwidth needs are different for > >>>> the two paths (to internal memory and DDR). > >>> > >>> Most chips have a small internal memory that can be used, though it > >>> seldomly is. However, in that case I would expect the target to be a > >>> completely different device, so it'd look more like this: > >>> > >>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, > >>> ...; > >>> > >>> I don't think EMEM has any "downstream" other than external memory. > >> > >> The node ID should be mandatory in terms of interconnect, even if it's a > >> single node. EMC (provider) != EMEM (endpoint). > > > > I don't understand why. An ID only makes sense if you've got multiple > > endpoints. For example, a regulator is a provider with a single endpoint > > so we don't specify an ID. > > Because this is how ICC binding is defined, unless I'm missing something. I don't think so. It's defined as "pairs of phandles and interconnect provider specifiers", which is equivalent to what pretty much all of the resource bindings define. The #interconnect-cells property defines the number of cells used for the specifier. In the normal case this would be 1, and the value of the one cell would be the ID of the endpoint. But if there's only a single endpoint, it's customary to set the number of cells to 0, in which case only the phandle is required. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-29 9:36 ` Thierry Reding 0 siblings, 0 replies; 42+ messages in thread From: Thierry Reding @ 2020-01-29 9:36 UTC (permalink / raw) To: Dmitry Osipenko Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra, devicetree [-- Attachment #1: Type: text/plain, Size: 2200 bytes --] On Tue, Jan 28, 2020 at 10:27:00PM +0300, Dmitry Osipenko wrote: > 27.01.2020 15:21, Thierry Reding пишет: > > On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: > >> 21.01.2020 18:54, Thierry Reding пишет: > >>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: > >>>> On 1/21/20 16:10, Thierry Reding wrote: > > [...] > >>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always > >>>>> going to be the same and it's arbitrarily defined, so it's effectively > >>>>> useless. But other than that it looks good. > >>>> > >>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen > >>>> that other vendors that may have an additional internal memory, especially > >>>> dedicated to some DSPs and in such cases the bandwidth needs are different for > >>>> the two paths (to internal memory and DDR). > >>> > >>> Most chips have a small internal memory that can be used, though it > >>> seldomly is. However, in that case I would expect the target to be a > >>> completely different device, so it'd look more like this: > >>> > >>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, > >>> ...; > >>> > >>> I don't think EMEM has any "downstream" other than external memory. > >> > >> The node ID should be mandatory in terms of interconnect, even if it's a > >> single node. EMC (provider) != EMEM (endpoint). > > > > I don't understand why. An ID only makes sense if you've got multiple > > endpoints. For example, a regulator is a provider with a single endpoint > > so we don't specify an ID. > > Because this is how ICC binding is defined, unless I'm missing something. I don't think so. It's defined as "pairs of phandles and interconnect provider specifiers", which is equivalent to what pretty much all of the resource bindings define. The #interconnect-cells property defines the number of cells used for the specifier. In the normal case this would be 1, and the value of the one cell would be the ID of the endpoint. But if there's only a single endpoint, it's customary to set the number of cells to 0, in which case only the phandle is required. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-29 9:36 ` Thierry Reding @ 2020-01-29 16:02 ` Dmitry Osipenko -1 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-29 16:02 UTC (permalink / raw) To: Thierry Reding Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 29.01.2020 12:36, Thierry Reding пишет: > On Tue, Jan 28, 2020 at 10:27:00PM +0300, Dmitry Osipenko wrote: >> 27.01.2020 15:21, Thierry Reding пишет: >>> On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: >>>> 21.01.2020 18:54, Thierry Reding пишет: >>>>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>>>>> On 1/21/20 16:10, Thierry Reding wrote: >>> [...] >>>>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>>>>> going to be the same and it's arbitrarily defined, so it's effectively >>>>>>> useless. But other than that it looks good. >>>>>> >>>>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>>>>> that other vendors that may have an additional internal memory, especially >>>>>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>>>>> the two paths (to internal memory and DDR). >>>>> >>>>> Most chips have a small internal memory that can be used, though it >>>>> seldomly is. However, in that case I would expect the target to be a >>>>> completely different device, so it'd look more like this: >>>>> >>>>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >>>>> ...; >>>>> >>>>> I don't think EMEM has any "downstream" other than external memory. >>>> >>>> The node ID should be mandatory in terms of interconnect, even if it's a >>>> single node. EMC (provider) != EMEM (endpoint). >>> >>> I don't understand why. An ID only makes sense if you've got multiple >>> endpoints. For example, a regulator is a provider with a single endpoint >>> so we don't specify an ID. >> >> Because this is how ICC binding is defined, unless I'm missing something. > > I don't think so. It's defined as "pairs of phandles and interconnect > provider specifiers", which is equivalent to what pretty much all of the > resource bindings define. The #interconnect-cells property defines the > number of cells used for the specifier. In the normal case this would be > 1, and the value of the one cell would be the ID of the endpoint. But if > there's only a single endpoint, it's customary to set the number of > cells to 0, in which case only the phandle is required. Right, setting interconnect-cells=0 should work. I'll give it a try, thank you! ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-29 16:02 ` Dmitry Osipenko 0 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-29 16:02 UTC (permalink / raw) To: Thierry Reding Cc: Georgi Djakov, Rob Herring, Jon Hunter, linux-tegra, devicetree 29.01.2020 12:36, Thierry Reding пишет: > On Tue, Jan 28, 2020 at 10:27:00PM +0300, Dmitry Osipenko wrote: >> 27.01.2020 15:21, Thierry Reding пишет: >>> On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: >>>> 21.01.2020 18:54, Thierry Reding пишет: >>>>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>>>>> On 1/21/20 16:10, Thierry Reding wrote: >>> [...] >>>>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>>>>> going to be the same and it's arbitrarily defined, so it's effectively >>>>>>> useless. But other than that it looks good. >>>>>> >>>>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>>>>> that other vendors that may have an additional internal memory, especially >>>>>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>>>>> the two paths (to internal memory and DDR). >>>>> >>>>> Most chips have a small internal memory that can be used, though it >>>>> seldomly is. However, in that case I would expect the target to be a >>>>> completely different device, so it'd look more like this: >>>>> >>>>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >>>>> ...; >>>>> >>>>> I don't think EMEM has any "downstream" other than external memory. >>>> >>>> The node ID should be mandatory in terms of interconnect, even if it's a >>>> single node. EMC (provider) != EMEM (endpoint). >>> >>> I don't understand why. An ID only makes sense if you've got multiple >>> endpoints. For example, a regulator is a provider with a single endpoint >>> so we don't specify an ID. >> >> Because this is how ICC binding is defined, unless I'm missing something. > > I don't think so. It's defined as "pairs of phandles and interconnect > provider specifiers", which is equivalent to what pretty much all of the > resource bindings define. The #interconnect-cells property defines the > number of cells used for the specifier. In the normal case this would be > 1, and the value of the one cell would be the ID of the endpoint. But if > there's only a single endpoint, it's customary to set the number of > cells to 0, in which case only the phandle is required. Right, setting interconnect-cells=0 should work. I'll give it a try, thank you! ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <0b8692ab-4e06-b277-bbe2-93922e47c2f6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-29 16:02 ` Dmitry Osipenko @ 2020-01-29 16:13 ` Georgi Djakov -1 siblings, 0 replies; 42+ messages in thread From: Georgi Djakov @ 2020-01-29 16:13 UTC (permalink / raw) To: Dmitry Osipenko, Thierry Reding Cc: Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA On 1/29/20 18:02, Dmitry Osipenko wrote: > 29.01.2020 12:36, Thierry Reding пишет: >> On Tue, Jan 28, 2020 at 10:27:00PM +0300, Dmitry Osipenko wrote: >>> 27.01.2020 15:21, Thierry Reding пишет: >>>> On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: >>>>> 21.01.2020 18:54, Thierry Reding пишет: >>>>>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>>>>>> On 1/21/20 16:10, Thierry Reding wrote: >>>> [...] >>>>>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>>>>>> going to be the same and it's arbitrarily defined, so it's effectively >>>>>>>> useless. But other than that it looks good. >>>>>>> >>>>>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>>>>>> that other vendors that may have an additional internal memory, especially >>>>>>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>>>>>> the two paths (to internal memory and DDR). >>>>>> >>>>>> Most chips have a small internal memory that can be used, though it >>>>>> seldomly is. However, in that case I would expect the target to be a >>>>>> completely different device, so it'd look more like this: >>>>>> >>>>>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >>>>>> ...; >>>>>> >>>>>> I don't think EMEM has any "downstream" other than external memory. >>>>> >>>>> The node ID should be mandatory in terms of interconnect, even if it's a >>>>> single node. EMC (provider) != EMEM (endpoint). >>>> >>>> I don't understand why. An ID only makes sense if you've got multiple >>>> endpoints. For example, a regulator is a provider with a single endpoint >>>> so we don't specify an ID. >>> >>> Because this is how ICC binding is defined, unless I'm missing something. >> >> I don't think so. It's defined as "pairs of phandles and interconnect >> provider specifiers", which is equivalent to what pretty much all of the >> resource bindings define. The #interconnect-cells property defines the >> number of cells used for the specifier. In the normal case this would be >> 1, and the value of the one cell would be the ID of the endpoint. But if >> there's only a single endpoint, it's customary to set the number of >> cells to 0, in which case only the phandle is required. > > Right, setting interconnect-cells=0 should work. I'll give it a try, > thank you! Yes, it's fine to have #interconnect-cells = <0>. Here is a patch [1] which is a bit related to this. Thanks, Georgi [1] https://patchwork.kernel.org/patch/11305295/ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-29 16:13 ` Georgi Djakov 0 siblings, 0 replies; 42+ messages in thread From: Georgi Djakov @ 2020-01-29 16:13 UTC (permalink / raw) To: Dmitry Osipenko, Thierry Reding Cc: Rob Herring, Jon Hunter, linux-tegra, devicetree On 1/29/20 18:02, Dmitry Osipenko wrote: > 29.01.2020 12:36, Thierry Reding пишет: >> On Tue, Jan 28, 2020 at 10:27:00PM +0300, Dmitry Osipenko wrote: >>> 27.01.2020 15:21, Thierry Reding пишет: >>>> On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: >>>>> 21.01.2020 18:54, Thierry Reding пишет: >>>>>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>>>>>> On 1/21/20 16:10, Thierry Reding wrote: >>>> [...] >>>>>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>>>>>> going to be the same and it's arbitrarily defined, so it's effectively >>>>>>>> useless. But other than that it looks good. >>>>>>> >>>>>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>>>>>> that other vendors that may have an additional internal memory, especially >>>>>>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>>>>>> the two paths (to internal memory and DDR). >>>>>> >>>>>> Most chips have a small internal memory that can be used, though it >>>>>> seldomly is. However, in that case I would expect the target to be a >>>>>> completely different device, so it'd look more like this: >>>>>> >>>>>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >>>>>> ...; >>>>>> >>>>>> I don't think EMEM has any "downstream" other than external memory. >>>>> >>>>> The node ID should be mandatory in terms of interconnect, even if it's a >>>>> single node. EMC (provider) != EMEM (endpoint). >>>> >>>> I don't understand why. An ID only makes sense if you've got multiple >>>> endpoints. For example, a regulator is a provider with a single endpoint >>>> so we don't specify an ID. >>> >>> Because this is how ICC binding is defined, unless I'm missing something. >> >> I don't think so. It's defined as "pairs of phandles and interconnect >> provider specifiers", which is equivalent to what pretty much all of the >> resource bindings define. The #interconnect-cells property defines the >> number of cells used for the specifier. In the normal case this would be >> 1, and the value of the one cell would be the ID of the endpoint. But if >> there's only a single endpoint, it's customary to set the number of >> cells to 0, in which case only the phandle is required. > > Right, setting interconnect-cells=0 should work. I'll give it a try, > thank you! Yes, it's fine to have #interconnect-cells = <0>. Here is a patch [1] which is a bit related to this. Thanks, Georgi [1] https://patchwork.kernel.org/patch/11305295/ ^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <7db91ca2-6ef7-7161-6ec9-f69a8d8d8a34-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property 2020-01-29 16:13 ` Georgi Djakov @ 2020-01-29 16:16 ` Dmitry Osipenko -1 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-29 16:16 UTC (permalink / raw) To: Georgi Djakov, Thierry Reding Cc: Rob Herring, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 29.01.2020 19:13, Georgi Djakov пишет: > On 1/29/20 18:02, Dmitry Osipenko wrote: >> 29.01.2020 12:36, Thierry Reding пишет: >>> On Tue, Jan 28, 2020 at 10:27:00PM +0300, Dmitry Osipenko wrote: >>>> 27.01.2020 15:21, Thierry Reding пишет: >>>>> On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: >>>>>> 21.01.2020 18:54, Thierry Reding пишет: >>>>>>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>>>>>>> On 1/21/20 16:10, Thierry Reding wrote: >>>>> [...] >>>>>>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>>>>>>> going to be the same and it's arbitrarily defined, so it's effectively >>>>>>>>> useless. But other than that it looks good. >>>>>>>> >>>>>>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>>>>>>> that other vendors that may have an additional internal memory, especially >>>>>>>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>>>>>>> the two paths (to internal memory and DDR). >>>>>>> >>>>>>> Most chips have a small internal memory that can be used, though it >>>>>>> seldomly is. However, in that case I would expect the target to be a >>>>>>> completely different device, so it'd look more like this: >>>>>>> >>>>>>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >>>>>>> ...; >>>>>>> >>>>>>> I don't think EMEM has any "downstream" other than external memory. >>>>>> >>>>>> The node ID should be mandatory in terms of interconnect, even if it's a >>>>>> single node. EMC (provider) != EMEM (endpoint). >>>>> >>>>> I don't understand why. An ID only makes sense if you've got multiple >>>>> endpoints. For example, a regulator is a provider with a single endpoint >>>>> so we don't specify an ID. >>>> >>>> Because this is how ICC binding is defined, unless I'm missing something. >>> >>> I don't think so. It's defined as "pairs of phandles and interconnect >>> provider specifiers", which is equivalent to what pretty much all of the >>> resource bindings define. The #interconnect-cells property defines the >>> number of cells used for the specifier. In the normal case this would be >>> 1, and the value of the one cell would be the ID of the endpoint. But if >>> there's only a single endpoint, it's customary to set the number of >>> cells to 0, in which case only the phandle is required. >> >> Right, setting interconnect-cells=0 should work. I'll give it a try, >> thank you! > > Yes, it's fine to have #interconnect-cells = <0>. Here is a patch [1] which is a > bit related to this. > > Thanks, > Georgi > > [1] https://patchwork.kernel.org/patch/11305295/ Georgi, thank you very much! This patch will be handy! ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property @ 2020-01-29 16:16 ` Dmitry Osipenko 0 siblings, 0 replies; 42+ messages in thread From: Dmitry Osipenko @ 2020-01-29 16:16 UTC (permalink / raw) To: Georgi Djakov, Thierry Reding Cc: Rob Herring, Jon Hunter, linux-tegra, devicetree 29.01.2020 19:13, Georgi Djakov пишет: > On 1/29/20 18:02, Dmitry Osipenko wrote: >> 29.01.2020 12:36, Thierry Reding пишет: >>> On Tue, Jan 28, 2020 at 10:27:00PM +0300, Dmitry Osipenko wrote: >>>> 27.01.2020 15:21, Thierry Reding пишет: >>>>> On Tue, Jan 21, 2020 at 11:12:11PM +0300, Dmitry Osipenko wrote: >>>>>> 21.01.2020 18:54, Thierry Reding пишет: >>>>>>> On Tue, Jan 21, 2020 at 05:18:43PM +0200, Georgi Djakov wrote: >>>>>>>> On 1/21/20 16:10, Thierry Reding wrote: >>>>> [...] >>>>>>>>> I'm not sure if that TEGRA_ICC_EMEM makes a lot of sense. It's always >>>>>>>>> going to be the same and it's arbitrarily defined, so it's effectively >>>>>>>>> useless. But other than that it looks good. >>>>>>>> >>>>>>>> Well, in most cases the target would be the EMEM, so that's fine. I have seen >>>>>>>> that other vendors that may have an additional internal memory, especially >>>>>>>> dedicated to some DSPs and in such cases the bandwidth needs are different for >>>>>>>> the two paths (to internal memory and DDR). >>>>>>> >>>>>>> Most chips have a small internal memory that can be used, though it >>>>>>> seldomly is. However, in that case I would expect the target to be a >>>>>>> completely different device, so it'd look more like this: >>>>>>> >>>>>>> interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &iram>, >>>>>>> ...; >>>>>>> >>>>>>> I don't think EMEM has any "downstream" other than external memory. >>>>>> >>>>>> The node ID should be mandatory in terms of interconnect, even if it's a >>>>>> single node. EMC (provider) != EMEM (endpoint). >>>>> >>>>> I don't understand why. An ID only makes sense if you've got multiple >>>>> endpoints. For example, a regulator is a provider with a single endpoint >>>>> so we don't specify an ID. >>>> >>>> Because this is how ICC binding is defined, unless I'm missing something. >>> >>> I don't think so. It's defined as "pairs of phandles and interconnect >>> provider specifiers", which is equivalent to what pretty much all of the >>> resource bindings define. The #interconnect-cells property defines the >>> number of cells used for the specifier. In the normal case this would be >>> 1, and the value of the one cell would be the ID of the endpoint. But if >>> there's only a single endpoint, it's customary to set the number of >>> cells to 0, in which case only the phandle is required. >> >> Right, setting interconnect-cells=0 should work. I'll give it a try, >> thank you! > > Yes, it's fine to have #interconnect-cells = <0>. Here is a patch [1] which is a > bit related to this. > > Thanks, > Georgi > > [1] https://patchwork.kernel.org/patch/11305295/ Georgi, thank you very much! This patch will be handy! ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] dt-bindings: firmware: Convert Tegra186 BPMP bindings to json-schema 2020-01-14 18:15 ` Thierry Reding @ 2020-01-16 19:28 ` Rob Herring -1 siblings, 0 replies; 42+ messages in thread From: Rob Herring @ 2020-01-16 19:28 UTC (permalink / raw) Cc: Georgi Djakov, Thierry Reding, Jon Hunter, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA On Tue, 14 Jan 2020 19:15:18 +0100, Thierry Reding wrote: > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > > Convert the Tegra186 BPMP bindings from the old free-form text format to > a json-schema and fix things up so that existing device trees properly > validate. > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > --- > .../firmware/nvidia,tegra186-bpmp.txt | 107 --------- > .../firmware/nvidia,tegra186-bpmp.yaml | 209 ++++++++++++++++++ > 2 files changed, 209 insertions(+), 107 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt > create mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/display/simple-framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node Error: Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.example.dts:52.29-30 syntax error FATAL ERROR: Unable to parse input tree scripts/Makefile.lib:300: recipe for target 'Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.example.dt.yaml' failed make[1]: *** [Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.example.dt.yaml] Error 1 Makefile:1263: recipe for target 'dt_binding_check' failed make: *** [dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1222983 Please check and re-submit. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] dt-bindings: firmware: Convert Tegra186 BPMP bindings to json-schema @ 2020-01-16 19:28 ` Rob Herring 0 siblings, 0 replies; 42+ messages in thread From: Rob Herring @ 2020-01-16 19:28 UTC (permalink / raw) To: Thierry Reding Cc: Georgi Djakov, Thierry Reding, Jon Hunter, linux-tegra, devicetree On Tue, 14 Jan 2020 19:15:18 +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Convert the Tegra186 BPMP bindings from the old free-form text format to > a json-schema and fix things up so that existing device trees properly > validate. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > .../firmware/nvidia,tegra186-bpmp.txt | 107 --------- > .../firmware/nvidia,tegra186-bpmp.yaml | 209 ++++++++++++++++++ > 2 files changed, 209 insertions(+), 107 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt > create mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/display/simple-framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node Error: Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.example.dts:52.29-30 syntax error FATAL ERROR: Unable to parse input tree scripts/Makefile.lib:300: recipe for target 'Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.example.dt.yaml' failed make[1]: *** [Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.example.dt.yaml] Error 1 Makefile:1263: recipe for target 'dt_binding_check' failed make: *** [dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1222983 Please check and re-submit. ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2020-02-05 21:34 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 18:15 [PATCH 1/2] dt-bindings: firmware: Convert Tegra186 BPMP bindings to json-schema Thierry Reding
2020-01-14 18:15 ` Thierry Reding
[not found] ` <20200114181519.3402385-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2020-01-14 18:15 ` [RFC 2/2] dt-bindings: firmware: tegra186-bpmp: Document interconnects property Thierry Reding
2020-01-14 18:15 ` Thierry Reding
[not found] ` <20200114181519.3402385-2-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2020-01-17 15:23 ` Georgi Djakov
2020-01-17 15:23 ` Georgi Djakov
[not found] ` <7aefac6c-092c-b5a6-2fa6-e283d2147fc3-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2020-01-20 15:06 ` Thierry Reding
2020-01-20 15:06 ` Thierry Reding
2020-01-21 6:53 ` Dmitry Osipenko
2020-01-21 6:53 ` Dmitry Osipenko
[not found] ` <57c37b3c-1473-d444-db59-8c6650241188-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2020-01-21 14:10 ` Thierry Reding
2020-01-21 14:10 ` Thierry Reding
2020-01-21 15:18 ` Georgi Djakov
2020-01-21 15:18 ` Georgi Djakov
[not found] ` <83d94918-bc01-131b-924c-9750767d3b29-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2020-01-21 15:54 ` Thierry Reding
2020-01-21 15:54 ` Thierry Reding
2020-01-21 20:12 ` Dmitry Osipenko
2020-01-21 20:12 ` Dmitry Osipenko
[not found] ` <ffc22502-0e7e-522c-543d-0e74cc25f4b1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2020-01-26 21:56 ` Dmitry Osipenko
2020-01-26 21:56 ` Dmitry Osipenko
[not found] ` <853bb7bd-8e04-38ac-d0d6-a958135a49be-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2020-01-26 22:03 ` Dmitry Osipenko
2020-01-26 22:03 ` Dmitry Osipenko
[not found] ` <f915949a-b46e-26fe-f103-fbc8d1fa3bb1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2020-01-27 12:52 ` Thierry Reding
2020-01-27 12:52 ` Thierry Reding
2020-01-27 12:49 ` Thierry Reding
2020-01-27 12:49 ` Thierry Reding
2020-02-05 21:34 ` Dmitry Osipenko
2020-02-05 21:34 ` Dmitry Osipenko
2020-01-27 12:21 ` Thierry Reding
2020-01-27 12:21 ` Thierry Reding
2020-01-28 19:27 ` Dmitry Osipenko
2020-01-28 19:27 ` Dmitry Osipenko
[not found] ` <d56618e1-8940-65ae-381e-796e44bcf537-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2020-01-29 9:36 ` Thierry Reding
2020-01-29 9:36 ` Thierry Reding
2020-01-29 16:02 ` Dmitry Osipenko
2020-01-29 16:02 ` Dmitry Osipenko
[not found] ` <0b8692ab-4e06-b277-bbe2-93922e47c2f6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2020-01-29 16:13 ` Georgi Djakov
2020-01-29 16:13 ` Georgi Djakov
[not found] ` <7db91ca2-6ef7-7161-6ec9-f69a8d8d8a34-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2020-01-29 16:16 ` Dmitry Osipenko
2020-01-29 16:16 ` Dmitry Osipenko
2020-01-16 19:28 ` [PATCH 1/2] dt-bindings: firmware: Convert Tegra186 BPMP bindings to json-schema Rob Herring
2020-01-16 19:28 ` Rob Herring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.