Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel LGM SOC

[Reddy, MallikarjunaX]

Hi Thomas,

On 11/11/2020 1:39 AM, Thomas Langer wrote:
> > -----Original Message-----
> > From: Reddy, MallikarjunaX <mallikarjunax.reddy@xxxxxxxxxxxxxxx>
> > Sent: Montag, 2. November 2020 15:42
> > To: Thomas Langer <tlanger@xxxxxxxxxxxxx>; dmaengine@xxxxxxxxxxxxxxx;
> > vkoul@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; Shevchenko, Andriy
> > <andriy.shevchenko@xxxxxxxxx>; chuanhua.lei@xxxxxxxxxxxxxxx; Kim,
> > Cheol Yong <Cheol.Yong.Kim@xxxxxxxxx>; Wu, Qiming <qi-
> > ming.wu@xxxxxxxxx>; malliamireddy009@xxxxxxxxx; peter.ujfalusi@xxxxxx;
> > Langer, Thomas <thomas.langer@xxxxxxxxx>
> > Subject: Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel
> > LGM SOC
> >
> > This email was sent from outside of MaxLinear.
> >
> >
> > Hi Thomas,
> > Thanks for the review, my comments inline.
> >
> > On 10/28/2020 3:24 AM, Thomas Langer wrote:
> > > Hello Reddy,
> > >
> > > I think "Intel" should always be written with a capital "I" (like in
> > the Subject, but except in the binding below)
> > OK.
> > > > + compatible:
> > > > +  oneOf:
> > > > +   - const: intel,lgm-cdma
> > > > +   - const: intel,lgm-dma2tx
> > > > +   - const: intel,lgm-dma1rx
> > > > +   - const: intel,lgm-dma1tx
> > > > +   - const: intel,lgm-dma0tx
> > > > +   - const: intel,lgm-dma3
> > > > +   - const: intel,lgm-toe-dma30
> > > > +   - const: intel,lgm-toe-dma31
> > > Bindings are normally not per instance.
> > > What if next generation chip gets more DMA modules but has no other
> > changes in the HW block?
> > > What is wrong with
> > >     - const: intel,lgm-cdma
> > >     - const: intel,lgm-hdma
> > > and extra attributes to define the rx/tx restriction (or what do it
> > mean?)?
> > >   From the driver code I saw that "toe" is also just of type "hdma"
> > and no further differences in code are done.
> > We had a discussion on the same in the previous patches and Rob
> > Herring
> > said Okay using Different compatibles.
> > below the snippet.
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > +
> >   >>> + compatible:
> >   >>> +  anyOf:
> >   >>> +   - const: intel,lgm-cdma
> >   >>> +   - const: intel,lgm-dma2tx
> >   >>> +   - const: intel,lgm-dma1rx
> >   >>> +   - const: intel,lgm-dma1tx
> >   >>> +   - const: intel,lgm-dma0tx
> >   >>> +   - const: intel,lgm-dma3
> >   >>> +   - const: intel,lgm-toe-dma30
> >   >>> +   - const: intel,lgm-toe-dma31
> >   >> Please explain why you need so many different compatible strings.
> >   > This hw dma has 7 DMA instances.
> >   > Some for datapath, some for memcpy  and some for TOE.
> >   > Some for TX only, some for RX only, and some for TX/RX(memcpy and
> > ToE).
> >   >
> >   > dma TX/RX type we considered as driver specific data of each
> > instance and
> >   > used different compatible strings for each instance.
> >   > And also idea is in future if any driver specific data of any
> > particular
> >   > instance we can handle.
> >   >
> >   > Here if dma name and type(tx or rx) will be accepted as devicetree
> >   > attributes then we can move .name = "toe_dma31", & .type =
> > DMA_TYPE_MCPY
> >   > to devicetree. So that the compatible strings can be limited to
> > two.
> >   > intel,lgm-cdma & intel,lgm-hdma .
> >
> > [Rob]
> > Different compatibles are okay if the instances are different and we
> > don't have properties to describe the differences.
> Okay, but then explain what the differences are, that cannot be described
> by other properties/attributes. In the driver code I cannot see anything,
> except the "name". But for printouts in driver, "drv_dbg" or similar will
> just use the node path for the instance.
On patch4 series we had the same discussion.
i will brief it here again.

This hw dma has 7 DMA instances, and each Some for TX only, some for RX 
only, and some for TX/RX.

dma TX/RX type we considered as driver specific data and it cant be used 
as dt property as per the previous reviewers.

So i moved it to driver specific data.

If type(tx or rx) will be accepted as devicetree attributes then we can 
move it to devicetree.

So as you said we can limit compatible strings can be limited to two. 
intel,lgm-cdma & intel,lgm-hdma .

One more advantage i see with this model is in future if any driver 
specific data of any particular instance we can handle easily.
> > For some of what you have in this binding, I think it should be part
> > of
> > the consumer cells.
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > ++
> > > Best regards,
> > > Thomas