On 02/03/2023 18:14, Martyn Welch wrote: > From: Hari Nagalla <hnagalla@xxxxxx> Subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings. > > 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] > 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> > + > +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(@.*)?" Drop. It's not a generic name. Also we do not enforce names in device schemas. > + > + 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. Drop description, especially that the args depend on provider, not consumer. > + maxItems: 1 > + > + "#address-cells": > + const: 2 > + > + "#size-cells": > + const: 2 > + > + reg: > + items: > + - description: Address and Size of the IRAM internal memory region Just "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. Drop description. > + maxItems: 1 > + > + firmware-name: > + description: | > + Should contain the name of the default firmware image > + file located on the firmware search path This description is basically duplicating the name... say something useful or shorten it (e.g. "Should contain" is really redundant). You also need $ref because we do not have the type defined anywhere. > + > + mboxes: > + description: | > + OMAP Mailbox specifier denoting the sub-mailbox, to be used for OMAP? > + 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. Don't repeat constraints in free form text. > 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 And what is the purpose of the rest of reserved nodes? > + > +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>; > + }; > + }; Does not look related to this binding... or is it somehow very specific and needs showing? > + > + bus@f0000 { > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges = <0x00 0x04000000 0x00 0x04000000 0x00 0x01ff1400>; Why another bus? You already have soc in example, why more nodes? > + > + bus@4000000 { And one more bus? > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges = <0x00 0x04000000 0x00 0x04000000 0x00 0x01ff1400>; > + > + mcu_m4fss: m4fss@5000000 { Generic node name. Qualcomm uses remoteproc. > + 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>; > + }; > + }; > + }; > + }; Best regards, Krzysztof
