On Tue, Jul 10, 2007 at 05:45:26PM +0800, Zhang Wei wrote:
> +config FSL_DMA
> + bool "Freescale MPC8xxx DMA support"
> + depends on DMA_ENGINE && (PPC_86xx || PPC_85xx)
Remove the dependency on specific PPC chips... let the device tree say
whether the hardware is present.
> +static inline int fsl_dma_idle(struct fsl_dma_chan *fsl_chan)
> +{
> + return (((in_be32(&fsl_chan->reg_base->sr) & FSL_DMA_SR_CB) == 0) &&
> + ((in_be32(&fsl_chan->reg_base->mr) & FSL_DMA_MR_CC) == 0));
> +}
I'm still not convinced that there's any reason to check MR_CC, even if
the driver *did* set it.
> + /* We need the descriptor to be aligned to 32bytes
> + * for meeting FSL DMA specification requirement.
> + */
> + fsl_chan->desc_pool = dma_pool_create("fsl_dma_engine_desc_pool",
> + fsl_chan->device->dev, sizeof(struct fsl_desc_sw),
> + 32, 0);
> + if (unlikely(!fsl_chan->desc_pool)) {
> + dev_err(fsl_chan->device->dev, "No memory for channel %d "
> + "descriptor dma pool.\n", fsl_chan->id);
> + return 0;
> + }
> +
> + /* Allocate list ring, and form the static list ring */
> + for (i = 0; i < FSLDMA_LD_INIT_RING_SIZE; i++) {
> + desc = fsl_dma_alloc_descriptor(fsl_chan->desc_pool,
> + GFP_KERNEL);
It'd be much simpler to allocate the entire ring at once. No need for
linked lists, DMA pools, etc. Just a single dma_alloc_coherent.
In general, this driver seems far more complex than it needs to be.
> + switch (fsl_chan->mode) {
> + case FSL_DMA_EXTENDED:
What benefit do we get out of using extended mode? If the driver can do
everything it needs to with basic, with no performance penalty, why not
always use basic?
> +static dma_cookie_t do_fsl_dma_memcpy(struct fsl_dma_chan *fsl_chan,
> + dma_addr_t dest,
> + dma_addr_t src, size_t len,
> + dma_xfer_callback cb, void *data)
> +{
> + struct fsl_desc_sw *first = NULL, *prev = NULL, *list, *new;
> + size_t copy;
> + dma_cookie_t cookie;
> + unsigned long flags;
> + struct fsl_dma_device *fdev = fsl_chan->device;
> + int err = 0;
> + LIST_HEAD(link_chain);
> +
> + if (unlikely(!fsl_chan || !dest || !src))
> + return -EFAULT;
-EINVAL for fsl_chan, if you bother checking at all (I wouldn't; it
doesn't come from untrusted code. Better to show a nasty-looking oops to
bring attention to the problem).
Don't check dest and src against NULL; zero is a potentially valid DMA
address.
> + /* Stop the DMA */
> + fsl_dma_halt(fsl_chan);
> + /* Insert the ld descriptor to the LD ring */
> + list_add(&ld->node, fsl_chan->enque);
> + switch (fsl_chan->mode) {
> + case FSL_DMA_EXTENDED:
> + INSERT_LD_RING(fsl_chan, ld, list, next_ls_addr);
> + break;
> + case FSL_DMA_BASIC:
> + INSERT_LD_RING(fsl_chan, ld, link, next_ln_addr);
> + break;
> + }
> + spin_unlock_irqrestore(&fsl_chan->desc_lock, flags);
It'd be nice if we didn't have to stop the DMA in order to insert new
descriptors.
> + /* cookie incr and addition to used_list must be atomic */
> + cookie = fsl_chan->common.cookie;
> + cookie++;
> + if (cookie < 0)
> + cookie = 1;
Why not just use the index into the ring as the cookie?
> + stat = in_be32(&fsl_chan->reg_base->sr);
> + dev_dbg(fsl_chan->device->dev, "event: channel %d, stat = 0x%x\n",
> + fsl_chan->id, stat);
> + if (!stat)
> + return IRQ_NONE;
> + busy = stat & (FSL_DMA_SR_CB);
> + stat &= ~(FSL_DMA_SR_CB | FSL_DMA_SR_CH);
This masking must happen *before* the IRQ_NONE check.
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
