RFC: changing DMA slave configuration API

RFC: changing DMA slave configuration API

[Russell King - ARM Linux]

Dan, Vinod,

There's a change I would like to do to the DMA slave configuration.
It's currently a pain to have the source and destination parameters in
the dma_slave_config structure as separate elements; it means when you
want to extract them, you end up with code in DMA engine drivers like:

+	if (dir == DMA_DEV_TO_MEM) {
+		dev_addr = c->src_addr;
+		dev_width = c->src_addr_width;
+		burst = c->src_maxburst;
+	} else if (dir == DMA_MEM_TO_DEV) {
+		dev_addr = c->dst_addr;
+		dev_width = c->dst_addr_width;
+		burst = c->dst_maxburst;
+	}

If we redefine the structure as below, this all becomes more simple:

+	if (dir == DMA_DEV_TO_MEM)
+		cfg = &c->dev_src;
+	else if (dir == DMA_MEM_TO_DEV)
+		cfg = &c->dev_dst;

and then we can access the data through cfg->{element} rather than having
to cache each individual elements value in a local variable.

Thoughts?

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 56377df..e6519f7 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -367,6 +367,18 @@ struct dma_slave_config {
 	bool device_fc;
 };
 
+struct dma_dev_cfg {
+	dma_addr_t addr;
+	enum dma_slave_buswidth width;
+	u32 maxburst;
+};
+
+struct dma_slave_cfg {
+	struct dma_dev_cfg dev_src;
+	struct dma_dev_cfg dev_dst;
+	bool device_fc;
+};
+
 static inline const char *dma_chan_name(struct dma_chan *chan)
 {
 	return dev_name(&chan->dev->device);

RFC: changing DMA slave configuration API

[Barry Song]

2012/6/10 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> Dan, Vinod,
>
> There's a change I would like to do to the DMA slave configuration.
> It's currently a pain to have the source and destination parameters in
> the dma_slave_config structure as separate elements; it means when you
> want to extract them, you end up with code in DMA engine drivers like:
>
> + ? ? ? if (dir == DMA_DEV_TO_MEM) {
> + ? ? ? ? ? ? ? dev_addr = c->src_addr;
> + ? ? ? ? ? ? ? dev_width = c->src_addr_width;
> + ? ? ? ? ? ? ? burst = c->src_maxburst;
> + ? ? ? } else if (dir == DMA_MEM_TO_DEV) {
> + ? ? ? ? ? ? ? dev_addr = c->dst_addr;
> + ? ? ? ? ? ? ? dev_width = c->dst_addr_width;
> + ? ? ? ? ? ? ? burst = c->dst_maxburst;
> + ? ? ? }
>
> If we redefine the structure as below, this all becomes more simple:
>
> + ? ? ? if (dir == DMA_DEV_TO_MEM)
> + ? ? ? ? ? ? ? cfg = &c->dev_src;
> + ? ? ? else if (dir == DMA_MEM_TO_DEV)
> + ? ? ? ? ? ? ? cfg = &c->dev_dst;

it seems that might mean an union in your dma_slave_cfg, but not
co-exitense of both?

struct dma_slave_cfg {
+       union {
              struct dma_dev_cfg dev_src;
              struct dma_dev_cfg dev_dst;
       }
       bool device_fc;
};

>
> and then we can access the data through cfg->{element} rather than having
> to cache each individual elements value in a local variable.
>
> Thoughts?
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 56377df..e6519f7 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -367,6 +367,18 @@ struct dma_slave_config {
> ? ? ? ?bool device_fc;
> ?};
>
> +struct dma_dev_cfg {
> + ? ? ? dma_addr_t addr;
> + ? ? ? enum dma_slave_buswidth width;
> + ? ? ? u32 maxburst;
> +};
> +
> +struct dma_slave_cfg {
> + ? ? ? struct dma_dev_cfg dev_src;
> + ? ? ? struct dma_dev_cfg dev_dst;
> + ? ? ? bool device_fc;
> +};
> +
> ?static inline const char *dma_chan_name(struct dma_chan *chan)
> ?{
> ? ? ? ?return dev_name(&chan->dev->device);
>
Thanks
barry

RFC: changing DMA slave configuration API

[Russell King - ARM Linux]

On Sun, Jun 10, 2012 at 07:19:47PM +0800, Barry Song wrote:
> 2012/6/10 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > Dan, Vinod,
> >
> > There's a change I would like to do to the DMA slave configuration.
> > It's currently a pain to have the source and destination parameters in
> > the dma_slave_config structure as separate elements; it means when you
> > want to extract them, you end up with code in DMA engine drivers like:
> >
> > + ? ? ? if (dir == DMA_DEV_TO_MEM) {
> > + ? ? ? ? ? ? ? dev_addr = c->src_addr;
> > + ? ? ? ? ? ? ? dev_width = c->src_addr_width;
> > + ? ? ? ? ? ? ? burst = c->src_maxburst;
> > + ? ? ? } else if (dir == DMA_MEM_TO_DEV) {
> > + ? ? ? ? ? ? ? dev_addr = c->dst_addr;
> > + ? ? ? ? ? ? ? dev_width = c->dst_addr_width;
> > + ? ? ? ? ? ? ? burst = c->dst_maxburst;
> > + ? ? ? }
> >
> > If we redefine the structure as below, this all becomes more simple:
> >
> > + ? ? ? if (dir == DMA_DEV_TO_MEM)
> > + ? ? ? ? ? ? ? cfg = &c->dev_src;
> > + ? ? ? else if (dir == DMA_MEM_TO_DEV)
> > + ? ? ? ? ? ? ? cfg = &c->dev_dst;
> 
> it seems that might mean an union in your dma_slave_cfg, but not
> co-exitense of both?

No, I want both so it's possible to select between the two when preparing
a DMA slave transfer.

> struct dma_slave_cfg {
> +       union {
>               struct dma_dev_cfg dev_src;
>               struct dma_dev_cfg dev_dst;
>        }
>        bool device_fc;
> };

If you do that, the union becomes pointless, and you might as well have:

struct dma_slave_cfg {
	struct dma_dev_cfg dev;
	bool device_fc;
};

instead.

RFC: changing DMA slave configuration API

[Vinod Koul]

On Sun, 2012-06-10 at 12:22 +0100, Russell King - ARM Linux wrote:
> On Sun, Jun 10, 2012 at 07:19:47PM +0800, Barry Song wrote:
> > 2012/6/10 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > > Dan, Vinod,
> > >
> > > There's a change I would like to do to the DMA slave configuration.
> > > It's currently a pain to have the source and destination parameters in
> > > the dma_slave_config structure as separate elements; it means when you
> > > want to extract them, you end up with code in DMA engine drivers like:
> > >
> > > +       if (dir == DMA_DEV_TO_MEM) {
> > > +               dev_addr = c->src_addr;
> > > +               dev_width = c->src_addr_width;
> > > +               burst = c->src_maxburst;
> > > +       } else if (dir == DMA_MEM_TO_DEV) {
> > > +               dev_addr = c->dst_addr;
> > > +               dev_width = c->dst_addr_width;
> > > +               burst = c->dst_maxburst;
> > > +       }
> > >
> > > If we redefine the structure as below, this all becomes more simple:
> > >
> > > +       if (dir == DMA_DEV_TO_MEM)
> > > +               cfg = &c->dev_src;
> > > +       else if (dir == DMA_MEM_TO_DEV)
> > > +               cfg = &c->dev_dst;
> > 
> > it seems that might mean an union in your dma_slave_cfg, but not
> > co-exitense of both?
> 
> No, I want both so it's possible to select between the two when preparing
> a DMA slave transfer.
> 
> > struct dma_slave_cfg {
> > +       union {
> >               struct dma_dev_cfg dev_src;
> >               struct dma_dev_cfg dev_dst;
> >        }
> >        bool device_fc;
> > };
> 
> If you do that, the union becomes pointless, and you might as well have:
> 
> struct dma_slave_cfg {
> 	struct dma_dev_cfg dev;
> 	bool device_fc;
> };
> 
> instead.
Hi Russell,

I think it is a good idea. And I would like to extend it even a little
bit. Do we have any users of peripheral to peripheral slave dma?
IIRC  that is not the case, or does anyone know of existence or plans
for such a h/w?

If not, lets junk the src/dst fields and keep burst, length, addr fields
which point to the peripheral values.

Alternatively if we need both, then we can't have union and Russell's
idea seems good one :)


-- 
~Vinod

RFC: changing DMA slave configuration API

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 10:20:49AM +0530, Vinod Koul wrote:
> I think it is a good idea. And I would like to extend it even a little
> bit. Do we have any users of peripheral to peripheral slave dma?
> IIRC  that is not the case, or does anyone know of existence or plans
> for such a h/w?
> 
> If not, lets junk the src/dst fields and keep burst, length, addr fields
> which point to the peripheral values.
> 
> Alternatively if we need both, then we can't have union and Russell's
> idea seems good one :)

