[PATCH 1/5] ARM: define the PrimeCell DMA API

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Linus Walleij]

This patch extends the DMA engine with a PrimeCell superset with
three functions: configure DMA channel, stop channel, and one for
getting the number of bytes pending on a channel. These have been
identified as the basic extensions needed to drive DMA on top of
the PL011 and PL180 PrimeCells.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 include/linux/amba/dma.h |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/amba/dma.h

diff --git a/include/linux/amba/dma.h b/include/linux/amba/dma.h
new file mode 100644
index 0000000..13196af
--- /dev/null
+++ b/include/linux/amba/dma.h
@@ -0,0 +1,44 @@
+/*
+ *  linux/include/amba/dma.h
+ *
+ *  Copyright (C) 2010 ST-Ericsson AB
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#if !defined(AMBA_DMA_H) && defined(CONFIG_DMADEVICES)
+#define AMBA_DMA_H
+
+/**
+ * struct amba_dma_channel_request - this struct is passed in as
+ * configuration data to a DMA engine in order to set up a certain
+ * channel for DMA transport. Anything the DMA engine needs to
+ * know about the PrimeCell shall be passed through this struct.
+ * The DMA engine has to provide an additional function:
+ * dma_set_ambaconfig() in order for it to work with PrimeCells.
+ * @addr: this is the physical address where DMA data should be
+ * read (RX) or written (TX)
+ * @addr_width: this is the width of the source (RX) or target
+ * (TX) register where DMA data shall be read/written, in bytes.
+ * legal values: 1, 2, 4, 8.
+ */
+struct amba_dma_channel_config {
+	dma_addr_t addr;
+	u8 addr_width:4;
+};
+
+/*
+ * The following are extensions to the DMA engine framework
+ * that are needed to use the engine with PrimeCells. We need
+ * to configure devices and be able to stop transfers and
+ * retrieve the number of bytes left on the channel for some
+ * RX transfers when we don't know where and how the transfer
+ * will occur.
+ */
+void dma_set_ambaconfig(struct dma_chan *chan,
+			struct amba_dma_channel_config *config);
+void dma_stop_channel(struct dma_chan *chan);
+u32 dma_get_channel_bytes_left(struct dma_chan *chan);
+
+#endif /* AMBA_DMA_H */
-- 
1.6.2.5

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Dan Williams]

[ catching up on the patch backlog ]

On Mon, Mar 8, 2010 at 6:52 AM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> This patch extends the DMA engine with a PrimeCell superset with
> three functions: configure DMA channel, stop channel, and one for
> getting the number of bytes pending on a channel. These have been
> identified as the basic extensions needed to drive DMA on top of
> the PL011 and PL180 PrimeCells.
>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
> ?include/linux/amba/dma.h | ? 44 ++++++++++++++++++++++++++++++++++++++++++++
> ?1 files changed, 44 insertions(+), 0 deletions(-)
> ?create mode 100644 include/linux/amba/dma.h
>
> diff --git a/include/linux/amba/dma.h b/include/linux/amba/dma.h
> new file mode 100644
> index 0000000..13196af
> --- /dev/null
> +++ b/include/linux/amba/dma.h
> @@ -0,0 +1,44 @@
> +/*
> + * ?linux/include/amba/dma.h
> + *
> + * ?Copyright (C) 2010 ST-Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#if !defined(AMBA_DMA_H) && defined(CONFIG_DMADEVICES)
> +#define AMBA_DMA_H
> +
> +/**
> + * struct amba_dma_channel_request - this struct is passed in as
> + * configuration data to a DMA engine in order to set up a certain
> + * channel for DMA transport. Anything the DMA engine needs to
> + * know about the PrimeCell shall be passed through this struct.
> + * The DMA engine has to provide an additional function:
> + * dma_set_ambaconfig() in order for it to work with PrimeCells.
> + * @addr: this is the physical address where DMA data should be
> + * read (RX) or written (TX)
> + * @addr_width: this is the width of the source (RX) or target
> + * (TX) register where DMA data shall be read/written, in bytes.
> + * legal values: 1, 2, 4, 8.
> + */
> +struct amba_dma_channel_config {
> + ? ? ? dma_addr_t addr;
> + ? ? ? u8 addr_width:4;
> +};
> +
> +/*
> + * The following are extensions to the DMA engine framework
> + * that are needed to use the engine with PrimeCells. We need
> + * to configure devices and be able to stop transfers and
> + * retrieve the number of bytes left on the channel for some
> + * RX transfers when we don't know where and how the transfer
> + * will occur.
> + */
> +void dma_set_ambaconfig(struct dma_chan *chan,
> + ? ? ? ? ? ? ? ? ? ? ? struct amba_dma_channel_config *config);
> +void dma_stop_channel(struct dma_chan *chan);
> +u32 dma_get_channel_bytes_left(struct dma_chan *chan);

