Re: [PATCH v9 2/2] Add Intel LGM SoC DMA support.

["Reddy, MallikarjunaX" <mallikarjunax.reddy@xxxxxxxxxxxxxxx>]

Hi Vinod,

Thanks for your valuable comments. My reply inline.

On 11/26/2020 12:50 PM, Vinod Koul wrote:
> On 25-11-20, 18:39, Reddy, MallikarjunaX wrote:
>
> > > > > > desc needs to be configure for each dma channel and the remapped address of
> > > > > > the IGP & EGP is desc base adress.
> > > > > Why should this address not passed as src_addr/dst_addr?
> > > > src_addr/dst_addr is the data pointer. Data pointer indicates address
> > > > pointer of data buffer.
> > > >
> > > > ldma_chan_desc_cfg() carries the descriptor address.
> > > >
> > > > The descriptor list entry contains the data pointer, which points to the
> > > > data section in the memory.
> > > >
> > > > So we should not use src_addr/dst_addr as desc base address.
> > > Okay sounds reasonable. why is this using in API here?
> > descriptor base address needs to be write into the dma register (DMA_CDBA).
> Why cant descriptor be allocated by damenegine driver, passed to client
> as we normally do in prep_* callbacks ? Why do you need a custom API
1)
client needs to set the descriptor base address and also number of 
descriptors used in the descriptor list.
reg DMA_CDBA used to configure descriptor base address and reg DMA_CDLEN 
used to configure number of descriptors used in the descriptor list.

In case of (ver > DMA_VER22) all descriptor fields and data pointer will 
be set by client, so we just need to write desc base and num desc length 
in to corresponding registers from the driver side.

dma_async_tx_descriptor * data is not really needed from driver to 
client side , so i am not planned to return 'struct 
dma_async_tx_descriptor *'.

because of this reason i used custom API (return -Ve for error and ZERO 
for success) instead of standard dmaengine_prep_slave_sg() callback 
(return 'struct dma_async_tx_descriptor *' descriptor)

2)
We can also use the dmaengine_prep_slave_sg( ) to pass desc base addr & 
desc number from client.
In that case we have to use (sg)->dma_address as desc base address and 
(sg)->length as desc length.

dmaengine prep_* callback return 'struct dma_async_tx_descriptor *, this 
can be used on client side as to check  prep_* callback SUCCESS/FAIL.

Example:
/* code snippet */

static struct dma_async_tx_descriptor *
ldma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
                  unsigned int sglen, enum dma_transfer_direction dir,
                  unsigned long flags, void *context)
{

.....

    if (d->ver > DMA_VER22)
        return ldma_chan_desc_cfg(chan, sgl->dma_address, sglen);

.....

}

static struct dma_async_tx_descriptor *
ldma_chan_desc_cfg(struct dma_chan *chan, dma_addr_t desc_base, int 
desc_num)
{
        struct ldma_chan *c = to_ldma_chan(chan);
        struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
        struct dma_async_tx_descriptor *tx;
        struct dw2_desc_sw *ds;

        if (!desc_num) {
                dev_err(d->dev, "Channel %d must allocate descriptor 
first\n",
                        c->nr);
                return NULL;
        }

        if (desc_num > DMA_MAX_DESC_NUM) {
                dev_err(d->dev, "Channel %d descriptor number out of 
range %d\n",
                        c->nr, desc_num);
                return NULL;
        }

        ldma_chan_desc_hw_cfg(c, desc_base, desc_num);
        c->flags |= DMA_HW_DESC;
        c->desc_cnt = desc_num;
        c->desc_phys = desc_base;

        ds = kzalloc(sizeof(*ds), GFP_NOWAIT);
        if (!ds)
                return NULL;

        tx = &ds->vdesc.tx;
        dma_async_tx_descriptor_init(tx, chan);

        return tx;
}
Please let me know if this is OK, So that i will include in the next patch.
>