We don't need the union whatever way that goes.

The question over whether we have the src/dst fields is whether we want
to support a different configuration for DMA_DEV_TO_MEM/DMA_MEM_TO_DEV
without having to reconfigure the channel each time its direction is
switched.

Out of the following users:
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c
drivers/tty/serial/amba-pl011.c

with amba-pl08x, I don't see any which set a different configuration
depending on direction, and sa11x0 and OMAP DMA engine drivers only
support one direction per channel.

So, the question really comes down to whether we _want_ to support this,
and how painful it would be to re-introduce this if we did need it.
Maybe the right answer is to use the control command value to sort that
out when we come to it.

RFC: changing DMA slave configuration API

[Vinod Koul]

On Mon, 2012-06-11 at 09:24 +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 11, 2012 at 10:20:49AM +0530, Vinod Koul wrote:
> > I think it is a good idea. And I would like to extend it even a little
> > bit. Do we have any users of peripheral to peripheral slave dma?
> > IIRC  that is not the case, or does anyone know of existence or plans
> > for such a h/w?
> > 
> > If not, lets junk the src/dst fields and keep burst, length, addr fields
> > which point to the peripheral values.
> > 
> > Alternatively if we need both, then we can't have union and Russell's
> > idea seems good one :)
> 
> We don't need the union whatever way that goes.
Based on comment we need to support both.