I question if these last two can be generically promoted to dmaengine?
 I already discussed a potential dma_get_channel_bytes() synonym with
Guennadi [1] (wrap ->device_is_tx_complete()), and dma_stop_channel()
looks like it could wrap ->device_terminate_all().

Thoughts?

--
Dan

[1]: http://marc.info/?l=linux-kernel&m=126324913710421&w=2

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Linus WALLEIJ]

As it happens I'm entertaining myself with this patchset
right now...

> > +void dma_set_ambaconfig(struct dma_chan *chan,
> > + ? ? ? ? ? ? ? ? ? ? ? struct amba_dma_channel_config *config);
> > +void dma_stop_channel(struct dma_chan *chan);
> > +u32 dma_get_channel_bytes_left(struct dma_chan *chan);
> 
> I question if these last two can be generically promoted to dmaengine?

OK...

I renamed get_channel_bytes_left() to get_channel_residue() btw.

>  I already discussed a potential dma_get_channel_bytes() synonym with
> Guennadi [1] (wrap ->device_is_tx_complete()),
> and dma_stop_channel()
> looks like it could wrap ->device_terminate_all().

Actually its a little bit like STOP/PAUSE/UNPAUSE on a tape
recorder, I find that in practice I have these two usecases:

* STOP (as in cancel) the DMA transfer and retrieve the number
  of bytes left at that point

* PAUSE the DMA transfer and retrieve the number of bytes
  left at that point, later UNPAUSE or STOP

* retrieve the number of bytes left in an ongoing transfer,
  a fluctuating value. This is typically nice for audio
  progress bars to take some simple example.

Right now, for the PrimeCells, I only need to be able to STOP the
channel and get the residual bytes at this time. So I'll create
something like dma_get_residue() in the DMA devices, and specify
the sematics such that if device_terminate_all() is called before
this point the residual at that time shall be returned, else
the current fluctuating value, would that be OK?

Yours,
Linus Walleij

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Dan Williams]

On Wed, Mar 24, 2010 at 2:38 PM, Linus WALLEIJ
<linus.walleij@stericsson.com> wrote:
> As it happens I'm entertaining myself with this patchset
> right now...
>
>> > +void dma_set_ambaconfig(struct dma_chan *chan,
>> > + ? ? ? ? ? ? ? ? ? ? ? struct amba_dma_channel_config *config);
>> > +void dma_stop_channel(struct dma_chan *chan);
>> > +u32 dma_get_channel_bytes_left(struct dma_chan *chan);
>>
>> I question if these last two can be generically promoted to dmaengine?
>
> OK...
>
> I renamed get_channel_bytes_left() to get_channel_residue() btw.
>
>> ?I already discussed a potential dma_get_channel_bytes() synonym with
>> Guennadi [1] (wrap ->device_is_tx_complete()),
>> and dma_stop_channel()
>> looks like it could wrap ->device_terminate_all().
>
> Actually its a little bit like STOP/PAUSE/UNPAUSE on a tape
> recorder, I find that in practice I have these two usecases:
>
> * STOP (as in cancel) the DMA transfer and retrieve the number
> ?of bytes left at that point
>
> * PAUSE the DMA transfer and retrieve the number of bytes
> ?left at that point, later UNPAUSE or STOP
>
> * retrieve the number of bytes left in an ongoing transfer,
> ?a fluctuating value. This is typically nice for audio
> ?progress bars to take some simple example.
>
> Right now, for the PrimeCells, I only need to be able to STOP the
> channel and get the residual bytes at this time. So I'll create
> something like dma_get_residue() in the DMA devices, and specify
> the sematics such that if device_terminate_all() is called before
> this point the residual at that time shall be returned, else
> the current fluctuating value, would that be OK?

Sounds ok, and I would not mind renaming device_is_tx_complete() to
device_tx_status() for this purpose.

What are your semantics for keeping the transaction information alive
until it gets consumed.  Currently the dma_ctrl_flags have the
DMA_CTRL_ACK bit to specify that the descriptor not be
recycled/destroyed until the client has a chance to attach a dependent
operation.  Seems you need something similar to indicate that the
residue information not be destroyed until it is consumed.  In other
words how does a client guarantee that the engine provides the
information it needs?

--
Dan

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Linus WALLEIJ]

[Dan]

> What are your semantics for keeping the transaction information alive
> until it gets consumed.  Currently the dma_ctrl_flags have the
> DMA_CTRL_ACK bit to specify that the descriptor not be
> recycled/destroyed until the client has a chance to attach a dependent
> operation.  Seems you need something similar to indicate that the
> residue information not be destroyed until it is consumed.  In other
> words how does a client guarantee that the engine provides the
> information it needs?

