diff mbox series

[v2,6/9] dt-bindings: dma: add fsl-edma3 yaml

Message ID 1594748508-22179-7-git-send-email-yibin.gong@nxp.com (mailing list archive)
State Changes Requested
Headers show
Series add fsl-edma3 support | expand

Commit Message

Robin Gong July 14, 2020, 5:41 p.m. UTC 
Add device binding doc for fsl-edma3 driver.

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 .../devicetree/bindings/dma/nxp,fsl-edma3.yaml     | 134 +++++++++++++++++++++
 1 file changed, 134 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/nxp,fsl-edma3.yaml

Comments

Rob Herring (Arm) July 16, 2020, 7:47 p.m. UTC | #1 
On Wed, Jul 15, 2020 at 01:41:45AM +0800, Robin Gong wrote:
> Add device binding doc for fsl-edma3 driver.
> 
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
>  .../devicetree/bindings/dma/nxp,fsl-edma3.yaml     | 134 +++++++++++++++++++++
>  1 file changed, 134 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/nxp,fsl-edma3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/dma/nxp,fsl-edma3.yaml b/Documentation/devicetree/bindings/dma/nxp,fsl-edma3.yaml
> new file mode 100644
> index 00000000..ebdad90
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/nxp,fsl-edma3.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/nxp,fsl-edma3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP eDMA3 Controller
> +
> +maintainers:
> +  - Robin Gong <yibin.gong@nxp.com>
> +
> +allOf:
> +  - $ref: "dma-controller.yaml#"
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - fsl,imx8qm-edma  # i.mx8qm/qxp
> +
> +  reg:
> +    minItems: 2
> +    maxItems: 32

Needs some sort of description as to what each region is.
 
> +
> +  interrupts:
> +    minItems: 2
> +    maxItems: 32

ditto

> +
> +  interrupt-names:
> +    minItems: 2
> +    maxItems: 32
> +    items:
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"

What about interrupts 8-31? If you want a pattern for all entries, you 
do:

items:
  pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"

(If 'items' is a schema instead of a list, then it applies to all 
entries.)

What does edma[0-2] correspond to? The names should be local to the 
instance.

Really, what's the point of names that just have the channel number in 
them? The driver can create names dynamically if needed. We already have 
properties to define how many channels and a mask of present channels.

> +
> +  '#dma-cells':
> +    const: 3
> +    description: |
> +      The 1st cell specifies the channel ID.
> +
> +      The 2nd cell specifies the channel priority.
> +
> +      The 3rd cell specifies the channel attributes:
> +        bit0 transmit or receive.
> +          0 = transmit
> +          1 = receive
> +        bit1 local or remote access.
> +          0 = local
> +          1 = remote
> +        bit2 dualfifo case or not(only in Audio cyclic now).
> +          0 = not dual fifo case
> +          1 =  dualfifo case.
> +
> +  dma-channels:
> +    minimum: 2
> +    maximum: 32
> +
> +  power-domains:
> +    minItems: 2
> +    maxItems: 32
> +
> +  power-domain-names:
> +    minItems: 2
> +    maxItems: 32
> +    items:
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"

