Hi Vinod,
On Mon, May 6, 2019 at 12:20:1, Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> On 23-04-19, 20:30, Gustavo Pimentel wrote:
> > Add Synopsys PCIe Endpoint eDMA IP core driver to kernel.
>
> Still an RFC ?
Yes, it was RFC til I get a formal validation from the HW team. Now that
I got it, I can formally submit the very first patch version.
>
> > +static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc)
> > +{
> > + struct dw_edma_chan *chan = desc->chan;
> > + struct dw_edma *dw = chan->chip->dw;
> > + struct dw_edma_chunk *chunk;
> > +
> > + chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);
>
> Looking at the code this should be called from one of the
> device_prep_xxx calls so this should not sleep, so GFP_NOWAIT please
>
> (pls audit rest of the mem allocations in the code)
Ok. Fixed on dw_edma_alloc_burst(), dw_edma_alloc_chunk() and
dw_edma_alloc_desc().
The other memory allocations are on probe() and dw_edma_channel_setup()
that is called by probe(), that doesn't require to be GFP_NOWAIT.
>
> > + if (unlikely(!chunk))
> > + return NULL;
> > +
> > + INIT_LIST_HEAD(&chunk->list);
> > + chunk->chan = chan;
> > + chunk->cb = !(desc->chunks_alloc % 2);
> ? why %2?
I think it's explained on the patch description. CB also is known as
Change Bit that must be toggled in order to the HW assume a new linked
list is available to be consumed.
Since desc->chunks_alloc variable is an incremental counter the remainder
after division by 2 will be zero (if chunks_alloc is even) or one (if
chunks_alloc is odd).
>
> > +static enum dma_status
> > +dw_edma_device_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > + struct dw_edma_desc *desc;
> > + struct virt_dma_desc *vd;
> > + unsigned long flags;
> > + enum dma_status ret;
> > + u32 residue = 0;
> > +
> > + ret = dma_cookie_status(dchan, cookie, txstate);
> > + if (ret == DMA_COMPLETE)
> > + return ret;
> > +
> > + if (ret == DMA_IN_PROGRESS && chan->status == EDMA_ST_PAUSE)
> > + ret = DMA_PAUSED;
>
> Don't you want to set residue on paused channel, how else will user know
> the position of pause?
I didn't catch you on this. I'm only setting the dma status here. After
this function, the residue is calculated and set, isn't it?
>
> > +static struct dma_async_tx_descriptor *
> > +dw_edma_device_transfer(struct dw_edma_transfer *xfer)
> > +{
> > + struct dw_edma_chan *chan = dchan2dw_edma_chan(xfer->dchan);
> > + enum dma_transfer_direction direction = xfer->direction;
> > + phys_addr_t src_addr, dst_addr;
> > + struct scatterlist *sg = NULL;
> > + struct dw_edma_chunk *chunk;
> > + struct dw_edma_burst *burst;
> > + struct dw_edma_desc *desc;
> > + u32 cnt;
> > + int i;
> > +
> > + if ((direction == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_WRITE) ||
> > + (direction == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ))
> > + return NULL;
> > +
> > + if (xfer->cyclic) {
> > + if (!xfer->xfer.cyclic.len || !xfer->xfer.cyclic.cnt)
> > + return NULL;
> > + } else {
> > + if (xfer->xfer.sg.len < 1)
> > + return NULL;
> > + }
> > +
> > + if (!chan->configured)
> > + return NULL;
> > +
> > + desc = dw_edma_alloc_desc(chan);
> > + if (unlikely(!desc))
> > + goto err_alloc;
> > +
> > + chunk = dw_edma_alloc_chunk(desc);
> > + if (unlikely(!chunk))
> > + goto err_alloc;
> > +
> > + src_addr = chan->config.src_addr;
> > + dst_addr = chan->config.dst_addr;
> > +
> > + if (xfer->cyclic) {
> > + cnt = xfer->xfer.cyclic.cnt;
> > + } else {
> > + cnt = xfer->xfer.sg.len;
> > + sg = xfer->xfer.sg.sgl;
> > + }
> > +
> > + for (i = 0; i < cnt; i++) {
> > + if (!xfer->cyclic && !sg)
> > + break;
> > +
> > + if (chunk->bursts_alloc == chan->ll_max) {
> > + chunk = dw_edma_alloc_chunk(desc);
> > + if (unlikely(!chunk))
> > + goto err_alloc;
> > + }
> > +
> > + burst = dw_edma_alloc_burst(chunk);
> > + if (unlikely(!burst))
> > + goto err_alloc;
> > +
> > + if (xfer->cyclic)
> > + burst->sz = xfer->xfer.cyclic.len;
> > + else
> > + burst->sz = sg_dma_len(sg);
> > +
> > + chunk->ll_region.sz += burst->sz;
> > + desc->alloc_sz += burst->sz;
> > +
> > + if (direction == DMA_DEV_TO_MEM) {
> > + burst->sar = src_addr;
>
> We are device to mem, so src is peripheral.. okay
>
> > + if (xfer->cyclic) {
> > + burst->dar = xfer->xfer.cyclic.paddr;
> > + } else {
> > + burst->dar = sg_dma_address(sg);
> > + src_addr += sg_dma_len(sg);
>
> and we increment the src, doesn't make sense to me!
>
> > + }
> > + } else {
> > + burst->dar = dst_addr;
> > + if (xfer->cyclic) {
> > + burst->sar = xfer->xfer.cyclic.paddr;
> > + } else {
> > + burst->sar = sg_dma_address(sg);
> > + dst_addr += sg_dma_len(sg);
>
> same here as well
This is hard to explain in words...
Well, in my perspective I want to transfer a piece of memory from the
peripheral into local RAM
Through the DMA client API I'll break this piece of memory in several
small parts and add all into a list (scatter-gather), right?
Each element of the scatter-gather has the sg_dma_address (in the
DMA_DEV_TO_MEM case will be the destination address) and the
corresponding size.
However, I still need the other address (in the DMA_DEV_TO_MEM case will
be the source address) for that small part of memory.
Since I get that address from the config, I still need to increment the
source address in the same proportion of the destination address, in
other words, the increment will be the part size.
If there is some way to set and get the address for the source (in this
case) into each scatter-gather element, that would be much nicer, is that
possible?
>
> > +static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> > +{
> > + struct dw_edma_desc *desc;
> > + struct virt_dma_desc *vd;
> > + unsigned long flags;
> > +
> > + dw_edma_v0_core_clear_done_int(chan);
> > +
> > + spin_lock_irqsave(&chan->vc.lock, flags);
> > + vd = vchan_next_desc(&chan->vc);
> > + if (vd) {
> > + switch (chan->request) {
> > + case EDMA_REQ_NONE:
> > + desc = vd2dw_edma_desc(vd);
> > + if (desc->chunks_alloc) {
> > + chan->status = EDMA_ST_BUSY;
> > + dw_edma_start_transfer(chan);
> > + } else {
> > + list_del(&vd->node);
> > + vchan_cookie_complete(vd);
> > + chan->status = EDMA_ST_IDLE;
> > + }
> > + break;
>
> Empty line after each break please
Ok. Done.
>
> > + case EDMA_REQ_STOP:
> > + list_del(&vd->node);
> > + vchan_cookie_complete(vd);
> > + chan->request = EDMA_REQ_NONE;
> > + chan->status = EDMA_ST_IDLE;
>
> Why do we need to track request as well as status?
Since I don't actually have the PAUSE state feature available on HW, I'm
emulating it through software. As far as HW is concerned, it thinks that
it has transferred everything (no more bursts valid available), but in
terms of software, we still have a lot of chunks (each one containing
several bursts) to process.
>
> > +static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
> > +{
> > + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > +
> > + if (chan->status != EDMA_ST_IDLE)
> > + return -EBUSY;
> > +
> > + dma_cookie_init(dchan);
>
> not using vchan_init() you need to do this and init the lists..?
That's right, vchan_init() already does that.
>
> > +struct dw_edma_transfer {
> > + struct dma_chan *dchan;
> > + union Xfer {
>
> no camel case please
Ok.
>
> It would help to run checkpatch with --strict option to find any style
> issues and fix them as well
I usually run with that option, but for now, that option is giving some
warnings about macro variable names that are pure noise.
> --
> ~Vinod
Regards,
Gustavo
