lkml.org 
[lkml]   [2007]   [Sep]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRE: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xxprocessors.
    Date
    From
    Hi, 

    > > +static void fsl_dma_set_src(dma_addr_t addr,
    > > + struct dma_async_tx_descriptor
    > *tx, int index)
    > > +{
    >
    > What is index supposed to mean? It's not used, or documented
    > anywhere than
    > I can see.

    I've also got more document here. Hi, Dan, could you give me some
    explanation about this API? :)

    >
    > > + else {
    > > + /* Run the link descriptor callback function */
    > > + if (desc->async_tx.callback) {
    > > +
    > spin_unlock_irqrestore(&fsl_chan->desc_lock,
    > > + flags);
    > > + dev_dbg(fsl_chan->device->dev,
    > > + "link descriptor %p
    > callback\n", desc);
    > > + desc->async_tx.callback(
    > > +
    > desc->async_tx.callback_param);
    > > +
    > spin_lock_irqsave(&fsl_chan->desc_lock, flags);
    >
    > After dropping the lock, you can no longer assume that your
    > iterator is
    > still valid; you need to work off of the list head.
    >

    list_for_each_entry_safe() is used here. I think the safe should be ok.
    :P

    > > + /* Find the first un-transfer desciptor */
    > > + for (ld_node = fsl_chan->ld_queue.next;
    > > + (ld_node != &fsl_chan->ld_queue)
    > > + && (DMA_SUCCESS == dma_async_is_complete(
    > > +
    > to_fsl_desc(ld_node)->async_tx.cookie,
    > > + fsl_chan->completed_cookie,
    > > + fsl_chan->common.cookie));
    > > + ld_node = ld_node->next);
    >
    > Call fsl_dma_is_complete directly, don't waste time going through the
    > virtual call.
    >
    > And you have a recursive lock usage here; fsl_dma_is_complete calls
    > fsl_chan_ld_cleanup, which acquires desc_lock, but you
    > already have it.
    >
    > Couldn't you just call fsl_chan_ld_cleanup, and then check
    > what's at the
    > head of the list?
    >

    I'll split interrupt and poll here.

    > > +static irqreturn_t fsl_dma_do_interrupt(int irq, void *data)
    > > +{
    > > + struct fsl_dma_device *fdev = (struct fsl_dma_device *)data;
    > > + struct fsl_dma_chan *fsl_chan = NULL;
    > > + u32 gsr;
    > > + int ch_nr;
    > > + struct dma_chan *int_chan;
    > > +
    > > + gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ?
    > in_be32(fdev->reg_base)
    > > + : in_le32(fdev->reg_base);
    > > + ch_nr = (32 - ffs(gsr)) / 8;
    > > +
    > > + list_for_each_entry(int_chan, &fdev->common.channels,
    > device_node)
    > > + if (to_fsl_chan(int_chan)->id == ch_nr)
    > > + fsl_chan = to_fsl_chan(int_chan);
    >
    > Why not use an array of channels?

    The list is used in dma engine core file. And it's possible that there
    are not all channel listed in dts and array.

    > > +
    > > + return fsl_chan ? fsl_dma_chan_do_interrupt(irq,
    > fsl_chan) : IRQ_NONE;
    > > +
    > > +}
    > > +
    > > +static void dma_do_tasklet(unsigned long unused)
    > > +{
    > > + struct fsl_desc_sw *desc, *_desc;
    > > + unsigned long flags;
    > > +
    > > + spin_lock_irqsave(&recy_ln_lock, flags);
    > > + list_for_each_entry_safe(desc, _desc, &recy_ln_chain, node) {
    > > + struct fsl_dma_chan *fsl_chan =
    > > +
    > to_fsl_chan(desc->async_tx.chan);
    > > + /* Run the link descriptor callback function */
    > > + if (desc->async_tx.callback) {
    > > + spin_unlock_irqrestore(&recy_ln_lock, flags);
    > > + dev_dbg(fsl_chan->device->dev,
    > > + "dma_tasklet: link descriptor
    > %p callback\n",
    > > + desc);
    > > + desc->async_tx.callback(
    > > + desc->async_tx.callback_param);
    > > + spin_lock_irqsave(&recy_ln_lock, flags);
    > > + }
    > > + /* Recycle it! */
    > > + list_del(&desc->node);
    >
    > You should remove it from the list before dropping the lock,
    > as otherwise
    > something else could come along and remove it again.

    All right!

    >
    > > + if (strcmp(match->compatible, "fsl,mpc8540-dma-channel") == 0)
    > > + new_fsl_chan->feature = FSL_DMA_IP_86XX |
    > FSL_DMA_BIG_ENDIAN;
    >
    > Shouldn't it be 85XX, to be consistent?
    >
    > > + else if (strcmp(match->compatible,
    > "fsl,mpc8349-dma-channel") == 0)
    > > + new_fsl_chan->feature = FSL_DMA_IP_83XX |
    > FSL_DMA_LITTLE_ENDIAN;
    >
    > You could have the features be part of the match struct, so
    > you don't have
    > to do extra strcmps.
    >

    Can I use the data field of struct of_device_id?

    >
    > > +static struct of_device_id of_fsl_dma_ids[] = {
    > > + { .compatible = "fsl,dma", },
    > > +};
    >
    > Why do we need to bind to the parent node at all?

    Yes, the MPC83xx should get interrupt source from DMA device register.

    >
    > > +/* There is no asm instructions for 64 bits reverse loads
    > and stores */
    > > +static u64 in_le64(const u64 __iomem *addr)
    > > +{
    > > + return le64_to_cpu(in_be64(addr));
    > > +}
    > > +
    > > +static void out_le64(u64 __iomem *addr, u64 val)
    > > +{
    > > + out_be64(addr, cpu_to_le64(val));
    > > +}
    > > +#endif
    >
    > You can use asm instructions for this, as such:

    Aha, they are just copied from io.h.

    >
    > static u64 in_le64(const u64 __iomem *addr)
    > {
    > return ((u64)in_le32((u32 *)addr + 1) << 32) |
    > (in_le32((u32 *)addr));
    > }
    >
    >
    > static void out_le64(u64 __iomem *addr, u64 val)
    > {
    > out_le32((u32 *)addr, (u32)val);
    > out_le32((u32 *)addr + 1, val >> 32);
    > }
    >

    And I agree with your other comments.

    Thanks a lot!
    - zw
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-09-13 12:15    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site