Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Robin Gong <yibin.gong@nxp.com>
To:     Rob Herring <robh@kernel.org>
CC:     "vkoul@kernel.org" <vkoul@kernel.org>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "festevam@gmail.com" <festevam@gmail.com>,
        "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "will@kernel.org" <will@kernel.org>,
        "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
        "angelo@sysam.it" <angelo@sysam.it>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>,
        dl-linux-imx <linux-imx@nxp.com>,
        "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH v2 6/9] dt-bindings: dma: add fsl-edma3 yaml
Thread-Topic: [PATCH v2 6/9] dt-bindings: dma: add fsl-edma3 yaml
Thread-Index: AQHWWcEHFW49eRTRd0i86/lmLEZUy6kKoHIAgApw4+A=
Date:   Fri, 24 Jul 2020 10:13:41 +0000
Message-ID: <VE1PR04MB6638C3006D405D9C8E13547C89770@VE1PR04MB6638.eurprd04.prod.outlook.com>
References: <1594748508-22179-1-git-send-email-yibin.gong@nxp.com>
 <1594748508-22179-7-git-send-email-yibin.gong@nxp.com>
 <20200716194746.GA2716872@bogus>
In-Reply-To: <20200716194746.GA2716872@bogus>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 43b7d7da-1d0c-4d6c-4e9a-08d82fba3d5e
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jul 2020 10:13:41.1169
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: yQ923lzmJLn9ZofjqoU6ncIIJvnvx6mG22/VGn1MUIVWhN8JEzppQnNovLydmIWv5e4Wu5SqTcBlkOGJLXCGOw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6640
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2020/07/17 3:48 Rob Herring <robh@kernel.org> wrote:
> > +
> > +  reg:
> > +    minItems: 2
> > +    maxItems: 32
>=20
> Needs some sort of description as to what each region is.
Okay, will add it in v3.
>=20
> > +
> > +  interrupts:
> > +    minItems: 2
> > +    maxItems: 32
>=20
> ditto
Will add it in v3.
>=20
> > +
> > +  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+$"
>=20
> 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:
>=20
> items:
>   pattern: "^edma[0-2]-chan([0-9]|[1-2][0-9]|3[0-1])-tx|rx+$"
>=20
> (If 'items' is a schema instead of a list, then it applies to all
> entries.)
>=20
> 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.

>=20
> Really, what's the point of names that just have the channel number in th=
em?
> 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, be=
sides, the clear interrupt name for every channel including edma controller
id which passed by dts could be used to register irq.

>=20
> > +
> > +  '#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 =3D transmit
> > +          1 =3D receive
> > +        bit1 local or remote access.
> > +          0 =3D local
> > +          1 =3D remote
> > +        bit2 dualfifo case or not(only in Audio cyclic now).
> > +          0 =3D not dual fifo case
> > +          1 =3D  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])+$"
>=20
> Again, why do you need names here?
>=20
> > +
> > +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 =3D "fsl,imx8qm-edma";
> > +        reg =3D <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 =3D <3>;
> > +        dma-channels =3D <8>;
> > +        interrupts =3D <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 =3D "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 =3D <&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 =3D "edma2-chan8", "edma2-chan9",
> > +                            "edma2-chan10", "edma2-chan11",
> > +                            "edma2-chan12", "edma2-chan13",
> > +                            "edma2-chan14", "edma2-chan15";
> > +    };
> > --
> > 2.7.4
> >