| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250824185543.475785-1-ninozhang001@gmail.com>
Date: Mon, 25 Aug 2025 02:55:43 +0800
From: Nino Zhang <ninozhang001@...il.com>
To: robh@...nel.org,
krzk+dt@...nel.org
Cc: conor+dt@...nel.org,
devicetree@...r.kernel.org,
dmaengine@...r.kernel.org,
linux-kernel@...r.kernel.org,
vkoul@...nel.org,
rahulbedarkar89@...il.com,
linux-mips@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: dma: img-mdc-dma: convert to DT schema
On Fri, 22 Aug 2025 14:50:24 -0500 Rob Herring wrote:
> > Convert the img-mdc-dma binding from txt to YAML schema.
> > No functional changes except dropping the consumer node
> > (spi@...00f00) from the example, which belongs to the
> > consumer binding instead.
> >
> > Tested with 'make dt_binding_check'.
>
> No need to say that in the commit msg. It is assumed you did this.
>
> >
> > Signed-off-by: Nino Zhang <ninozhang001@...il.com>
> > ---
> > .../devicetree/bindings/dma/img-mdc-dma.txt | 57 -----------
> > .../devicetree/bindings/dma/img-mdc-dma.yaml | 98 +++++++++++++++++++
> > 2 files changed, 98 insertions(+), 57 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/dma/img-mdc-dma.txt
> > create mode 100644 Documentation/devicetree/bindings/dma/img-mdc-dma.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/dma/img-mdc-dma.txt b/Documentation/devicetree/bindings/dma/img-mdc-dma.txt
> > deleted file mode 100644
> > index 28c1341db346..000000000000
> > --- a/Documentation/devicetree/bindings/dma/img-mdc-dma.txt
> > +++ /dev/null
> > @@ -1,57 +0,0 @@
> > -* IMG Multi-threaded DMA Controller (MDC)
> > -
> > -Required properties:
> > -- compatible: Must be "img,pistachio-mdc-dma".
> > -- reg: Must contain the base address and length of the MDC registers.
> > -- interrupts: Must contain all the per-channel DMA interrupts.
> > -- clocks: Must contain an entry for each entry in clock-names.
> > - See ../clock/clock-bindings.txt for details.
> > -- clock-names: Must include the following entries:
> > - - sys: MDC system interface clock.
> > -- img,cr-periph: Must contain a phandle to the peripheral control syscon
> > - node which contains the DMA request to channel mapping registers.
> > -- img,max-burst-multiplier: Must be the maximum supported burst size multiplier.
> > - The maximum burst size is this value multiplied by the hardware-reported bus
> > - width.
> > -- #dma-cells: Must be 3:
> > - - The first cell is the peripheral's DMA request line.
> > - - The second cell is a bitmap specifying to which channels the DMA request
> > - line may be mapped (i.e. bit N set indicates channel N is usable).
> > - - The third cell is the thread ID to be used by the channel.
> > -
> > -Optional properties:
> > -- dma-channels: Number of supported DMA channels, up to 32. If not specified
> > - the number reported by the hardware is used.
> > -
> > -Example:
> > -
> > -mdc: dma-controller@...43000 {
> > - compatible = "img,pistachio-mdc-dma";
> > - reg = <0x18143000 0x1000>;
> > - interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 28 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 29 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 30 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 31 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 32 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 33 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 34 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 35 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 36 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 37 IRQ_TYPE_LEVEL_HIGH>,
> > - <GIC_SHARED 38 IRQ_TYPE_LEVEL_HIGH>;
> > - clocks = <&system_clk>;
> > - clock-names = "sys";
> > -
> > - img,max-burst-multiplier = <16>;
> > - img,cr-periph = <&cr_periph>;
> > -
> > - #dma-cells = <3>;
> > -};
> > -
> > -spi@...00f00 {
> > - ...
> > - dmas = <&mdc 9 0xffffffff 0>, <&mdc 10 0xffffffff 0>;
> > - dma-names = "tx", "rx";
> > - ...
> > -};
> > diff --git a/Documentation/devicetree/bindings/dma/img-mdc-dma.yaml b/Documentation/devicetree/bindings/dma/img-mdc-dma.yaml
> > new file mode 100644
> > index 000000000000..b635125d7ae3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/img-mdc-dma.yaml
>
> Use the compatible string for the filename.
>
> > @@ -0,0 +1,98 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/dma/img-mdc-dma.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: IMG Multi-threaded DMA Controller (MDC)
> > +
> > +maintainers:
> > + - Vinod Koul <vkoul@...nel.org>
>
> No, must be someone with this h/w and cares about this h/w.
>
> > +
> > +allOf:
> > + - $ref: /schemas/dma/dma-controller.yaml#
> > +
> > +properties:
> > + compatible:
> > + description: Must be "img,pistachio-mdc-dma".
>
> Drop. The schema says that. Same goes for all the other descriptions, so
> I won't repeat it everywhere.
>
> > + const: img,pistachio-mdc-dma
> > +
> > + reg:
> > + description:
> > + Must contain the base address and length of the MDC registers.
>
> Drop.
>
> > + minItems: 1
>
> maxItems instead.
>
> > +
> > + interrupts:
> > + description:
> > + Must contain all the per-channel DMA interrupts.
>
> Must define how many.
>
> > +
> > + clocks:
> > + description: |
> > + Must contain an entry for each entry in clock-names.
> > + See clock/clock.yaml for details.
>
> Drop.
>
> Must define how many clocks and what they are.
>
> > +
> > + clock-names:
> > + description: |
> > + Must include the following entries:
> > + - sys: MDC system interface clock.
>
> Drop. The schema says that.
>
> > + minItems: 1
> > + contains: { const: sys }
>
> Must be exact list of values, not 'sys' plus anything else you want.
>
> > +
> > + img,cr-periph:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: |
>
> Drop '|'. Not needed if no formatting to maintain.
>
>
> > + Must contain a phandle to the peripheral control syscon node
> > + which contains the DMA request to channel mapping registers.
> > +
> > + img,max-burst-multiplier:
> > + description: |
> > + Must be the maximum supported burst size multiplier.
> > + The maximum burst size is this value multiplied by the
> > + hardware-reported bus width.
>
> Wrap lines at 80 and drop '|'.
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> constraints?
>
> > +
> > + "#dma-cells":
> > + description: |
> > + Must be 3:
> > + - The first cell is the peripheral's DMA request line.
> > + - The second cell is a bitmap specifying to which channels the DMA request
> > + line may be mapped (i.e. bit N set indicates channel N is usable).
> > + - The third cell is the thread ID to be used by the channel.
> > + const: 3
> > +
> > + dma-channels:
> > + description: |
> > + Number of supported DMA channels, up to 32. If not specified
> > + the number reported by the hardware is used.
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> Drop. Already has a type defined.
>
> > + maximum: 32
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - clock-names
> > + - "img,cr-periph"
> > + - "img,max-burst-multiplier"
>
> Don't need quotes.
>
> > + - "#dma-cells"
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/mips-gic.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + mdc: dma-controller@...43000 {
>
> Drop 'mdc'
>
> > + compatible = "img,pistachio-mdc-dma";
> > + reg = <0x18143000 0x1000>;
> > + interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>,
> > + <GIC_SHARED 28 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&system_clk>;
> > + clock-names = "sys";
> > +
> > + img,max-burst-multiplier = <16>;
> > + img,cr-periph = <&cr_periph>;
> > +
> > + #dma-cells = <3>;
> > + };
> > --
> > 2.43.0
> >
Hi Rob, Krzysztof,
Thanks for your detailed reviews and guidance. As requested, I went back to the previous posting and responded to each comment below. This summarizes what was fixed in v2 and what I will address in v3.
---
> > Tested with 'make dt_binding_check'.
>
> No need to say that in the commit msg. It is assumed you did this.
Fixed in v2. Dropped from commit message.
---
> > +++ b/Documentation/devicetree/bindings/dma/img-mdc-dma.yaml
>
> Use the compatible string for the filename.
Fixed in v2.
---
> > +maintainers:
> > + - Vinod Koul <vkoul@...nel.org>
>
> No, must be someone with this h/w and cares about this h/w.
Fixed in v2. Changed to Rahul Bedarkar (Pistachio DT maintainer) + linux-mips list.
Andrew Bresticker (original author) seems inactive. I updated to Rahul Bedarkar, who maintains the Pistachio/CI40 DT (which uses this controller), and added linux-mips list so the platform community is notified.
---
> > + compatible:
> > + description: Must be "img,pistachio-mdc-dma".
>
> Drop. The schema says that. Same goes for all the other descriptions, so
> I won't repeat it everywhere.
Fixed in v2. Dropped redundant descriptions.
---
> > + reg:
> > + description:
> > + Must contain the base address and length of the MDC registers.
>
> Drop.
Fixed in v2. Dropped redundant description.
---
> > + minItems: 1
> >
> maxItems instead.
Fixed in v2. Changed to `maxItems: 1`.
---
> > + interrupts:
> > + description:
> > + Must contain all the per-channel DMA interrupts.
> >
> Must define how many.
Fixed in v2. Defined `minItems: 1`, `maxItems: 32`.
This matches the hardware max channels (DMA supports up to 32).
The original txt said "all per-channel interrupts", so schema now enforces that.
---
> > + clocks:
> > + description: |
> > + Must contain an entry for each entry in clock-names.
> > + See clock/clock.yaml for details.
>
> Drop.
>
> Must define how many clocks and what they are.
Fixed in v2. Set `clocks: maxItems: 1`.
---
> > + clock-names:
> > + description: |
> > + Must include the following entries:
> > + - sys: MDC system interface clock.
>
> Drop. The schema says that.
Fixed in v2. Dropped description.
---
> > + minItems: 1
> > + contains: { const: sys }
>
> Must be exact list of values, not 'sys' plus anything else you want.
Fixed in v2. Restricted to `sys`.
---
> > + img,cr-periph:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: |
>
> Drop '|'. Not needed if no formatting to maintain.
Ack. I mistakenly switched to `>` in v2. Will drop the indicator in v3.
---
> > + img,max-burst-multiplier:
> > + description: |
> > + Must be the maximum supported burst size multiplier.
> > + The maximum burst size is this value multiplied by the
> > + hardware-reported bus width.
>
> Wrap lines at 80 and drop '|'.
Ack. Will fix in v3.
---
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> constraints?
Fixed in v2. Kept as uint32 with `minimum: 1` to exclude invalid 0. Actual maximum not confirmed in available docs.
Example uses 16; I will add `maximum` once platform maintainers confirm.
---
> > + dma-channels:
> > + description: |
> > + Number of supported DMA channels, up to 32. If not specified
> > + the number reported by the hardware is used.
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> Drop. Already has a type defined.
Ack. Will fix in v3.
---
> > +required:
> > + - "img,cr-periph"
> > + - "img,max-burst-multiplier"
>
> Don't need quotes.
Fixed in v2. Removed quotes.
---
> > +examples:
> > + mdc: dma-controller@...43000 {
>
> Drop 'mdc'
Fixed in v2. Dropped node label.
---
Please let me know if there are any further concerns. Otherwise, I will send [PATCH v3] shortly as a new thread with the remaining fixes.
Thanks,
Nino
Powered by blists - more mailing lists