Well exactly to avoid that kind of trouble I STOP (or PAUSE),
get numer of bytes left, then terminate in that order. This
keeps the descriptor.

Perhaps it's better to keep the two functions _pause() and
_get_residue() and indicate that device_terminate_all()
may destroy the state.

Yours,
Linus Walleij

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Dan Williams]

On Wed, Mar 24, 2010 at 3:13 PM, Linus WALLEIJ
<linus.walleij@stericsson.com> wrote:
> [Dan]
>
>> What are your semantics for keeping the transaction information alive
>> until it gets consumed. ?Currently the dma_ctrl_flags have the
>> DMA_CTRL_ACK bit to specify that the descriptor not be
>> recycled/destroyed until the client has a chance to attach a dependent
>> operation. ?Seems you need something similar to indicate that the
>> residue information not be destroyed until it is consumed. ?In other
>> words how does a client guarantee that the engine provides the
>> information it needs?
>
> Well exactly to avoid that kind of trouble I STOP (or PAUSE),
> get numer of bytes left, then terminate in that order. This
> keeps the descriptor.

But doesn't this assume you win the race with the engine completing
and cleaning up the descriptor before the 'stop' takes effect?

> Perhaps it's better to keep the two functions _pause() and
> _get_residue() and indicate that device_terminate_all()
> may destroy the state.

The configuration issues will always be arch specific I grant you
that, but I think we can define top level "operation query" and
"channel control" apis in a generic/extensible fashion.  Something
like:

1/ Change ->device_terminate_all() into ->device_control():

enum {
	DMA_TERMINATE_ALL, /* legacy semantics */
	DMA_PAUSE, /* your new primecell command semantics */
} dma_crtl;

int (*device_control)(struct dma_chan *chan, enum dma_ctrl cmd);


2/ Change device_is_tx_complete to device_tx_status along the lines of
what Guennadi and I talked about.

--
Dan

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Linus WALLEIJ]

[Dan]

> From: dan.j.williams at gmail.com [mailto:dan.j.williams at gmail.com] On Behalf
> Of Dan Williams
> On Wed, Mar 24, 2010 at 3:13 PM, Linus WALLEIJ
> <linus.walleij@stericsson.com> wrote:
> > [Dan]
> >
> >> What are your semantics for keeping the transaction information alive
> >> until it gets consumed. ?Currently the dma_ctrl_flags have the
> >> DMA_CTRL_ACK bit to specify that the descriptor not be
> >> recycled/destroyed until the client has a chance to attach a dependent
> >> operation. ?Seems you need something similar to indicate that the
> >> residue information not be destroyed until it is consumed. ?In other
> >> words how does a client guarantee that the engine provides the
> >> information it needs?
> >
> > Well exactly to avoid that kind of trouble I STOP (or PAUSE),
> > get numer of bytes left, then terminate in that order. This
> > keeps the descriptor.
> 
> But doesn't this assume you win the race with the engine completing
> and cleaning up the descriptor before the 'stop' takes effect?

Sure does! So now I have to think of some solution to that.
I'll hack around a bit...

> The configuration issues will always be arch specific I grant you
> that, but I think we can define top level "operation query" and
> "channel control" apis in a generic/extensible fashion.  Something
> like:
> 
> 1/ Change ->device_terminate_all() into ->device_control():
> 
> enum {
> 	DMA_TERMINATE_ALL, /* legacy semantics */
> 	DMA_PAUSE, /* your new primecell command semantics */
> } dma_crtl;
> 
> int (*device_control)(struct dma_chan *chan, enum dma_ctrl cmd);
> 
> 
> 2/ Change device_is_tx_complete to device_tx_status along the lines of
> what Guennadi and I talked about.

OK I'll take a stab at this... There are not too many users so
should be possible to switch them over easily.

Yours,
Linus Walleij

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Dan Williams]

