On 23-04-19, 20:30, Gustavo Pimentel wrote:
> Add Synopsys PCIe Endpoint eDMA IP core driver to kernel.
Still an RFC ?
> +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)
> + if (unlikely(!chunk))
> + return NULL;
> +
> + INIT_LIST_HEAD(&chunk->list);
> + chunk->chan = chan;
> + chunk->cb = !(desc->chunks_alloc % 2);
? why %2?
> +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?
> +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
> +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
> + 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?
> +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..?
> +struct dw_edma_transfer {
> + struct dma_chan *dchan;
> + union Xfer {
no camel case please
It would help to run checkpatch with --strict option to find any style
issues and fix them as well
--
~Vinod
