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@xxxxxx> > > 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@xxxxxx> > [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@xxxxxxxxxxxxx> > --- > > 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@xxxxxx> > + 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 >