On Mon, Mar 8, 2010 at 6:52 AM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> This patch extends the DMA engine with a PrimeCell superset with
> three functions: configure DMA channel, stop channel, and one for
> getting the number of bytes pending on a channel. These have been
> identified as the basic extensions needed to drive DMA on top of
> the PL011 and PL180 PrimeCells.
>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
> ?include/linux/amba/dma.h | ? 44 ++++++++++++++++++++++++++++++++++++++++++++
> ?1 files changed, 44 insertions(+), 0 deletions(-)
> ?create mode 100644 include/linux/amba/dma.h
>
> diff --git a/include/linux/amba/dma.h b/include/linux/amba/dma.h
> new file mode 100644
> index 0000000..13196af
> --- /dev/null
> +++ b/include/linux/amba/dma.h
> @@ -0,0 +1,44 @@
> +/*
> + * ?linux/include/amba/dma.h
> + *
> + * ?Copyright (C) 2010 ST-Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#if !defined(AMBA_DMA_H) && defined(CONFIG_DMADEVICES)
> +#define AMBA_DMA_H
> +
> +/**
> + * struct amba_dma_channel_request - this struct is passed in as
> + * configuration data to a DMA engine in order to set up a certain
> + * channel for DMA transport. Anything the DMA engine needs to
> + * know about the PrimeCell shall be passed through this struct.
> + * The DMA engine has to provide an additional function:
> + * dma_set_ambaconfig() in order for it to work with PrimeCells.
> + * @addr: this is the physical address where DMA data should be
> + * read (RX) or written (TX)
> + * @addr_width: this is the width of the source (RX) or target
> + * (TX) register where DMA data shall be read/written, in bytes.
> + * legal values: 1, 2, 4, 8.
> + */
> +struct amba_dma_channel_config {
> + ? ? ? dma_addr_t addr;
> + ? ? ? u8 addr_width:4;
> +};

The usages of this seem to be constant data based off something that
comes from the platform data or 'slave' device.  Could this constant
data just be munged/merged into the platform data and we can look it
up at runtime via chan->private.  Or am I oversimplifying?

None the of the usages seem to justify forking from core dmaengine at
this point (I reserve the right to be proven wrong later).

--
Dan

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Linus WALLEIJ]

[Dan]

> > +/**
> > + * struct amba_dma_channel_request - this struct is passed in as
> > + * configuration data to a DMA engine in order to set up a certain
> > + * channel for DMA transport. Anything the DMA engine needs to
> > + * know about the PrimeCell shall be passed through this struct.
> > + * The DMA engine has to provide an additional function:
> > + * dma_set_ambaconfig() in order for it to work with PrimeCells.
> > + * @addr: this is the physical address where DMA data should be
> > + * read (RX) or written (TX)
> > + * @addr_width: this is the width of the source (RX) or target
> > + * (TX) register where DMA data shall be read/written, in bytes.
> > + * legal values: 1, 2, 4, 8.
> > + */
> > +struct amba_dma_channel_config {
> > + ? ? ? dma_addr_t addr;
> > + ? ? ? u8 addr_width:4;
> > +};
> 
> The usages of this seem to be constant data based off something that
> comes from the platform data or 'slave' device.  Could this constant
> data just be munged/merged into the platform data and we can look it
> up at runtime via chan->private.  Or am I oversimplifying?

Well actually, the addr is constant in the cases I've seen so
far but doesn't have to be, e.g. for a bidirectional channel
which shall do RX from one address and TX from a different address,
though I haven't seen such a thing.

addr_width is subject to the above case as well, however now I
also have DMA for the PL022 driver cooking and that actually needs
to switch the width of the address endpoint in runtime. (Soon
to come...)

So this needs some specific function call or alter chan->private
in runtime.

> None the of the usages seem to justify forking from core dmaengine at
> this point (I reserve the right to be proven wrong later).

It's more of a superset than a fork really...

I think the crucial point is that a client driver, say
drivers/serial/amba-pl011.c need to supply PrimeCell-specific
configuration in runtime without knowing which DMA engine
is being used.

If I just assign PrimeCell configuration say something like
this:

struct amba_dma_channel_config conf;
chan->private = &conf;

Then the DMA engine has to either:

- Only support primecells and cast all chan->private to
  struct amba_dma_channel_config (this is rarely the case,
  we have a lot of other custom devices which are not
  primecells at all so won't work for us)

- Or know which channels are assigned to primecells before
  we assign this configuration, so it can keep track of it
  internally in some

struct mypriv {
	bool is_amba;
	struct amba_dma_channel_config *ambaconf;
	bool is_other;
	struct other_config *otherconf;
}
struct mypriv foo = {
	.is_amba = true,
	...
};
chan->private = &foo;

  And is_amba must be set to true for all primecells before
  we start probing clients. In runtime this would require
  things like this:

struct mypriv *foo = chan->private;
foo->ambaconf->addr_width = 2;

Do you prefer this latter solution? I'm OK with either, I just
think the runtime dma_set_ambaconfig() is a bit more elegant.

Linus

[PATCH 1/5] ARM: define the PrimeCell DMA API

[Dan Williams]

On Wed, Mar 24, 2010 at 3:08 PM, Linus WALLEIJ
<linus.walleij@stericsson.com> wrote:
> Do you prefer this latter solution? I'm OK with either, I just
> think the runtime dma_set_ambaconfig() is a bit more elegant.

I agree, too many hoops to jump through to shoehorn it into the
current api.  Let's go with your current amba specific version for
now.

--
Dan