> -----Original Message-----
> From: Wood Scott-B07421
>
> 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.
.... Remove it.
>
> > +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.
>
Remove 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.
>
I use a flexible ld ring size here. If there is no more free ring, the
driver can add new ld to the ring.
> 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?
>
Why there are extended mode in silicon? :) Since there are here, we use
it.
> > +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.
Ok. Do not check dest and src with NULL value. Hope the caller know what
he do.:)
>
> > + /* 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.
Since there is only happen at no more free ld in the ring. We need to
break the ld-ring and add new ld to it. So the most safe operation is to
stop DMA controller.
>
> > + /* 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?
The cookie will record all transfer by increased number. The ring index
is less for all transfer.
>
> > + 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.
>
Really? FSL_DMA_SR_CH is also a stat of event. And I need the
FSL_DMA_SR_CB status.
Thanks!
Wei.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