> 
> The question over whether we have the src/dst fields is whether we want
> to support a different configuration for DMA_DEV_TO_MEM/DMA_MEM_TO_DEV
> without having to reconfigure the channel each time its direction is
> switched.
The biggest issue here is the design of the API. IMO the slave_config
should be passed along with respective prepare API for slave and not
thru separate slave config. That will remove the unnecessary limitation
and allow the same channel to be used for tx for one transfer and rx for
subsequent etc.

In the .device_prep_slave_sg() we should add the struct slave_config as
an argument. Obviously the slave_config will have _one_ pair of members
(not both src/dstn fields then).

For DMA_DEV_TO_DEV, anyway we need a new API, which can have both src
and dstn slave_config

Thoughts..?


-- 
~Vinod

RFC: changing DMA slave configuration API

[Dong Aisheng]

On Mon, Jun 11, 2012 at 10:20:49AM +0530, Vinod Koul wrote:
> On Sun, 2012-06-10 at 12:22 +0100, Russell King - ARM Linux wrote:
> > On Sun, Jun 10, 2012 at 07:19:47PM +0800, Barry Song wrote:
> > > 2012/6/10 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > > > Dan, Vinod,
> > > >
> > > > There's a change I would like to do to the DMA slave configuration.
> > > > It's currently a pain to have the source and destination parameters in
> > > > the dma_slave_config structure as separate elements; it means when you
> > > > want to extract them, you end up with code in DMA engine drivers like:
> > > >
> > > > +       if (dir == DMA_DEV_TO_MEM) {
> > > > +               dev_addr = c->src_addr;
> > > > +               dev_width = c->src_addr_width;
> > > > +               burst = c->src_maxburst;
> > > > +       } else if (dir == DMA_MEM_TO_DEV) {
> > > > +               dev_addr = c->dst_addr;
> > > > +               dev_width = c->dst_addr_width;
> > > > +               burst = c->dst_maxburst;
> > > > +       }
> > > >
> > > > If we redefine the structure as below, this all becomes more simple:
> > > >
> > > > +       if (dir == DMA_DEV_TO_MEM)
> > > > +               cfg = &c->dev_src;
> > > > +       else if (dir == DMA_MEM_TO_DEV)
> > > > +               cfg = &c->dev_dst;
> > > 
> > > it seems that might mean an union in your dma_slave_cfg, but not
> > > co-exitense of both?
> > 
> > No, I want both so it's possible to select between the two when preparing
> > a DMA slave transfer.
> > 
> > > struct dma_slave_cfg {
> > > +       union {
> > >               struct dma_dev_cfg dev_src;
> > >               struct dma_dev_cfg dev_dst;
> > >        }
> > >        bool device_fc;
> > > };
> > 
> > If you do that, the union becomes pointless, and you might as well have:
> > 
> > struct dma_slave_cfg {
> > 	struct dma_dev_cfg dev;
> > 	bool device_fc;
> > };
> > 
> > instead.
> Hi Russell,
> 
> I think it is a good idea. And I would like to extend it even a little
> bit. Do we have any users of peripheral to peripheral slave dma?
Yes, IMX sdma does support such kind of transfer.
The driver still does not support it currently.