Again, why do you need names here?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - '#dma-cells'
> +  - dma-channels
> +  - power-domains
> +  - power-domain-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/firmware/imx/rsrc.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    edma2: dma-controller@5a1f0000 {
> +        compatible = "fsl,imx8qm-edma";
> +        reg = <0x5a280000 0x10000>, /* channel8 UART0 rx */
> +              <0x5a290000 0x10000>, /* channel9 UART0 tx */
> +              <0x5a2a0000 0x10000>, /* channel10 UART1 rx */
> +              <0x5a2b0000 0x10000>, /* channel11 UART1 tx */
> +              <0x5a2c0000 0x10000>, /* channel12 UART2 rx */
> +              <0x5a2d0000 0x10000>, /* channel13 UART2 tx */
> +              <0x5a2e0000 0x10000>, /* channel14 UART3 rx */
> +              <0x5a2f0000 0x10000>; /* channel15 UART3 tx */
> +        #dma-cells = <3>;
> +        dma-channels = <8>;
> +        interrupts = <GIC_SPI 434 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 435 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 436 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 437 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 438 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 439 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 440 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 441 IRQ_TYPE_LEVEL_HIGH>;
> +       interrupt-names = "edma2-chan8-rx", "edma2-chan9-tx",
> +                         "edma2-chan10-rx", "edma2-chan11-tx",
> +                         "edma2-chan12-rx", "edma2-chan13-tx",
> +                         "edma2-chan14-rx", "edma2-chan15-tx";
> +       power-domains = <&pd IMX_SC_R_DMA_2_CH8>,
> +                       <&pd IMX_SC_R_DMA_2_CH9>,
> +                       <&pd IMX_SC_R_DMA_2_CH10>,
> +                       <&pd IMX_SC_R_DMA_2_CH11>,
> +                       <&pd IMX_SC_R_DMA_2_CH12>,
> +                       <&pd IMX_SC_R_DMA_2_CH13>,
> +                       <&pd IMX_SC_R_DMA_2_CH14>,
> +                       <&pd IMX_SC_R_DMA_2_CH15>;
> +       power-domain-names = "edma2-chan8", "edma2-chan9",
> +                            "edma2-chan10", "edma2-chan11",
> +                            "edma2-chan12", "edma2-chan13",
> +                            "edma2-chan14", "edma2-chan15";
> +    };
> -- 
> 2.7.4
>
Robin Gong July 24, 2020, 10:13 a.m. UTC | #2 
On 2020/07/17 3:48 Rob Herring <robh@kernel.org> wrote:
> > +
> > +  reg:
> > +    minItems: 2
> > +    maxItems: 32
> 
> Needs some sort of description as to what each region is.
Okay, will add it in v3.
> 
> > +
> > +  interrupts:
> > +    minItems: 2
> > +    maxItems: 32
> 
> ditto
Will add it in v3.
> 
> > +
> > +  interrupt-names:
> > +    minItems: 2
> > +    maxItems: 32
> > +    items:
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> 
> What about interrupts 8-31? If you want a pattern for all entries, you
Here 8 lines matches with the 8 channel used in dts (same example as the
bottom of this yaml file below), otherwise, below warning comes out with
'make dt_binding_check' if I remove the above last line:
dma-controller@5a1f0000: interrupt-names: Additional items are not allowed ('edma2-chan15-tx' was unexpected)

Maybe there something I miss, please correct me if I'm wrong, thanks.

> do:
> 
> items:
>   pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
> 
> (If 'items' is a schema instead of a list, then it applies to all
> entries.)
> 
> What does edma[0-2] correspond to? The names should be local to the
> instance.
There are 3 edma controllers on i.mx8QXP/QM, [0-2] is controller index.
Clear controller index here may make dts readable since every edma channel
has unique register map, interrupt number, power domain.

> 
> Really, what's the point of names that just have the channel number in them?
> The driver can create names dynamically if needed. We already have
> properties to define how many channels and a mask of present channels.
interrupt-names/power-domain-names here just for better readable in dts, besides, the clear interrupt name for every channel including edma controller
id which passed by dts could be used to register irq.

