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

[Robin Gong <yibin.gong@nxp.com>]

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