| Message ID | 20230302171450.1598576-2-martyn.welch@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | TI K3 M4F support on AM64x and AM62x SoCs | expand |
Hi Martyn, Due to the size of this patch my comments will likely span a few days. I will clearly let you know when I am done reviewing your submission. On Thu, Mar 02, 2023 at 05:14:48PM +0000, Martyn Welch wrote: > From: Hari Nagalla <hnagalla@ti.com> > > K3 AM64x SoC has a Cortex M4F subsystem in the MCU voltage domain. > The remote processor's life cycle management and IPC mechanisms are > similar across the R5F and M4F cores from remote processor driver > point of view. However, there are subtle differences in image loading > and starting the M4F subsystems. > > The YAML binding document provides the various node properties to be > configured by the consumers of the M4F subsystem. > > Signed-off-by: Hari Nagalla <hnagalla@ti.com> > [Martyn Welch: Amended as per review comments and to pass DT tests] This isn't needed. Since you are taking over this patchset it is expected you will make modifications. > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > --- > > Changes since v1: > - Spelling corrections > - Corrected to pass DT checks > > Changes since v2: > - Missed spelling correction to commit message > > Note: The only review comment that I don't see directly addressed is the > lack of description of `ti,sci`, `ti,sci-dev-id` and > `ti,sci-proc-ids`. A reference has been added to > `/schemas/arm/keystone/ti,k3-sci-common.yaml#` where they are > described. I believe this is the correct approach, please advise if > that is not the case. > > .../bindings/remoteproc/ti,k3-m4f-rproc.yaml | 158 ++++++++++++++++++ > 1 file changed, 158 insertions(+) > create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml > > diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml > new file mode 100644 > index 000000000000..1b38df0be2e6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml > @@ -0,0 +1,158 @@ > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/remoteproc/ti,k3-m4f-rproc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: TI K3 M4F processor subsystems > + > +maintainers: > + - Hari Nagalla <hnagalla@ti.com> > + Hari did not finish upstreaming this set. As such I doubt he intents to maintain it and I suppose it is the same for you since you didn't add your name either. You can add my name there. > +description: | > + Some K3 family SoCs have Arm Cortex M4F cores. AM64x is a SoC in K3 > + family with a M4F core. Typically safety oriented applications may use > + the M4F core in isolation without an IPC. Where as some industrial and > + home automation applications, may use the M4F core as a remote processor > + with IPC communications. > + > +$ref: /schemas/arm/keystone/ti,k3-sci-common.yaml# > + > +properties: > + $nodename: > + pattern: "^m4fss(@.*)?" > + > + compatible: > + enum: > + - ti,am64-m4fss > + > + power-domains: > + description: | > + Should contain a phandle to a PM domain provider node and an args > + specifier containing the M4FSS device id value. > + maxItems: 1 > + > + "#address-cells": > + const: 2 > + > + "#size-cells": > + const: 2 > + > + reg: > + items: > + - description: Address and Size of the IRAM internal memory region > + - description: Address and Size of the DRAM internal memory region > + > + reg-names: > + items: > + - const: iram > + - const: dram > + > + resets: > + description: | > + Should contain the phandle to the reset controller node managing the > + local resets for this device, and a reset specifier. > + maxItems: 1 > + > + firmware-name: > + description: | > + Should contain the name of the default firmware image > + file located on the firmware search path > + > + 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. > + maxItems: 1 > + > + memory-region: > + description: | > + phandle to the reserved memory nodes to be associated with the > + remoteproc device. There should be at least two reserved memory nodes > + defined. The reserved memory nodes should be carveout nodes, and > + should be defined with a "no-map" property as per the bindings in > + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml > + minItems: 2 > + maxItems: 8 > + items: > + - description: region used for dynamic DMA allocations like vrings and > + vring buffers > + - description: region reserved for firmware image sections > + additionalItems: true > + > +required: > + - compatible > + - reg > + - reg-names > + - ti,sci > + - ti,sci-dev-id > + - ti,sci-proc-ids > + - resets > + - firmware-name > + - mboxes The 'mboxes' property is marked as required but the description section above clearly state the M4F can operate without IPC. > + - memory-region > + > +unevaluatedProperties: false > + > +examples: > + - | > + reserved-memory { > + #address-cells = <2>; > + #size-cells = <2>; > + > + mcu_m4fss_dma_memory_region: m4f-dma-memory@9cb00000 { > + compatible = "shared-dma-pool"; > + reg = <0x00 0x9cb00000 0x00 0x100000>; > + no-map; > + }; > + > + mcu_m4fss_memory_region: m4f-memory@9cc00000 { > + compatible = "shared-dma-pool"; > + reg = <0x00 0x9cc00000 0x00 0xe00000>; > + no-map; > + }; > + }; > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + mailbox0_cluster0: mailbox-0 { > + #mbox-cells = <1>; > + > + mbox_m4_0: mbox-m4-0 { > + ti,mbox-rx = <0 0 0>; > + ti,mbox-tx = <1 0 0>; > + }; > + }; > + > + bus@f0000 { > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges = <0x00 0x04000000 0x00 0x04000000 0x00 0x01ff1400>; > + > + bus@4000000 { > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges = <0x00 0x04000000 0x00 0x04000000 0x00 0x01ff1400>; > + > + mcu_m4fss: m4fss@5000000 { > + compatible = "ti,am64-m4fss"; > + reg = <0x00 0x5000000 0x00 0x30000>, > + <0x00 0x5040000 0x00 0x10000>; > + reg-names = "iram", "dram"; > + ti,sci = <&dmsc>; > + ti,sci-dev-id = <9>; > + ti,sci-proc-ids = <0x18 0xff>; > + resets = <&k3_reset 9 1>; > + firmware-name = "am62-mcu-m4f0_0-fw"; > + mboxes = <&mailbox0_cluster0 &mbox_m4_0>; This doesn't look right since mbox_m4_0 is defined within mailbox0_cluster0. > + memory-region = <&mcu_m4fss_dma_memory_region>, > + <&mcu_m4fss_memory_region>; > + }; > + }; > + }; > + }; > -- > 2.39.1 >
On Fri, 2023-03-10 at 08:41 -0700, Mathieu Poirier wrote: > On Thu, Mar 09, 2023 at 05:18:01PM -0600, Hari Nagalla wrote: > > On 3/8/23 14:58, Mathieu Poirier wrote: > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - reg-names > > > > + - ti,sci > > > > + - ti,sci-dev-id > > > > + - ti,sci-proc-ids > > > > + - resets > > > > + - firmware-name > > > > + - mboxes > > > The 'mboxes' property is marked as required but the description > > > section above > > > clearly state the M4F can operate without IPC. > > > > > Well, when the M4F is used as a safety processor it is typically > > booted from > > SBL/u-boot and may isolate the MCU domain from main domain/A53 to > > function > > in higher safety level. In these scenarios there is no remote proc > > handling > > of M4F life cycle management (LCM) and IPC. But, on the other hand, > > when the > > M4F is used as a non safety processor its LCM is handled by remote > > proc(main > > domain) and mailboxes for IPC are required. > > Well, what you wrote above is pretty much explained verbatim in the > "description" section of the bindings. Mailboxes are optional and as > such > should not be found under the "required" section. > Which means the memory regions are also optional as in the isolated case they're be no communications with the main domain. Martyn
On Wed, 2023-03-08 at 13:58 -0700, Mathieu Poirier wrote: > On Thu, Mar 02, 2023 at 05:14:48PM +0000, Martyn Welch wrote: > > > +examples: > > + - | > > + reserved-memory { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + mcu_m4fss_dma_memory_region: m4f-dma-memory@9cb00000 { > > + compatible = "shared-dma-pool"; > > + reg = <0x00 0x9cb00000 0x00 0x100000>; > > + no-map; > > + }; > > + > > + mcu_m4fss_memory_region: m4f-memory@9cc00000 { > > + compatible = "shared-dma-pool"; > > + reg = <0x00 0x9cc00000 0x00 0xe00000>; > > + no-map; > > + }; > > + }; > > + > > + soc { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + mailbox0_cluster0: mailbox-0 { > > + #mbox-cells = <1>; > > + > > + mbox_m4_0: mbox-m4-0 { > > + ti,mbox-rx = <0 0 0>; > > + ti,mbox-tx = <1 0 0>; > > + }; > > + }; > > + > > + bus@f0000 { > > + compatible = "simple-bus"; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges = <0x00 0x04000000 0x00 0x04000000 0x00 > > 0x01ff1400>; > > + > > + bus@4000000 { > > + compatible = "simple-bus"; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges = <0x00 0x04000000 0x00 0x04000000 0x00 > > 0x01ff1400>; > > + > > + mcu_m4fss: m4fss@5000000 { > > + compatible = "ti,am64-m4fss"; > > + reg = <0x00 0x5000000 0x00 0x30000>, > > + <0x00 0x5040000 0x00 0x10000>; > > + reg-names = "iram", "dram"; > > + ti,sci = <&dmsc>; > > + ti,sci-dev-id = <9>; > > + ti,sci-proc-ids = <0x18 0xff>; > > + resets = <&k3_reset 9 1>; > > + firmware-name = "am62-mcu-m4f0_0-fw"; > > + mboxes = <&mailbox0_cluster0 &mbox_m4_0>; > > This doesn't look right since mbox_m4_0 is defined within > mailbox0_cluster0. > Looking at other users of the omap mailboxes (and not wanting to spend ages trawling back through the mailing list archives to try and work out why it's specified the way it is) it seems that this is the way that these mailboxes are specified. For instance in `arch/arm/boot/dts/omap4-l4.dtsi`, the mailbox is defined: mailbox: mailbox@0 { compatible = "ti,omap4-mailbox"; reg = <0x0 0x200>; interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>; #mbox-cells = <1>; ti,mbox-num-users = <3>; ti,mbox-num-fifos = <8>; mbox_ipu: mbox-ipu { ti,mbox-tx = <0 0 0>; ti,mbox-rx = <1 0 0>; }; mbox_dsp: mbox-dsp { ti,mbox-tx = <3 0 0>; ti,mbox-rx = <2 0 0>; }; }; It's users in `arch/arm/boot/dts/omap4.dtsi`: dsp: dsp { ... mboxes = <&mailbox &mbox_dsp>; ... }; ipu: ipu@55020000 { ... mboxes = <&mailbox &mbox_ipu>; ... }; > > + memory-region = > > <&mcu_m4fss_dma_memory_region>, > > + <&mcu_m4fss_memory_region>; > > + }; > > + }; > > + }; > > + }; > > -- > > 2.39.1 > >
diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml new file mode 100644 index 000000000000..1b38df0be2e6 --- /dev/null +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml @@ -0,0 +1,158 @@ +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/remoteproc/ti,k3-m4f-rproc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI K3 M4F processor subsystems + +maintainers: + - Hari Nagalla <hnagalla@ti.com> + +description: | + Some K3 family SoCs have Arm Cortex M4F cores. AM64x is a SoC in K3 + family with a M4F core. Typically safety oriented applications may use + the M4F core in isolation without an IPC. Where as some industrial and + home automation applications, may use the M4F core as a remote processor + with IPC communications. + +$ref: /schemas/arm/keystone/ti,k3-sci-common.yaml# + +properties: + $nodename: + pattern: "^m4fss(@.*)?" + + compatible: + enum: + - ti,am64-m4fss + + power-domains: + description: | + Should contain a phandle to a PM domain provider node and an args + specifier containing the M4FSS device id value. + maxItems: 1 + + "#address-cells": + const: 2 + + "#size-cells": + const: 2 + + reg: + items: + - description: Address and Size of the IRAM internal memory region + - description: Address and Size of the DRAM internal memory region + + reg-names: + items: + - const: iram + - const: dram + + resets: + description: | + Should contain the phandle to the reset controller node managing the + local resets for this device, and a reset specifier. + maxItems: 1 + + firmware-name: + description: | + Should contain the name of the default firmware image + file located on the firmware search path + + 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. + maxItems: 1 + + memory-region: + description: | + phandle to the reserved memory nodes to be associated with the + remoteproc device. There should be at least two reserved memory nodes + defined. The reserved memory nodes should be carveout nodes, and + should be defined with a "no-map" property as per the bindings in + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml + minItems: 2 + maxItems: 8 + items: + - description: region used for dynamic DMA allocations like vrings and + vring buffers + - description: region reserved for firmware image sections + additionalItems: true + +required: + - compatible + - reg + - reg-names + - ti,sci + - ti,sci-dev-id + - ti,sci-proc-ids + - resets + - firmware-name + - mboxes + - memory-region + +unevaluatedProperties: false + +examples: + - | + reserved-memory { + #address-cells = <2>; + #size-cells = <2>; + + mcu_m4fss_dma_memory_region: m4f-dma-memory@9cb00000 { + compatible = "shared-dma-pool"; + reg = <0x00 0x9cb00000 0x00 0x100000>; + no-map; + }; + + mcu_m4fss_memory_region: m4f-memory@9cc00000 { + compatible = "shared-dma-pool"; + reg = <0x00 0x9cc00000 0x00 0xe00000>; + no-map; + }; + }; + + soc { + #address-cells = <2>; + #size-cells = <2>; + + mailbox0_cluster0: mailbox-0 { + #mbox-cells = <1>; + + mbox_m4_0: mbox-m4-0 { + ti,mbox-rx = <0 0 0>; + ti,mbox-tx = <1 0 0>; + }; + }; + + bus@f0000 { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + ranges = <0x00 0x04000000 0x00 0x04000000 0x00 0x01ff1400>; + + bus@4000000 { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + ranges = <0x00 0x04000000 0x00 0x04000000 0x00 0x01ff1400>; + + mcu_m4fss: m4fss@5000000 { + compatible = "ti,am64-m4fss"; + reg = <0x00 0x5000000 0x00 0x30000>, + <0x00 0x5040000 0x00 0x10000>; + reg-names = "iram", "dram"; + ti,sci = <&dmsc>; + ti,sci-dev-id = <9>; + ti,sci-proc-ids = <0x18 0xff>; + resets = <&k3_reset 9 1>; + firmware-name = "am62-mcu-m4f0_0-fw"; + mboxes = <&mailbox0_cluster0 &mbox_m4_0>; + memory-region = <&mcu_m4fss_dma_memory_region>, + <&mcu_m4fss_memory_region>; + }; + }; + }; + };