Re: [PATCH v5 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource

[Sanjay R Mehta]

On 7/3/2020 1:07 PM, Vinod Koul wrote:
> 
> On 16-06-20, 20:11, Sanjay R Mehta wrote:
> 
>> --- a/drivers/dma/ptdma/Makefile
>> +++ b/drivers/dma/ptdma/Makefile
>> @@ -5,6 +5,7 @@
>>
>>  obj-$(CONFIG_AMD_PTDMA) += ptdma.o
>>
>> -ptdma-objs := ptdma-dev.o
>> +ptdma-objs := ptdma-dev.o \
>> +           ptdma-dmaengine.o
> 
> Single line?
> 
Yes.

>> +static void pt_free_chan_resources(struct dma_chan *dma_chan)
>> +{
>> +     struct pt_dma_chan *chan = container_of(dma_chan, struct pt_dma_chan,
>> +                                              vc.chan);
>> +
>> +     dev_dbg(chan->pt->dev, "%s - chan=%p\n", __func__, chan);
> 
> drop the dbg artifacts here and other places in this and other patches
> 
Sure. Will fix this in the next version of patch.
>> +static void pt_do_cleanup(struct virt_dma_desc       *vd)
>> +
>> +{
>> +     struct pt_dma_desc *desc = container_of(vd, struct pt_dma_desc, vd);
>> +     struct pt_device *pt = desc->pt;
>> +     struct pt_dma_chan *chan;
>> +
>> +     chan = container_of(desc->vd.tx.chan, struct pt_dma_chan,
>> +                         vc.chan);
> 
> add a to_pt_chan() macro for this?
> 
Will fix this in the next version of patch.
>> +static int pt_issue_next_cmd(struct pt_dma_desc *desc)
>> +{
>> +     struct pt_passthru_engine *pt_engine;
>> +     struct pt_dma_cmd *cmd;
>> +     struct pt_device *pt;
>> +     struct pt_cmd *pt_cmd;
>> +     struct pt_cmd_queue *cmd_q;
>> +
>> +     cmd = list_first_entry(&desc->cmdlist, struct pt_dma_cmd, entry);
>> +     desc->actv = 1;
> 
> active?
> 
This is used to indicate that the command has been submitted to the PTDMA queue.
This variable is being used in many places in the code.
If the name "actv" is confusing here, I'll change to something else.

>> +
>> +     dev_dbg(desc->pt->dev, "%s - tx %d, cmd=%p\n", __func__,
>> +             desc->vd.tx.cookie, cmd);
>> +
>> +     pt_cmd = &cmd->pt_cmd;
>> +     pt = pt_cmd->pt;
>> +     cmd_q = &pt->cmd_q;
>> +     pt_engine = &pt_cmd->passthru;
>> +
>> +     if (!pt_engine->final)
>> +             return -EINVAL;
> 
> what does final mean here?
This was used to indicate completion in the initial version of code. This has no use now.
Will remove in the next version of patch.
>> +
>> +     if (!pt_engine->src_dma || !pt_engine->dst_dma)
>> +             return -EINVAL;
> 
> what does this check do? we have a valid cmd which IIUC implies a valid
> dma txn so why would one of this be invalid?
> 
Yes, you are right. Will fix this in the next version of patch.

>> +static struct pt_dma_desc *__pt_next_dma_desc(struct pt_dma_chan *chan)
>> +{
>> +     /* Get the next DMA descriptor on the active list */
>> +     struct virt_dma_desc *vd = vchan_next_desc(&chan->vc);
>> +
>> +     if (list_empty(&chan->vc.desc_submitted))
>> +             return NULL;
>> +
>> +     vd = list_empty(&chan->vc.desc_issued) ?
>> +               list_first_entry(&chan->vc.desc_submitted,
>> +                                struct virt_dma_desc, node) : NULL;
> 
> Always remember there might already be a macro, so check. In this case
> use of list_first_entry_or_null() looks apt
> 
Sure. Will fix this in the next version of patch.
>> +static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>> +                                              struct pt_dma_desc *desc)
>> +{
>> +     struct dma_async_tx_descriptor *tx_desc;
>> +     struct virt_dma_desc *vd;
>> +     unsigned long flags;
>> +
>> +     /* Loop over descriptors until one is found with commands */
> 
> This bit is strange, am not sure I follow. The fn name tell me it would
> handle and active descriptor which is passed as an arg, so why do you
> loop?
> 
> Can you explain this?
> 
There are two tasks this function handles.

First, this function checks whether the passed descriptor is already submitted
for the PDMA queue or not. If not, it will return the descriptor to submit for
the DMA txn to the queue.

Secondly, it loops through all the descriptors from the issued list and checks
if the all descriptor has been handled or not. If not, then processes them in the loop.

>> +static void pt_issue_pending(struct dma_chan *dma_chan)
>> +{
>> +     struct pt_dma_chan *chan = container_of(dma_chan, struct pt_dma_chan,
>> +                                              vc.chan);
>> +     struct pt_dma_desc *desc;
>> +     unsigned long flags;
>> +
>> +     dev_dbg(chan->pt->dev, "%s\n", __func__);
>> +
>> +     spin_lock_irqsave(&chan->vc.lock, flags);
>> +
>> +     desc = __pt_next_dma_desc(chan);
>> +
>> +     spin_unlock_irqrestore(&chan->vc.lock, flags);
>> +
>> +     /* If there was nothing active, start processing */
> 
> What if channel is already active and doing a transaction? This should
> check it first..
> 
This case is handled by PTDMA engine. Therefore,the channel busy case is not checked here.

The PTDMA hardware queue has two pointers to manage the queue "head" and "tail" pointer.
The head pointer points to first (oldest) command in the queue and only the initial value
written by software prior to enabling queue. Hardware updates this pointer when it fetches
a Command Descriptor from memory. Software is not allowed to modify this register.

The software is supposed to update only the tail pointer of the queue with DMA txn.

>> +int pt_dmaengine_register(struct pt_device *pt)
>> +{
>> +     struct pt_dma_chan *chan;
>> +     struct dma_device *dma_dev = &pt->dma_dev;
>> +     struct dma_chan *dma_chan;
>> +     char *dma_cmd_cache_name;
>> +     char *dma_desc_cache_name;
>> +     int ret;
>> +
>> +     pt->pt_dma_chan = devm_kcalloc(pt->dev, 1,
>> +                                    sizeof(*pt->pt_dma_chan),
>> +                                    GFP_KERNEL);
> 
> If n is 1, why you kcalloc, why not devm_kzalloc()?
Will fix this in the next version of patch.
> 
>> +     if (!pt->pt_dma_chan)
>> +             return -ENOMEM;
>> +
>> +     dma_cmd_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
>> +                                         "%s-dmaengine-cmd-cache",
>> +                                         pt->name);
>> +     if (!dma_cmd_cache_name)
>> +             return -ENOMEM;
>> +
>> +     pt->dma_cmd_cache = kmem_cache_create(dma_cmd_cache_name,
>> +                                           sizeof(struct pt_dma_cmd),
>> +                                           sizeof(void *),
>> +                                           SLAB_HWCACHE_ALIGN, NULL);
>> +     if (!pt->dma_cmd_cache)
>> +             return -ENOMEM;
>> +
>> +     dma_desc_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
>> +                                          "%s-dmaengine-desc-cache",
>> +                                          pt->name);
>> +     if (!dma_desc_cache_name) {
>> +             ret = -ENOMEM;
>> +             goto err_cache;
>> +     }
>> +
>> +     pt->dma_desc_cache = kmem_cache_create(dma_desc_cache_name,
>> +                                            sizeof(struct pt_dma_desc),
>> +                                            sizeof(void *),
>> +                                            SLAB_HWCACHE_ALIGN, NULL);
>> +     if (!pt->dma_desc_cache) {
>> +             ret = -ENOMEM;
>> +             goto err_cache;
>> +     }
>> +
>> +     dma_dev->dev = pt->dev;
>> +     dma_dev->src_addr_widths = PT_DMA_WIDTH(dma_get_mask(pt->dev));
>> +     dma_dev->dst_addr_widths = PT_DMA_WIDTH(dma_get_mask(pt->dev));
>> +     dma_dev->directions = DMA_MEM_TO_MEM;
>> +     dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
>> +     dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
>> +     dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
>> +     dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
> 
> Why DMA_PRIVATE if it supports only memcpy? Also have you tested this
> with dmatest?
> 
This DMA controller is intended to use with AMD Non-Transparent Bridge devices
and not for general purpose slave DMA. Therefore we made it as DMA_PRIVATE.

Yes, I had verified with the dmatest.

> --
> ~Vinod
>