lkml.org 
[lkml]   [2017]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v1 2/2] dmaengine: Add DW AXI DMAC driver
Date
Hi Vinod!

Thanks for respond.
My comments below.

On Tue, 2017-03-14 at 08:30 +0530, Vinod Koul wrote:
> On Tue, Feb 21, 2017 at 11:38:04PM +0300, Eugeniy Paltsev wrote:

> > +static void vchan_desc_put(struct virt_dma_desc *vdesc)
> > +{
> > + axi_desc_put(vd_to_axi_desc(vdesc));
> > +}
> well this has no logic and becomes a dummy fn, so why do we need it.

Both functions (vchan_desc_put and axi_desc_put) are used.
First one is put as callback to vchan, second one is used when we need
to free descriptors after errors in preparing function (before we call
vchan_tx_prep).

> > 
> > +
> > +static enum dma_status
> > +dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
> > +   struct dma_tx_state *txstate)
> > +{
> > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> > + enum dma_status ret;
> > +
> > + ret = dma_cookie_status(dchan, cookie, txstate);
> > +
> > + if (chan->is_paused && ret == DMA_IN_PROGRESS)
> > + return DMA_PAUSED;
> no residue calculation?

I don't think we need residue calculation in mem-to-mem transfers. I'm
planning to add residue calculation at once with DMA_SLAVE
functionality.

> > 
> > +static void dma_chan_free_chan_resources(struct dma_chan *dchan)
> > +{
> > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> > +
> > + /* ASSERT: channel is idle */
> > + if (axi_chan_is_hw_enable(chan))
> > + dev_err(dchan2dev(dchan), "%s is non-idle!\n",
> > + axi_chan_name(chan));
> > +
> > + axi_chan_disable(chan);
> > + axi_chan_irq_disable(chan, DWAXIDMAC_IRQ_ALL);
> > +
> > + vchan_free_chan_resources(&chan->vc);
> > +
> > + dev_vdbg(dchan2dev(dchan), "%s: %s: descriptor still
> > allocated: %u\n",
> > + __func__, axi_chan_name(chan), chan-
> > >descs_allocated);
> > +
> > + pm_runtime_put_sync_suspend(chan->chip->dev);
> any reason why sync variant is used?

I re-read the documentation and yes - I can use async variant here (and
in the other parts of this driver).

> > + block_ts = xfer_len >> src_width;
> > + if (block_ts > max_block_ts) {
> > + block_ts = max_block_ts;
> > + xfer_len = max_block_ts << src_width;
> > + }
> > +
> > + desc = axi_desc_get(chan);
> > + if (unlikely(!desc))
> > + goto err_desc_get;
> > +
> > + write_desc_sar(desc, src_adr);
> > + write_desc_dar(desc, dst_adr);
> > + desc->lli.block_ts_lo = cpu_to_le32(block_ts - 1);
> > + desc->lli.ctl_hi =
> > cpu_to_le32(CH_CTL_H_LLI_VALID);
> > +
> > + reg = (DWAXIDMAC_BURST_TRANS_LEN_4 <<
> > CH_CTL_L_DST_MSIZE_POS |
> > +        DWAXIDMAC_BURST_TRANS_LEN_4 <<
> > CH_CTL_L_SRC_MSIZE_POS |
> > +        dst_width << CH_CTL_L_DST_WIDTH_POS |
> > +        src_width << CH_CTL_L_SRC_WIDTH_POS |
> > +        DWAXIDMAC_CH_CTL_L_INC <<
> > CH_CTL_L_DST_INC_POS |
> > +        DWAXIDMAC_CH_CTL_L_INC <<
> > CH_CTL_L_SRC_INC_POS);
> > + desc->lli.ctl_lo = cpu_to_le32(reg);
> > +
> > + set_desc_src_master(desc);
> > + set_desc_dest_master(desc);
> > +
> > + /* Manage transfer list (xfer_list) */
> > + if (!first) {
> > + first = desc;
> > + } else {
> > + list_add_tail(&desc->xfer_list, &first-
> > >xfer_list);
> and since you use vchan why do you need this list

Each virtual descriptor encapsulates several hardware descriptors,
which belong to same transfer.
This list (xfer_list) is used only for allocating/freeing these
descriptors and it doesn't affect on virtual dma work logic.

-- 
 Eugeniy Paltsev
\
 
 \ /
  Last update: 2017-03-28 21:33    [W:0.065 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site