On 4/8/20 7:02 PM, Suman Anna wrote: > Hi Rob, > > On 3/26/20 11:53 AM, Rob Herring wrote: >> On Tue, Mar 24, 2020 at 2:18 PM Suman Anna <s-anna@xxxxxx> wrote: >>> >>> The Texas Instruments K3 family of SoCs have one or more dual-core >>> Arm Cortex R5F processor subsystems/clusters (R5FSS). The clusters >>> can be split between multiple voltage domains as well. Add the device >>> tree bindings document for these R5F subsystem devices. These R5F >>> processors do not have an MMU, and so require fixed memory carveout >>> regions matching the firmware image addresses. The nodes require more >>> than one memory region, with the first memory region used for DMA >>> allocations at runtime. The remaining memory regions are reserved >>> and are used for the loading and running of the R5F remote processors. >>> The R5F processors can also optionally use any internal on-chip SRAM >>> memories either for executing code or using it as fast-access data. >> >> I'm inclined to say the system DT stuff should be sorted out before >> accepting this. Is the system DT stuff going to be useful for your R5 >> cores? Do you really want to be stuck with this binding? > > Hmm, I am not dependent on System DT and prefer to be not gated by that. > This is still all from the Linux host perspective, and we don't have any > plans to use DT on the firmware-side. > >> >>> The added example illustrates the DT nodes for the single R5FSS device >>> present on K3 AM65x family of SoCs. >>> >>> Signed-off-by: Suman Anna <s-anna@xxxxxx> >>> --- >>> Hi Rob, >>> >>> The dt_bindings_check seems to throw couple of warnings around the >>> usage of ranges because the tooling is adding the #address-cells >>> and #size-cells of 1 by default, whereas our actual code uses 2. >> >> Then change the default by specifying what you want. Or change the >> example to be 1 cell. It is *just* an example. > > OK, was using the actual dts nodes as how they would be added in our dts > files. The only way to get rid of the warnings is to use 1 cell. I can > do that for the R5F bindings, but cannot really do that for the DSPs > since the addresses need 2 cells. > >> >>> No issues are found with dtbs_check. >> >> I doubt that if your dts matches the example. > > The top-level cells value is 2 in our dts files (See either of > arm64/dts/ti/k3-am65.dtsi or k3-j721e.dtsi). > >> >>> >>> regards >>> Suman >>> >>> .../bindings/remoteproc/ti,k3-r5f-rproc.yaml | 338 ++++++++++++++++++ >>> 1 file changed, 338 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml >>> new file mode 100644 >>> index 000000000000..bbfc1e6ae884 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml >>> @@ -0,0 +1,338 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/remoteproc/ti,k3-r5f-rproc.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: TI K3 R5F processor subsystems >>> + >>> +maintainers: >>> + - Suman Anna <s-anna@xxxxxx> >>> + >>> +description: | >>> + The TI K3 family of SoCs usually have one or more dual-core Arm Cortex R5F >>> + processor subsystems/clusters (R5FSS). The dual core cluster can be used >>> + either in a LockStep mode providing safety/fault tolerance features or in a >>> + Split mode providing two individual compute cores for doubling the compute >>> + capacity. These are used together with other processors present on the SoC >>> + to achieve various system level goals. >>> + >>> + Each Dual-Core R5F sub-system is represented as a single DTS node >>> + representing the cluster, with a pair of child DT nodes representing >>> + the individual R5F cores. Each node has a number of required or optional >>> + properties that enable the OS running on the host processor to perform >>> + the device management of the remote processor and to communicate with the >>> + remote processor. >>> + >>> +# Required properties: >>> +# -------------------- >>> +# The following are the mandatory properties: >>> + >>> +properties: >>> + $nodename: >>> + pattern: "^r5fss(@.*)?" >>> + >>> + compatible: >>> + enum: >>> + - ti,am654-r5fss >>> + - ti,j721e-r5fss >>> + >>> + power-domains: >>> + description: | >>> + Should contain a phandle to a PM domain provider node and an args >>> + specifier containing the R5FSS device id value. This property is >>> + as per the binding, >>> + Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt >> >> What implementation of power domains is used is outside the scope of >> this binding. I'd just drop the whole description as it is pretty >> generic. > > OK. > >> >>> + maxItems: 1 >>> + >>> + "#address-cells": >>> + const: 1 >>> + >>> + "#size-cells": >>> + const: 1 >>> + >>> + ranges: >>> + description: | >>> + Standard ranges definition providing address translations for >>> + local R5F TCM address spaces to bus addresses. >>> + >>> +# Optional properties: >>> +# -------------------- >>> + >>> + lockstep-mode: >> >> Needs a vendor prefix. > > Yep, will fix this one and all the others below. > >> >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1] >>> + description: | >>> + Configuration Mode for the Dual R5F cores within the R5F >>> + cluster. Should be either a value of 1 (LockStep mode) or >>> + 0 (Split mode), default is LockStep mode if omitted. >>> + >>> +# R5F Processor Child Nodes: >>> +# ========================== >>> + >>> +patternProperties: >>> + "^r5f@[a-f0-9]+$": >>> + type: object >>> + description: | >>> + The R5F Sub-System device node should define two R5F child nodes, each >>> + node representing a TI instantiation of the Arm Cortex R5F core. There >>> + are some specific integration differences for the IP like the usage of >>> + a Region Address Translator (RAT) for translating the larger SoC bus >>> + addresses into a 32-bit address space for the processor. >>> + >>> +# Required properties: >>> +# -------------------- >>> +# The following are the mandatory properties: >>> + >>> + properties: >>> + compatible: >>> + enum: >>> + - ti,am654-r5f >>> + - ti,j721e-r5f >>> + >>> + reg: >>> + description: | >>> + Should contain an entry for each value in 'reg-names'. >>> + Each entry should have the memory region's start address >>> + and the size of the region, the representation matching >>> + the parent node's '#address-cells' and '#size-cells' values. >> >> That's every 'reg' property. >> >>> + maxItems: 2 >> >> You need to define what each one is: >> >> items: >> - description: ... >> - description: ... > > OK, will fix. > >> >>> + >>> + reg-names: >>> + description: | >>> + Should contain strings with the names of the specific internal >>> + internal memory regions, and should be defined in this order >>> + maxItems: 2 >>> + items: >>> + - const: atcm >>> + - const: btcm >>> + >>> + ti,sci: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: >>> + Should be a phandle to the TI-SCI System Controller node >>> + >>> + ti,sci-dev-id: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: | >>> + Should contain the TI-SCI device id corresponding to the R5F core. >>> + Please refer to the corresponding System Controller documentation >>> + for valid values for the R5F cores. >>> + >>> + ti,sci-proc-ids: >>> + description: Should contain a single tuple of <proc_id host_id>. >>> + allOf: >>> + - $ref: /schemas/types.yaml#/definitions/uint32-matrix >> >> Sounds more like an array. > > OK, I can modify this. Went with this originally to reflect the tuple, > but guess both translate similarly for my usage. > >> >>> + - maxItems: 1 >>> + items: >>> + items: >>> + - description: TI-SCI processor id for the R5F core device >>> + - description: TI-SCI host id to which processor control >>> + ownership should be transferred to >>> + >>> + resets: >>> + description: | >>> + Should contain the phandle to the reset controller node >>> + managing the resets for this device, and a reset >>> + specifier. Please refer to the following reset bindings >>> + for the reset argument specifier, >>> + Documentation/devicetree/bindings/reset/ti,sci-reset.txt >>> + for AM65x and J721E SoCs >> >> Drop. How many resets (maxItems or items list)? > > Yeah, this is 1, will update. Do you want me to drop just the specifier > link or the entire description? > >> >>> + >>> + firmware-name: >>> + description: | >>> + Should contain the name of the default firmware image >>> + file located on the firmware search path >>> + >>> +# The following properties are mandatory for R5F Core0 in both LockStep and Split >>> +# modes, and are mandatory for R5F Core1 _only_ in Split mode. They are unused for >>> +# R5F Core1 in LockStep mode: Rob, If I were to actually encode this in YAML, I need to distinguish Core0 from Core1. I have currently followed the DT generic node name convention when defining the two nodes and interpret in the driver based on addresses. What is the preferred mechanism for this - define node names as r5f0 and r5f1 or introduce a ti,core-id or ti,cpu-id property? regards Suman >>> + >>> + mboxes: >>> + description: | >>> + OMAP Mailbox specifier denoting the sub-mailbox, to be used for >>> + communication with the remote processor. This property should match >>> + with the sub-mailbox node used in the firmware image. The specifier >>> + format is as per the bindings, >>> + Documentation/devicetree/bindings/mailbox/omap-mailbox.txt >> >> How many? > > OK, will fix. > >> >>> + >>> + memory-region: >>> + minItems: 2 >>> + description: | >>> + phandle to the reserved memory nodes to be associated with the remoteproc >>> + device. There should be atleast two reserved memory nodes defined - the >> >> What's the max number? As is, it will be 2. > > Aah, I misinterpreted that not having would be open-ended. OK, I will > have to give an arbitrary number here (maybe 4 or 8). Can we update this > later on if a usecase really needs more? > >> >>> + first one would be used for dynamic DMA allocations like vrings and vring >>> + buffers, and the remaining ones used for the firmware image sections. The >>> + reserved memory nodes should be carveout nodes, and should be defined as >>> + per the bindings in >>> + Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt >>> + >>> +# Optional properties: >>> +# -------------------- >>> +# The following properties are optional properties for each of the R5F cores: >>> + >>> + atcm-enable: >> >> Vendor prefix needed. >> >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1] >>> + description: | >>> + R5F core configuration mode dictating if ATCM should be enabled. R5F >>> + view of ATCM dictated by loczrama property. Should be either a value >>> + of 1 (enabled) or 0 (disabled), default is disabled if omitted. >>> + >>> + btcm-enable: >> >> ditto >> >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1] >>> + description: | >>> + R5F core configuration mode dictating if BTCM should be enabled. R5F >>> + view of BTCM dictated by loczrama property. Should be either a value >>> + of 1 (enabled) or 0 (disabled), default is enabled if omitted. >>> + >>> + loczrama: >> >> ditto >> >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1] >>> + description: | >>> + R5F core configuration mode dictating which TCM should appear at >>> + address 0 (from core's view). Should be either a value of 1 (ATCM >>> + at 0x0) or 0 (BTCM at 0x0), default value is 1 if omitted. >> >> I can't decipher how you came up with 'loczrama' based on the description. > > That's actually the signal name from the Arm R5 specs. > >> >>> + >>> + sram: >>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>> + minItems: 1 >>> + description: | >>> + pHandles to one or more reserved on-chip SRAM region. The regions >>> + should be defined as child nodes of the respective SRAM node, and >>> + should be defined as per the generic bindings in, >>> + Documentation/devicetree/bindings/sram/sram.yaml >>> + >>> + required: >>> + - compatible >>> + - reg >>> + - reg-names >>> + - ti,sci >>> + - ti,sci-dev-id >>> + - ti,sci-proc-ids >>> + - resets >>> + - firmware-name >>> + >>> + additionalProperties: false >>> + >>> +required: >>> + - compatible >>> + - power-domains >>> + - "#address-cells" >>> + - "#size-cells" >>> + - ranges >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + >>> + //Example: AM654 SoC >>> + /* R5F DDR Carveout reserved memory nodes */ >>> + reserved-memory { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + ranges; >>> + >>> + mcu_r5fss0_core1_dma_memory_region: r5f-dma-memory@9b000000 { >>> + compatible = "shared-dma-pool"; >>> + reg = <0x00 0x9b000000 0x00 0x100000>; >>> + no-map; >>> + }; >>> + >>> + mcu_r5fss0_core1_memory_region: r5f-memory@9b100000 { >>> + compatible = "shared-dma-pool"; >>> + reg = <0x00 0x9b100000 0x00 0xf00000>; >>> + no-map; >>> + }; >>> + >>> + mcu_r5fss0_core0_dma_memory_region: r5f-dma-memory@9c000000 { >>> + compatible = "shared-dma-pool"; >>> + reg = <0x00 0x9c000000 0x00 0x100000>; >>> + no-map; >>> + }; >>> + >>> + mcu_r5fss0_core0_memory_region: r5f-memory@9c100000 { >>> + compatible = "shared-dma-pool"; >>> + reg = <0x00 0x9c100000 0x00 0x700000>; >>> + no-map; >>> + }; >>> + }; >>> + >>> + cbass_main: interconnect@100000 { >> >> bus@... > > Yeah, will update the example to use bus. The DTS nodes in the kernel > are already using the interconnect name. > >> >> Doesn't look like the right address either. > > Yeah, I skipped the actual first entry from the ranges, and only > mentioned the ones that I am using in the nodes. Will fix this. > >> >>> + compatible = "simple-bus"; >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + ranges = <0x00 0x41000000 0x00 0x41000000 0x00 0x00020000>, >>> + <0x00 0x41400000 0x00 0x41400000 0x00 0x00020000>, >>> + <0x00 0x41c00000 0x00 0x41c00000 0x00 0x00080000>; >>> + >>> + cbass_mcu: interconnect@28380000 { >> >> Doesn't look like the right address. > > Same as above. > >> >>> + compatible = "simple-bus"; >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + ranges = <0x00 0x41000000 0x00 0x41000000 0x00 0x00020000>, /* MCU R5F Core0 */ >>> + <0x00 0x41400000 0x00 0x41400000 0x00 0x00020000>, /* MCU R5F Core1 */ >>> + <0x00 0x41c00000 0x00 0x41c00000 0x00 0x00080000>; /* MCU SRAM */ >>> + >>> + /* MCU domain SRAM node */ >>> + mcu_ram: mcu-ram@41c00000 { >> >> I would omit this node from the example. Nothing special here really. > > Showcasing the optional sram property usage from the mcu_r5f0 node. > > regards > Suman > >> >>> + compatible = "mmio-sram"; >>> + reg = <0x00 0x41c00000 0x00 0x80000>; >>> + ranges = <0x0 0x00 0x41c00000 0x80000>; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + >>> + mcu_r5fss0_core0_sram: r5f-sram@0 { >>> + reg = <0x0 0x40000>; >>> + }; >>> + }; >>> + >>> + /* AM65x MCU R5FSS node */ >>> + mcu_r5fss0: r5fss@41000000 { >>> + compatible = "ti,am654-r5fss"; >>> + power-domains = <&k3_pds 129>; >>> + lockstep-mode = <1>; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges = <0x41000000 0x00 0x41000000 0x20000>, >>> + <0x41400000 0x00 0x41400000 0x20000>; >>> + >>> + mcu_r5f0: r5f@41000000 { >>> + compatible = "ti,am654-r5f"; >>> + reg = <0x41000000 0x00008000>, >>> + <0x41010000 0x00008000>; >>> + reg-names = "atcm", "btcm"; >>> + ti,sci = <&dmsc>; >>> + ti,sci-dev-id = <159>; >>> + ti,sci-proc-ids = <0x01 0xFF>; >>> + resets = <&k3_reset 159 1>; >>> + firmware-name = "am65x-mcu-r5f0_0-fw"; >>> + atcm-enable = <1>; >>> + btcm-enable = <1>; >>> + loczrama = <1>; >>> + mboxes = <&mailbox0 &mbox_mcu_r5fss0_core0>; >>> + memory-region = <&mcu_r5fss0_core0_dma_memory_region>, >>> + <&mcu_r5fss0_core0_memory_region>; >>> + sram = <&mcu_r5fss0_core0_sram>; >>> + }; >>> + >>> + mcu_r5f1: r5f@41400000 { >>> + compatible = "ti,am654-r5f"; >>> + reg = <0x41400000 0x00008000>, >>> + <0x41410000 0x00008000>; >>> + reg-names = "atcm", "btcm"; >>> + ti,sci = <&dmsc>; >>> + ti,sci-dev-id = <245>; >>> + ti,sci-proc-ids = <0x02 0xFF>; >>> + resets = <&k3_reset 245 1>; >>> + firmware-name = "am65x-mcu-r5f0_1-fw"; >>> + atcm-enable = <1>; >>> + btcm-enable = <1>; >>> + loczrama = <1>; >>> + mboxes = <&mailbox1 &mbox_mcu_r5fss0_core1>; >>> + }; >>> + }; >>> + }; >>> + }; >>> -- >>> 2.23.0 >>> >
