Re: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xx processors.

["Dan Williams" <dan.j.williams@intel.com>]

On 9/7/07, Zhang Wei <wei.zhang@freescale.com> wrote:
> The driver implements DMA engine API for Freescale MPC85xx DMA
> controller, which could be used for MEM<-->MEM, IO_ADDR<-->MEM
> and IO_ADDR<-->IO_ADDR data transfer.
> The driver supports the Basic mode of Freescale MPC85xx DMA controller.
> The MPC85xx processors supported include MPC8540/60, MPC8555, MPC8548,
> MPC8641 and so on.
> The support for MPC83xx(MPC8349, MPC8360) is experimental.
>
> Signed-off-by: Zhang Wei <wei.zhang@freescale.com>
> Signed-off-by: Ebony Zhu <ebony.zhu@freescale.com>
> ---
Greetings,

Please copy me on any updates to this driver, drivers/dma, or crypto/async_tx.

Below are a few review comments...

Regards,
Dan

> +/**
> + * fsl_dma_alloc_descriptor - Allocate descriptor from channel's DMA pool.
> + *
> + * Return - The descriptor allocated. NULL for failed.
> + */
> +static struct fsl_desc_sw *fsl_dma_alloc_descriptor(
> +                                       struct fsl_dma_chan *fsl_chan,
> +                                       gfp_t flags)
> +{
> +       dma_addr_t pdesc;
> +       struct fsl_desc_sw *desc_sw;
> +
> +       desc_sw = dma_pool_alloc(fsl_chan->desc_pool, flags, &pdesc);
> +       if (desc_sw) {
> +               memset(desc_sw, 0, sizeof(struct fsl_desc_sw));
> +               dma_async_tx_descriptor_init(&desc_sw->async_tx,
> +                                               &fsl_chan->common);
> +               desc_sw->async_tx.tx_set_src = fsl_dma_set_src;
> +               desc_sw->async_tx.tx_set_dest = fsl_dma_set_dest;
> +               desc_sw->async_tx.tx_submit = fsl_dma_tx_submit;
> +               INIT_LIST_HEAD(&desc_sw->async_tx.tx_list);
> +               desc_sw->async_tx.phys = pdesc;
> +       }
> +
> +       return desc_sw;
> +}

I suggest pre-allocating the descriptors:
1/ It alleviates the need to initialize these async_tx fields which never change
2/ The GFP_ATOMIC allocation can be removed.

iop-adma gets by with one PAGE_SIZE buffer (128 descriptors).

[..]
> +static irqreturn_t fsl_dma_chan_do_interrupt(int irq, void *data)
> +{
> +       struct fsl_dma_chan *fsl_chan = (struct fsl_dma_chan *)data;
> +       dma_addr_t stat;
> +
> +       stat = get_sr(fsl_chan);
> +       dev_dbg(fsl_chan->device->dev, "event: channel %d, stat = 0x%x\n",
> +                                               fsl_chan->id, stat);
> +       set_sr(fsl_chan, stat);         /* Clear the event register */
> +
> +       stat &= ~(FSL_DMA_SR_CB | FSL_DMA_SR_CH);
> +       if (!stat)
> +               return IRQ_NONE;
> +
> +       /* If the link descriptor segment transfer finishes,
> +        * we will recycle the used descriptor.
> +        */
> +       if (stat & FSL_DMA_SR_EOSI) {
> +               dev_dbg(fsl_chan->device->dev, "event: End-of-segments INT\n");
> +               dev_dbg(fsl_chan->device->dev, "event: clndar 0x%016llx, "
> +                               "nlndar 0x%016llx\n", (u64)get_cdar(fsl_chan),
> +                               (u64)get_ndar(fsl_chan));
> +               stat &= ~FSL_DMA_SR_EOSI;
> +               fsl_chan_ld_cleanup(fsl_chan, 1);
> +       }
> +
> +       /* If it current transfer is the end-of-transfer,
> +        * we should clear the Channel Start bit for
> +        * prepare next transfer.
> +        */
> +       if (stat & (FSL_DMA_SR_EOLNI | FSL_DMA_SR_EOCDI)) {
> +               dev_dbg(fsl_chan->device->dev, "event: End-of-link INT\n");
> +               stat &= ~FSL_DMA_SR_EOLNI;
> +               fsl_chan_xfer_ld_queue(fsl_chan);
> +       }
> +
> +       if (stat)
> +               dev_dbg(fsl_chan->device->dev, "event: unhandled sr 0x%02x\n",
> +                                       stat);
> +
> +       dev_dbg(fsl_chan->device->dev, "event: Exit\n");
> +       tasklet_hi_schedule(&dma_tasklet);
> +       return IRQ_HANDLED;
> +}

