Re: [PATCH v5 2/2] Add Intel LGM soc DMA support.

[Reddy, MallikarjunaX]

Hi Peter,
Thanks for the review, please see my comment inline..

On 9/4/2020 2:31 PM, Peter Ujfalusi wrote:
> On 31/08/2020 11.07, Reddy, MallikarjunaX wrote:
> > On 8/28/2020 7:17 PM, Peter Ujfalusi wrote:
> > > Hi,
> > >
> > > On 27/08/2020 17.41, Reddy, MallikarjunaX wrote:
> > > > > > > > +
> > > > > > > > +    dma_dev->device_alloc_chan_resources =
> > > > > > > > +        d->inst->ops->device_alloc_chan_resources;
> > > > > > > > +    dma_dev->device_free_chan_resources =
> > > > > > > > +        d->inst->ops->device_free_chan_resources;
> > > > > > > > +    dma_dev->device_terminate_all =
> > > > > > > > d->inst->ops->device_terminate_all;
> > > > > > > > +    dma_dev->device_issue_pending =
> > > > > > > > d->inst->ops->device_issue_pending;
> > > > > > > > +    dma_dev->device_tx_status = d->inst->ops->device_tx_status;
> > > > > > > > +    dma_dev->device_resume = d->inst->ops->device_resume;
> > > > > > > > +    dma_dev->device_pause = d->inst->ops->device_pause;
> > > > > > > > +    dma_dev->device_config = d->inst->ops->device_config;
> > > > > > > > +    dma_dev->device_prep_slave_sg =
> > > > > > > > d->inst->ops->device_prep_slave_sg;
> > > > > > > > +    dma_dev->device_synchronize = d->inst->ops->device_synchronize;
> > > > > > > > +
> > > > > > > > +    if (d->ver == DMA_VER22) {
> > > > > > > > +        dma_dev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> > > > > > > > +        dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> > > > > > > > +        dma_dev->directions = BIT(DMA_MEM_TO_DEV) |
> > > > > > > > +                      BIT(DMA_DEV_TO_MEM);
> > > > > > > > +        dma_dev->residue_granularity =
> > > > > > > > +                    DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> > > > > > > > +    }
> > > > > > > So, if version is != DMA_VER22, then you don't support any direction?
> > > > > > > Why register the DMA device if it can not do any transfer?
> > > > > > Only dma0 instance (intel,lgm-cdma) is used as a general purpose slave
> > > > > > DMA. we set both control and datapath here.
> > > > > > Other instances we set only control path. data path is taken care
> > > > > > by dma
> > > > > > client(GSWIP).
> > > > > How the client (GSWIP) can request a channel from intel,lgm-* ? Don't
> > > > > you need some capabilities for the DMA device so core can sort out the
> > > > > request?
> > > > client request channel by name, dma_request_slave_channel(dev, name);
> > > clients should use dma_request_chan(dev, name);
> > >
> > > If the channel can be requested via DT or ACPI then we don't check the
> > > capabilities at all, so yes, that could work.
> > >
> > > > > > Only thing needs to do is get the channel, set the descriptor and just
> > > > > > on the channel.
> > > > > How do you 'on' the channel?
> > > > we on the channel in issue_pending.
> > > Right.
> > > Basically you only prep_slave_sg/single for the DMA_VER22? Or do you
> > > that for the others w/o direction support?
> > Yes. prep_slave_sg/single only for the DMA_VER22.
> So, you place the transfers to DMA_VER22's channel
>
> > > For the intel,lgm-* DMAs you only call issue_pending() and probably
> > > terminate_all?
> > Yes, correct.
> and issue_pending on a channel which does not have anything pending?
> How client knows which of the 'only need to be ON' channel to enable
> when it placed a transfer to another channel?
>
> How would the client driver (and the IP) would be integrated with
> different system DMA which have standard channel management?
>
> If DMA_VER22 knows which intel,lgm-* it should place the transfer then
> with virt-dma you can handle that just fine?
In the version point of view, "intel,lgm-cdma" dma instance support 
DMA_VER22. and other "intel,lgm-*" dma instances support  > DMA_VER22 .

The channels registered to kernel from "intel,lgm-cdma" dma instance 
used as a general purpose dma. The clients are SPI, ebu_nand etc..
It uses standard dmaengine calls dma_request_chan(), 
dmaengine_slave_config(), dmaengine_prep_slave_sg() & 
dma_async_issue_pending()..

and other dma instances the clients are GSWIP & CQM (Central Queue 
manager) which uses the DMA in their packet processing.
Each dma dma2tx, dma1rx, dma1tx, dma0tx and dma3 had channels 16, 8, 
16,16 and 16 respectively and these channels are used by GSWIP & CQM as 
part of
ingress(IGP) and egress(EGP) ports. and each of the dma port uses a dma 
channel. This is one to one mapping.

The GSWIP & CQM talk to the dma controller driver via dmaengine to get 
the dma channels using dma_request_slave_channel().
Configures the descriptor base address using intel_dma_chan_desc_cfg() 
which is EXPORT API from dma driver(this desc base address is the 
register address(descriptor) of the IGP/EGP), and ON the channel using 
dma_async_issue_pending.

GSWIP & CQM is highly low level register configurable/programmable take 
care about the the packet processing through the register configurations.
> Do you have public documentation for the DMA?
Unfortunately we dont have public documentation for this DMA IP.
>   It sounds a bit like TI's
> EDMA which is in essence is a two part  setup:
> CC: Channel Controller - to submit transfers (like your DMA_VER22)
> TC: Transfer Controller - it executes the transfers submitted by the CC
>
> One CC can be backed by multiple TCs. I don't have direct control over
> the TC (can not start/stop), it is controlled by the CC.
>
> Documentation/devicetree/bindings/dma/ti-edma.txt
>
> Or is it a different setup?
This is different setup. Explain above. Transfer control is directly 
controlled by client.
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki