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