This driver implements locking and callbacks inconsistently with how
it is done in ioatdma and iop-adma.  The big changes are that all
locks have been upgraded from 'spin_lock_bh' to 'spin_lock_irqsave',
and async_tx callbacks can happen in irq context.  I would like to
keep everything in bottom-half context to lessen the restrictions on
what async_tx clients can perform in their callback routines.  What
are the implications of moving 'fsl_chan_ld_cleanup' to the tasklet
and changing the 'tasklet_hi_schedule' to 'tasklet_schedule'?

[..]
> +static struct dma_chan *of_find_dma_chan_by_phandle(phandle phandle)
> +{
> +       struct device_node *np;
> +       struct dma_chan *chan;
> +       struct fsl_dma_device *fdev;
> +
> +       np = of_find_node_by_phandle(phandle);
> +       if (!np || !of_device_is_compatible(np->parent, "fsl,dma"))
> +               return NULL;
> +
> +       fdev = dev_get_drvdata(&of_find_device_by_node(np->parent)->dev);
> +
> +       list_for_each_entry(chan, &fdev->common.channels, device_node)
> +               if (to_of_device(to_fsl_chan(chan)->chan_dev)->node == np)
> +                       return chan;
> +       return NULL;
> +}
> +EXPORT_SYMBOL(of_find_dma_chan_by_phandle);

This routine implies that there is a piece of code somewhere that
wants to select which channels it can use.  A similar effect can be
achieved by registering a dma_client with the dmaengine interface
('dma_async_client_register').  Then when the client code makes a call
to 'dma_async_client_chan_request' it receives a 'dma_event_callback'
for each channel in the system.  It will also be asynchronously
notified of channels entering and leaving the system.  The goal is to
share a common infrastructure for channel management.

> +
> +static int __devinit of_fsl_dma_probe(struct of_device *dev,
> +                       const struct of_device_id *match)
> +{
> +       int err;
> +       unsigned int irq;
> +       struct fsl_dma_device *fdev;
> +
> +       fdev = kzalloc(sizeof(struct fsl_dma_device), GFP_KERNEL);
> +       if (!fdev) {
> +               dev_err(&dev->dev, "No enough memory for 'priv'\n");
> +               err = -ENOMEM;
> +               goto err;
> +       }
> +       fdev->dev = &dev->dev;
> +       INIT_LIST_HEAD(&fdev->common.channels);
> +
> +       /* get DMA controller register base */
> +       err = of_address_to_resource(dev->node, 0, &fdev->reg);
> +       if (err) {
> +               dev_err(&dev->dev, "Can't get %s property 'reg'\n",
> +                               dev->node->full_name);
> +               goto err;
> +       }
> +
> +       dev_info(&dev->dev, "Probe the Freescale DMA driver for %s "
> +                       "controller at 0x%08x...\n",
> +                       match->compatible, fdev->reg.start);
> +       fdev->reg_base = ioremap(fdev->reg.start, fdev->reg.end
> +                                               - fdev->reg.start + 1);
> +
> +       dma_cap_set(DMA_MEMCPY, fdev->common.cap_mask);
> +       fdev->common.device_alloc_chan_resources = fsl_dma_alloc_chan_resources;
> +       fdev->common.device_free_chan_resources = fsl_dma_free_chan_resources;
> +       fdev->common.device_prep_dma_memcpy = fsl_dma_prep_memcpy;
> +       fdev->common.device_is_tx_complete = fsl_dma_is_complete;
> +       fdev->common.device_issue_pending = fsl_dma_memcpy_issue_pending;
> +       fdev->common.device_dependency_added = fsl_dma_dependency_added;
> +       fdev->common.dev = &dev->dev;
> +
If this driver adds:

dma_cap_set(DMA_INTERRUPT, fdev->common.cap_mask);
fdev->common.device_prep_dma_interrupt = fsl_dma_prep_interrupt;

It will be able to take advantage of interrupt triggered callbacks in
async_tx.  Without these changes async_tx will poll for the completion
of each transaction.

[..]