On Tue, May 7, 2019 at 6:3:10, Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> On 06-05-19, 16:42, Gustavo Pimentel wrote:
>
> > > > + 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).
>
> Okay it would be great to add a comment here to explain as well
Ok, I'll add it.
>
> > > > +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?
>
> Hmm I thought you returned for paused case, if not then it is okay
No, I'm just setting the dma status in pause case, then I calculate the
residue.
>
> > > > +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
>
> Right and most of the case RAM address (sg) needs to increment whereas
> peripheral is a constant one
>
> > 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.
>
> Correct
>
> > 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.
>
> I don't think so. Typically the device address is a FIFO, which does not
> increment and you keep pushing data at same address. It is not a memory
In my use case, it's a memory, perhaps that is what is causing this
confusing.
I'm copying "plain and flat" data from point A to B, with the
particularity that the peripheral memory is always continuous and the CPU
memory can be constituted by scatter-gather chunks of contiguous memory
>
> > 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?
>
> > > > + 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.
>
> Why do you need to emulate, if HW doesnt support so be it?
> The applications should handle a device which doesnt support pause and
> not a low level driver
In this case, since I've to refill the eDMA memory and retrigger the HW
block each time the transfer is completed, it's easy to emulate a pause
state, by holding or not refilling the eDMA memory.
I thought that this could be a nice and easy feature to have.
>
> > > > +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.
>
> yeah that is a *guide* and to be used as guidance. If code looks worse
> off then it shouldn't be used. But many of the test are helpful. Some
> macros checks actually make sense, but again use your judgement :)
Sure.
>
> --
> ~Vinod
Regards,
Gustavo