> IIRC  that is not the case, or does anyone know of existence or plans
> for such a h/w?
> 
i.MX5 and i.MX6.

> If not, lets junk the src/dst fields and keep burst, length, addr fields
> which point to the peripheral values.
> 
> Alternatively if we need both, then we can't have union and Russell's
> idea seems good one :)
> 
Russell's idea seems reasonable and we may want to support peripheral to
peripheral in the future.

Regards
Dong Aisheng

RFC: changing DMA slave configuration API

[Vinod Koul]

On Mon, 2012-06-11 at 17:33 +0800, Dong Aisheng wrote:
> > I think it is a good idea. And I would like to extend it even a
> little
> > bit. Do we have any users of peripheral to peripheral slave dma?
> Yes, IMX sdma does support such kind of transfer.
> The driver still does not support it currently.
> 
> > IIRC  that is not the case, or does anyone know of existence or
> plans
> > for such a h/w?
> > 
> i.MX5 and i.MX6. 

Thanks for confirming, lets keep both.

-- 
~Vinod

RFC: changing DMA slave configuration API

[David Brown]

On Mon, Jun 11, 2012 at 10:20:49AM +0530, Vinod Koul wrote:

> I think it is a good idea. And I would like to extend it even a little
> bit. Do we have any users of peripheral to peripheral slave dma?
> IIRC  that is not the case, or does anyone know of existence or plans
> for such a h/w?

We have hardware that supports this, and I suspect it will become more
common in our future chips.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

RFC: changing DMA slave configuration API

[Linus Walleij]

On Mon, Jun 11, 2012 at 6:50 AM, Vinod Koul <vinod.koul@linux.intel.com> wrote:

> Do we have any users of peripheral to peripheral slave dma?
> IIRC  that is not the case, or does anyone know of existence or plans
> for such a h/w?

The U300 (COH901318) device can do this.

The usecase is basically that the modem CPU sets up a PCM
stream on the high-speed serial link (called MSL) and the DMA
controller reads 16 bit words from this link and routes it directly
to a PCM sink on an I2S hardware.

Actually it runs in both directions during a voicecall, you can
probably see the gain in power and CPU unload this will gain
on a phone.

However this is all out-of-tree code and not that big deal as
it's a legacy platform.

Yours,
Linus Walleij