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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
