Re: [PATCH v6 2/3] dmaengine: amlogic: Add general DMA driver for A9

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

Hi Vinod,
    Thanks for your advice.

On 2026/3/17 19:13, Vinod Koul wrote:
> On 09-03-26, 06:33, Xianwei Zhao via B4 Relay wrote:
>> From: Xianwei Zhao<xianwei.zhao@amlogic.com>
>> +static dma_cookie_t aml_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>> +{
>> +     return dma_cookie_assign(tx);
>> +}
> You lost tx, why was it not saved into a queue?
> 
>> +static struct dma_async_tx_descriptor *aml_dma_prep_slave_sg
>> +             (struct dma_chan *chan, struct scatterlist *sgl,
>> +             unsigned int sg_len, enum dma_transfer_direction direction,
>> +             unsigned long flags, void *context)
>> +{
>> +     struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
>> +     struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
>> +     struct aml_dma_sg_link *sg_link;
>> +     struct scatterlist *sg;
>> +     int idx = 0;
>> +     u64 paddr;
>> +     u32 reg, link_count, avail, chan_id;
>> +     u32 i;
>> +
>> +     if (aml_chan->direction != direction) {
>> +             dev_err(aml_dma->dma_device.dev, "direction not support\n");
>> +             return NULL;
>> +     }
>> +
>> +     switch (aml_chan->status) {
>> +     case DMA_IN_PROGRESS:
>> +             dev_err(aml_dma->dma_device.dev, "not support multi tx_desciptor\n");
>> +             return NULL;
> And why is that. You are preparing a descriptor and keep it ready and
> submit after the current one finishes
> 
> 
>> +
>> +     case DMA_COMPLETE:
>> +             aml_chan->data_len = 0;
>> +             chan_id = aml_chan->chan_id;
>> +             reg = (direction == DMA_DEV_TO_MEM) ? WCH_INT_MASK : RCH_INT_MASK;
>> +             regmap_set_bits(aml_dma->regmap, reg, BIT(chan_id));
>> +
>> +             break;
>> +     default:
>> +             dev_err(aml_dma->dma_device.dev, "status error\n");
>> +             return NULL;
>> +     }
>> +
>> +     link_count = sg_nents_for_dma(sgl, sg_len, SG_MAX_LEN);
>> +
>> +     if (link_count > DMA_MAX_LINK) {
>> +             dev_err(aml_dma->dma_device.dev,
>> +                     "maximum number of sg exceeded: %d > %d\n",
>> +                     sg_len, DMA_MAX_LINK);
>> +             aml_chan->status = DMA_ERROR;
>> +             return NULL;
>> +     }
>> +
>> +     aml_chan->status = DMA_IN_PROGRESS;
>> +
>> +     for_each_sg(sgl, sg, sg_len, i) {
>> +             avail = sg_dma_len(sg);
>> +             paddr = sg->dma_address;
>> +             while (avail > SG_MAX_LEN) {
>> +                     sg_link = &aml_chan->sg_link[idx++];
>> +                     /* set dma address and len  to sglink*/
>> +                     sg_link->address = paddr;
>> +                     sg_link->ctl = FIELD_PREP(LINK_LEN, SG_MAX_LEN);
>> +                     paddr = paddr + SG_MAX_LEN;
>> +                     avail = avail - SG_MAX_LEN;
>> +             }
>> +             sg_link = &aml_chan->sg_link[idx++];
>> +             /* set dma address and len  to sglink*/
>> +             sg_link->address = paddr;
>> +             sg_link->ctl = FIELD_PREP(LINK_LEN, avail);
>> +
>> +             aml_chan->data_len += sg_dma_len(sg);
>> +     }
>> +     aml_chan->sg_link_cnt = idx;
> There is no descriptor management here. You are directly writing to
> channel. This is_very_  inefficient and defeats the use of dmaengine.
> 
> Please revise the driver. Implement queues to manage multiple txns and
> we have vchan to help you implement these, so take use of that
> 

I will take use vchan to support mltiple txns.

> --
> ~Vinod

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic