Re: [PATCH v2 1/8] dt-bindings: dma: marvell,mmp-dma: Add SpacemiT K1 PDMA support

[Guodong Xu <guodong@riscstar.com>]

Hi, Krzysztof

On Tue, Jul 1, 2025 at 3:35 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 01/07/2025 07:36, Guodong Xu wrote:
> > Add "spacemit,k1-pdma" compatible string to support SpacemiT K1 PDMA
> > controller. This variant requires:
>
> Why is this marvell? This should be explained here, it's really unexpected.
>

SpacemiT K1 SoC uses the same DMA controller as Marvell MMP. They share most
of the registers (and address offsets) and only enhanced in addressing space
capability (from 32bit to 64bit).

Also, spacemit,k1-pdma and marvell,pdma-1.0 use the same driver (mmp_pdma.c),
that's the reason why I chose keeping them in the same binding file.


> > - clocks: Clock controller for the DMA
> > - resets: Reset controller for the DMA
> >
> > Also add explicit #dma-cells property definition with proper constraints:
> > - 2 cells for marvell,pdma-1.0 and spacemit,k1-pdma
> >     - (request number + unused)
> > - 1 cell for other variants
> >     - (request number only)
> >
> > This fixes "make dtbs_check W=3" warnings about unevaluated properties.
>
> How can I reproduce these warnings? Looks like this is not relevant
> here. Each patch is applied one after another and commit msg must be
> correct at this point.
>

You're absolutely right about the commit message needing to be accurate
at this point. The dtbs_check warnings only occur when compiling board DTS
files that contain nodes with marvell,pdma-1.0 or spacemit,k1-pdma
compatible strings - specifically when PATCH 7 of this series.

I'll revise the commit message to better reflect that this patch enhances
things, rather than claiming it "fixes" warnings at this stage.

>
> >
> > Signed-off-by: Guodong Xu <guodong@riscstar.com>
> > ---
> > v2:
> > - Used more specific compatible string "spacemit,k1-pdma"
> > - Enhanced DT bindings with conditional constraints:
> >   - clocks/resets properties only required for SpacemiT K1
> >   - #dma-cells set to 2 for marvell,pdma-1.0 and spacemit,k1-pdma
> >   - #dma-cells set to 1 for other variants, ie.
> >       marvell,adma-1.0 and  marvell,pxa910-squ
> > ---
> >  .../devicetree/bindings/dma/marvell,mmp-dma.yaml   | 49 ++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/marvell,mmp-dma.yaml b/Documentation/devicetree/bindings/dma/marvell,mmp-dma.yaml
> > index d447d5207be0436bc7fb648dffe31f8b780b491d..7b5f7ccfc9dbb69bfef250146cba5434548f3702 100644
> > --- a/Documentation/devicetree/bindings/dma/marvell,mmp-dma.yaml
> > +++ b/Documentation/devicetree/bindings/dma/marvell,mmp-dma.yaml
> > @@ -18,6 +18,7 @@ properties:
> >        - marvell,pdma-1.0
> >        - marvell,adma-1.0
> >        - marvell,pxa910-squ
> > +      - spacemit,k1-pdma
> >
> >    reg:
> >      maxItems: 1
> > @@ -32,6 +33,19 @@ properties:
> >        A phandle to the SRAM pool
> >      $ref: /schemas/types.yaml#/definitions/phandle
> >
> > +  clocks:
> > +    description: Clock for the controller
> > +    maxItems: 1
> > +
> > +  resets:
> > +    description: Reset controller for the DMA controller
> > +    maxItems: 1
> > +
> > +  '#dma-cells':
> > +    description:
> > +      DMA specifier, consisting of a phandle to DMA controller plus the
> > +      following integer cells
>
> Why do you need it here? Isn't this coming from dma common schemas?

Thanks for pointing out this. It should be removed.
I will fix it.

> > +
> >    '#dma-channels':
> >      deprecated: true
> >
> > @@ -52,12 +66,47 @@ allOf:
> >            contains:
> >              enum:
> >                - marvell,pdma-1.0
> > +              - spacemit,k1-pdma
> >      then:
> >        properties:
> >          asram: false
> >      else:
> >        required:
> >          - asram
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: spacemit,k1-pdma
> > +    then:
> > +      required:
> > +        - clocks
> > +        - resets
> > +    else:
> > +      properties:
> > +        clocks: false
> > +        resets: false
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - marvell,pdma-1.0
> > +              - spacemit,k1-pdma
> > +    then:
> > +      properties:
> > +        '#dma-cells':
> > +          const: 2
> > +          description:
> > +            The first cell contains the DMA request number for the peripheral
> > +            device. The second cell is currently unused but must be present for
> > +            backward compatibility.
> > +    else:
> > +      properties:
> > +        '#dma-cells':
> > +          const: 1
> > +          description:
> > +            The cell contains the DMA request number for the peripheral device.
>
>
> It's getting complicated. I suggest to make your own schema. Then you
> would also switch to preferred 'sram' property instead of that legacy
> 'asram'.
>
> Really, ancient schemas should not be grown for new, completely

The reason that they share the same device driver may not be strong enough
compared to what you said here.

> different platforms.
>

Complexity wise, I agree with you. I should admit that I haven't dug into
the history enough. Anyway, unless other people have strong opinion, I will
create a new schema namely spacemit,k1-pdma.yaml

Thanks Krzysztof.

BR,
Guodong


>
> Best regards,
> Krzysztof