> 
> > +
> > +  '#dma-cells':
> > +    const: 3
> > +    description: |
> > +      The 1st cell specifies the channel ID.
> > +
> > +      The 2nd cell specifies the channel priority.
> > +
> > +      The 3rd cell specifies the channel attributes:
> > +        bit0 transmit or receive.
> > +          0 = transmit
> > +          1 = receive
> > +        bit1 local or remote access.
> > +          0 = local
> > +          1 = remote
> > +        bit2 dualfifo case or not(only in Audio cyclic now).
> > +          0 = not dual fifo case
> > +          1 =  dualfifo case.
> > +
> > +  dma-channels:
> > +    minimum: 2
> > +    maximum: 32
> > +
> > +  power-domains:
> > +    minItems: 2
> > +    maxItems: 32
> > +
> > +  power-domain-names:
> > +    minItems: 2
> > +    maxItems: 32
> > +    items:
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> > +      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
> 
> Again, why do you need names here?
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-names
> > +  - '#dma-cells'
> > +  - dma-channels
> > +  - power-domains
> > +  - power-domain-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/firmware/imx/rsrc.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    edma2: dma-controller@5a1f0000 {
> > +        compatible = "fsl,imx8qm-edma";
> > +        reg = <0x5a280000 0x10000>, /* channel8 UART0 rx */
> > +              <0x5a290000 0x10000>, /* channel9 UART0 tx */
> > +              <0x5a2a0000 0x10000>, /* channel10 UART1 rx */
> > +              <0x5a2b0000 0x10000>, /* channel11 UART1 tx */
> > +              <0x5a2c0000 0x10000>, /* channel12 UART2 rx */
> > +              <0x5a2d0000 0x10000>, /* channel13 UART2 tx */
> > +              <0x5a2e0000 0x10000>, /* channel14 UART3 rx */
> > +              <0x5a2f0000 0x10000>; /* channel15 UART3 tx */
> > +        #dma-cells = <3>;
> > +        dma-channels = <8>;
> > +        interrupts = <GIC_SPI 434 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 435 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 436 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 437 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 438 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 439 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 440 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 441 IRQ_TYPE_LEVEL_HIGH>;
> > +       interrupt-names = "edma2-chan8-rx", "edma2-chan9-tx",
> > +                         "edma2-chan10-rx", "edma2-chan11-tx",
> > +                         "edma2-chan12-rx", "edma2-chan13-tx",
> > +                         "edma2-chan14-rx", "edma2-chan15-tx";
> > +       power-domains = <&pd IMX_SC_R_DMA_2_CH8>,
> > +                       <&pd IMX_SC_R_DMA_2_CH9>,
> > +                       <&pd IMX_SC_R_DMA_2_CH10>,
> > +                       <&pd IMX_SC_R_DMA_2_CH11>,
> > +                       <&pd IMX_SC_R_DMA_2_CH12>,
> > +                       <&pd IMX_SC_R_DMA_2_CH13>,
> > +                       <&pd IMX_SC_R_DMA_2_CH14>,
> > +                       <&pd IMX_SC_R_DMA_2_CH15>;
> > +       power-domain-names = "edma2-chan8", "edma2-chan9",
> > +                            "edma2-chan10", "edma2-chan11",
> > +                            "edma2-chan12", "edma2-chan13",
> > +                            "edma2-chan14", "edma2-chan15";
> > +    };
> > --
> > 2.7.4
> >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/dma/nxp,fsl-edma3.yaml b/Documentation/devicetree/bindings/dma/nxp,fsl-edma3.yaml
new file mode 100644
index 00000000..ebdad90
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/nxp,fsl-edma3.yaml
@@ -0,0 +1,134 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/nxp,fsl-edma3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP eDMA3 Controller
+
+maintainers:
+  - Robin Gong <yibin.gong@nxp.com>
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - fsl,imx8qm-edma  # i.mx8qm/qxp
+
+  reg:
+    minItems: 2
+    maxItems: 32
+
+  interrupts:
+    minItems: 2
+    maxItems: 32
+
+  interrupt-names:
+    minItems: 2
+    maxItems: 32
+    items:
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
+
+  '#dma-cells':
+    const: 3
+    description: |
+      The 1st cell specifies the channel ID.
+
+      The 2nd cell specifies the channel priority.
+
+      The 3rd cell specifies the channel attributes:
+        bit0 transmit or receive.
+          0 = transmit
+          1 = receive
+        bit1 local or remote access.
+          0 = local
+          1 = remote
+        bit2 dualfifo case or not(only in Audio cyclic now).
+          0 = not dual fifo case
+          1 =  dualfifo case.
+
+  dma-channels:
+    minimum: 2
+    maximum: 32
+
+  power-domains:
+    minItems: 2
+    maxItems: 32
+
+  power-domain-names:
+    minItems: 2
+    maxItems: 32
+    items:
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
+      - pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])+$"
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - '#dma-cells'
+  - dma-channels
+  - power-domains
+  - power-domain-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/firmware/imx/rsrc.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    edma2: dma-controller@5a1f0000 {
+        compatible = "fsl,imx8qm-edma";
+        reg = <0x5a280000 0x10000>, /* channel8 UART0 rx */
+              <0x5a290000 0x10000>, /* channel9 UART0 tx */
+              <0x5a2a0000 0x10000>, /* channel10 UART1 rx */
+              <0x5a2b0000 0x10000>, /* channel11 UART1 tx */
+              <0x5a2c0000 0x10000>, /* channel12 UART2 rx */
+              <0x5a2d0000 0x10000>, /* channel13 UART2 tx */
+              <0x5a2e0000 0x10000>, /* channel14 UART3 rx */
+              <0x5a2f0000 0x10000>; /* channel15 UART3 tx */
+        #dma-cells = <3>;
+        dma-channels = <8>;
+        interrupts = <GIC_SPI 434 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 435 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 436 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 437 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 438 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 439 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 440 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 441 IRQ_TYPE_LEVEL_HIGH>;
+       interrupt-names = "edma2-chan8-rx", "edma2-chan9-tx",
+                         "edma2-chan10-rx", "edma2-chan11-tx",
+                         "edma2-chan12-rx", "edma2-chan13-tx",
+                         "edma2-chan14-rx", "edma2-chan15-tx";
+       power-domains = <&pd IMX_SC_R_DMA_2_CH8>,
+                       <&pd IMX_SC_R_DMA_2_CH9>,
+                       <&pd IMX_SC_R_DMA_2_CH10>,
+                       <&pd IMX_SC_R_DMA_2_CH11>,
+                       <&pd IMX_SC_R_DMA_2_CH12>,
+                       <&pd IMX_SC_R_DMA_2_CH13>,
+                       <&pd IMX_SC_R_DMA_2_CH14>,
+                       <&pd IMX_SC_R_DMA_2_CH15>;
+       power-domain-names = "edma2-chan8", "edma2-chan9",
+                            "edma2-chan10", "edma2-chan11",
+                            "edma2-chan12", "edma2-chan13",
+                            "edma2-chan14", "edma2-chan15";
+    };