[PATCH v3 0/3] TI K3 M4F support on AM64x and AM62x SoCs

[PATCH v3 0/3] TI K3 M4F support on AM64x and AM62x SoCs

[Martyn Welch]

The following series introduces K3 M4F remoteproc driver support for
AM64x and AM62x SoC families. These SoCs have a ARM Cortex M4F core in
the MCU voltage domain. For safety oriented applications, this core is
operated independently with out any IPC to other cores on the SoC.
However, for non safety applications, some customers use it as a remote
processor and so linux remote proc support is extended to the M4F core.

See AM64x Technical Reference Manual (SPRUIM2C – SEPTEMBER 2021) for
further details: https://www.ti.com/lit/pdf/SPRUIM2

This series was originally submitted by Hari in Jan 2022. Review
comments were made, however I've been unable to find any further
submissions. I have tried to contact the author and have received no
reply. As we are interested in using this functionality, we have decided
to pick this up and see if we can get it merged.


Hari Nagalla (1):
  dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

Martyn Welch (2):
  remoteproc: k4: Split out functions common with M4 driver
  remoteproc: k4-m4: Add a remoteproc driver for M4F subsystem

 .../bindings/remoteproc/ti,k3-m4f-rproc.yaml  | 158 ++++++
 drivers/remoteproc/Kconfig                    |  13 +
 drivers/remoteproc/Makefile                   |   3 +-
 drivers/remoteproc/ti_k3_common.c             | 375 +++++++++++++
 drivers/remoteproc/ti_k3_common.h             | 107 ++++
 drivers/remoteproc/ti_k3_dsp_remoteproc.c     | 462 +---------------
 drivers/remoteproc/ti_k3_m4_remoteproc.c      | 491 ++++++++++++++++++
 7 files changed, 1174 insertions(+), 435 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml
 create mode 100644 drivers/remoteproc/ti_k3_common.c
 create mode 100644 drivers/remoteproc/ti_k3_common.h
 create mode 100644 drivers/remoteproc/ti_k3_m4_remoteproc.c

-- 
2.39.1

[PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Martyn Welch]

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]
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>
+
+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>;
+                };
+            };
+        };
+    };
-- 
2.39.1

Re: [PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Krzysztof Kozlowski]

On 02/03/2023 18:14, Martyn Welch wrote:
> From: Hari Nagalla <hnagalla@ti.com>

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@ti.com>
> [Martyn Welch: Amended as per review comments and to pass DT tests]
> 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>
> +
> +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

Re: [PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Martyn Welch]

On Fri, 2023-03-03 at 09:06 +0100, Krzysztof Kozlowski wrote:
> On 02/03/2023 18:14, Martyn Welch wrote:
> 
> > +
> > +  mboxes:
> > +    description: |
> > +      OMAP Mailbox specifier denoting the sub-mailbox, to be used
> > for
> 
> OMAP?
> 

This device uses a mailbox compatible with the OMAP Mailbox, as defined
in Documentation/devicetree/bindings/mailbox/ti,omap-mailbox.yaml.

I note that documents title reads "TI OMAP2+ and K3 Mailbox devices".
I'll drop the "OMAP" here.

> > +      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?
> 

Up to 8 regions can be specified with their purpose determined by the
firmware running on the M4F core. The suggestion (and the
implementation in the example firmware) is to use the first 2 regions
as defined above for remoteproc with the others available to be used
for other purposes if necessary. The address translation module used
can cope with up to 8 regions.

> 

Martyn

Re: [PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Krzysztof Kozlowski]

On 07/03/2023 19:26, Martyn Welch wrote:
> On Fri, 2023-03-03 at 09:06 +0100, Krzysztof Kozlowski wrote:
>> On 02/03/2023 18:14, Martyn Welch wrote:
>>
>>> +
>>> +  mboxes:
>>> +    description: |
>>> +      OMAP Mailbox specifier denoting the sub-mailbox, to be used
>>> for
>>
>> OMAP?
>>
> 
> This device uses a mailbox compatible with the OMAP Mailbox, as defined
> in Documentation/devicetree/bindings/mailbox/ti,omap-mailbox.yaml.
> 
> I note that documents title reads "TI OMAP2+ and K3 Mailbox devices".
> I'll drop the "OMAP" here.
> 
>>> +      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?
>>
> 
> Up to 8 regions can be specified with their purpose determined by the
> firmware running on the M4F core. The suggestion (and the
> implementation in the example firmware) is to use the first 2 regions
> as defined above for remoteproc with the others available to be used
> for other purposes if necessary. The address translation module used
> can cope with up to 8 regions.

Then mention the rest is just dedicated to the firmware and it's purpose
depends on the firmware.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Mathieu Poirier]

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
> 

Re: [PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Hari Nagalla]

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.

Re: [PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Mathieu Poirier]

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.

Re: [PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Martyn Welch]

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

Re: [PATCH v3 1/3] dt-bindings: remoteproc: k3-m4f: Add bindings for K3 AM64x SoCs

[Martyn Welch]

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
> >