Re: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xxprocessors.
On 9/13/07, Zhang Wei-r63237 <[EMAIL PROTECTED]> wrote:
> 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? :)
>
The index field gets used for multi-source (or multi-dest) operations.
XOR, for example, is a multi-source operation. I am preparing a
documentation patch.
[..]
--
Dan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xxprocessors.
On Thu, Sep 13, 2007 at 03:13:06AM -0700, Zhang Wei-r63237 wrote:
> > 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
Nope. The safety is against the particular item you're iterating on
being removed; it doesn't protect against the *next* entry being removed
when you drop the lock.
> > 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.
I'm not sure I understand what you mean by the latter comment...
> > 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?
Yes, that's what it's there for. :-)
> > > +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.
You don't need to bind to it for that, though -- just call of_get_parent
from the channel probe. Though it might be easier to bind to the parent
to ensure that you only register the IRQ once.
-Scott
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xxprocessors.
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(_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(_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 != _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, >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(_ln_lock, flags);
> > + list_for_each_entry_safe(desc, _desc, _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(_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(_ln_lock, flags);
> > + }
> > + /* Recycle it! */
> > + list_del(>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)
> > +{
>
RE: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xxprocessors.
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
Re: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xxprocessors.
On Thu, Sep 13, 2007 at 03:13:06AM -0700, Zhang Wei-r63237 wrote:
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
Nope. The safety is against the particular item you're iterating on
being removed; it doesn't protect against the *next* entry being removed
when you drop the lock.
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.
I'm not sure I understand what you mean by the latter comment...
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?
Yes, that's what it's there for. :-)
+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.
You don't need to bind to it for that, though -- just call of_get_parent
from the channel probe. Though it might be easier to bind to the parent
to ensure that you only register the IRQ once.
-Scott
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xxprocessors.
On 9/13/07, Zhang Wei-r63237 [EMAIL PROTECTED] wrote:
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? :)
The index field gets used for multi-source (or multi-dest) operations.
XOR, for example, is a multi-source operation. I am preparing a
documentation patch.
[..]
--
Dan
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
