Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Tue, 2014-03-11 at 11:08 +0100, Maxime Ripard wrote:
[]
> > > + spin_lock_irq(>lock);
> > > + for (pchan_idx = 0; pchan_idx < NR_MAX_CHANNELS; pchan_idx++) {
> > > + pchan = >pchans[pchan_idx];
> > > +
> > > + if (pchan->vchan == NULL && !list_empty(>pending)) {
> >
> > !pchan->vchan && ...
>
> Ok.
>
> > And you may decrease indentation level here if you use negative
> > condition.
>
> Hmmm, I'm not following you here. What do you mean?
for () {
...
if (!(your current condition))
continue;
... (do something else with decreased indentation level)
}
> > > + vchan = list_first_entry(>pending,
> > > + struct sun6i_vchan, node);
> > > +
> > > + /* Remove from pending channels */
> > > + list_del_init(>node);
> > > + pchan_alloc |= BIT(pchan_idx);
> > > +
> > > + /* Mark this channel allocated */
> > > + pchan->vchan = vchan;
> > > + vchan->phy = pchan;
> > > + dev_dbg(sdev->slave.dev, "pchan %u: alloc vchan %p\n",
> > > + pchan->idx, >vc);
> > > + }
> > > + }
> > > + spin_unlock_irq(>lock);
> > > +
> > > + for (pchan_idx = 0; pchan_idx < NR_MAX_CHANNELS; pchan_idx++) {
> > > + if (pchan_alloc & BIT(pchan_idx)) {
> >
> > Ditto.
Same here.
[]
> > > +#ifdef DEBUG
> > > + dev_dbg(chan2dev(chan), "First: %pad\n", >p_lli);
> >
> > dev_dbg is aware of DEBUG. So, please, remove that #ifdef at all.
>
> Yep, but the line just below isn't.
>
> The ifdef here is not really to prevent the call to dev_dbg, but rather...
>
> > > + for (prev = txd->v_lli; prev != NULL; prev = prev->v_lli_next)
>
> ... this.
It doesn't matter since implementation of sun6i_dma_dump_lli is (and
actually should be) aware of DEBUG already, because it's dev_dbg based.
> >
> > You may remove '!= NULL' part.
>
> Ok.
>
> > > + sun6i_dma_dump_lli(vchan, prev);
> > > +#endif
--
Andy Shevchenko
Intel Finland Oy
-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
Hi,
On Tue, Mar 11, 2014 at 09:52:55AM +, Shevchenko, Andriy wrote:
> On Mon, 2014-03-10 at 15:41 +0100, Maxime Ripard wrote:
> > The Allwinner A31 has a 16 channels DMA controller that it shares with the
> > newer A23. Although sharing some similarities with the DMA controller of the
> > older Allwinner SoCs, it's significantly different, I don't expect it to be
> > possible to share the driver for these two.
> >
> > The A31 Controller is able to memory-to-memory or memory-to-device
> > transfers on
> > the 16 channels in parallel.
>
> Since it's going to be next cycle of review, I add few more nitpicks
> below.
>
> >
> > Signed-off-by: Maxime Ripard
> > ---
> > .../devicetree/bindings/dma/sun6i-dma.txt | 45 +
> > drivers/dma/Kconfig| 8 +
> > drivers/dma/Makefile | 1 +
> > drivers/dma/sun6i-dma.c| 986
> > +
> > 4 files changed, 1040 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > create mode 100644 drivers/dma/sun6i-dma.c
> >
> > diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > new file mode 100644
> > index ..5d7c86d52665
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > @@ -0,0 +1,45 @@
> > +Allwinner A31 DMA Controller
> > +
> > +This driver follows the generic DMA bindings defined in dma.txt.
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "allwinner,sun6i-a31-dma"
> > +- reg: Should contain the registers base address and length
> > +- interrupts: Should contain a reference to the interrupt used by
> > this device
> > +- clocks: Should contain a reference to the parent AHB clock
> > +- resets: Should contain a reference to the reset controller asserting
> > + this device in reset
> > +- #dma-cells : Should be 1, a single cell holding a line request number
> > +
> > +Example:
> > + dma: dma-controller@01c02000 {
> > + compatible = "allwinner,sun6i-a31-dma";
> > + reg = <0x01c02000 0x1000>;
> > + interrupts = <0 50 4>;
> > + clocks = <_gates 6>;
> > + resets = <_rst 6>;
> > + #dma-cells = <1>;
> > + };
> > +
> > +Clients:
> > +
> > +DMA clients connected to the A31 DMA controller must use the format
> > +described in the dma.txt file, using a two-cell specifier for each
> > +channel: a phandle plus one integer cells.
> > +The two cells in order are:
> > +
> > +1. A phandle pointing to the DMA controller.
> > +2. The port ID as specified in the datasheet
> > +
> > +Example:
> > +spi2: spi@01c6a000 {
> > + compatible = "allwinner,sun6i-a31-spi";
> > + reg = <0x01c6a000 0x1000>;
> > + interrupts = <0 67 4>;
> > + clocks = <_gates 22>, <_clk>;
> > + clock-names = "ahb", "mod";
> > + dmas = < 25>, < 25>;
> > + dma-names = "rx", "tx";
> > + resets = <_rst 22>;
> > +};
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index 605b016bcea4..7923697eaa2e 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -351,6 +351,14 @@ config MOXART_DMA
> > help
> > Enable support for the MOXA ART SoC DMA controller.
> >
> > +config DMA_SUN6I
> > + tristate "Allwinner A31 SoCs DMA support"
> > + depends on ARCH_SUNXI
> > + select DMA_ENGINE
> > + select DMA_VIRTUAL_CHANNELS
> > + help
> > + Support for the DMA engine for Allwinner A31 SoCs.
> > +
> > config DMA_ENGINE
> > bool
> >
> > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> > index a029d0f4a1be..18cdbad1927c 100644
> > --- a/drivers/dma/Makefile
> > +++ b/drivers/dma/Makefile
> > @@ -44,3 +44,4 @@ obj-$(CONFIG_DMA_JZ4740) += dma-jz4740.o
> > obj-$(CONFIG_TI_CPPI41) += cppi41.o
> > obj-$(CONFIG_K3_DMA) += k3dma.o
> > obj-$(CONFIG_MOXART_DMA) += moxart-dma.o
> > +obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > new file mode 100644
> > index ..a11040633b30
> > --- /dev/null
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -0,0 +1,986 @@
> > +/*
> > + * Copyright (C) 2013-2014 Allwinner Tech Co., Ltd
> > + * Author: Sugar
> > + *
> > + * Copyright (C) 2014 Maxime Ripard
> > + * Maxime Ripard
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "virt-dma.h"
> > +
> > +/*
> > + * There's 16 physical channels that can work in parallel.
> > + *
> > + * However we have
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Mon, 2014-03-10 at 15:41 +0100, Maxime Ripard wrote:
> The Allwinner A31 has a 16 channels DMA controller that it shares with the
> newer A23. Although sharing some similarities with the DMA controller of the
> older Allwinner SoCs, it's significantly different, I don't expect it to be
> possible to share the driver for these two.
>
> The A31 Controller is able to memory-to-memory or memory-to-device transfers
> on
> the 16 channels in parallel.
Since it's going to be next cycle of review, I add few more nitpicks
below.
>
> Signed-off-by: Maxime Ripard
> ---
> .../devicetree/bindings/dma/sun6i-dma.txt | 45 +
> drivers/dma/Kconfig| 8 +
> drivers/dma/Makefile | 1 +
> drivers/dma/sun6i-dma.c| 986
> +
> 4 files changed, 1040 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/sun6i-dma.txt
> create mode 100644 drivers/dma/sun6i-dma.c
>
> diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> new file mode 100644
> index ..5d7c86d52665
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> @@ -0,0 +1,45 @@
> +Allwinner A31 DMA Controller
> +
> +This driver follows the generic DMA bindings defined in dma.txt.
> +
> +Required properties:
> +
> +- compatible:Must be "allwinner,sun6i-a31-dma"
> +- reg: Should contain the registers base address and length
> +- interrupts:Should contain a reference to the interrupt used by
> this device
> +- clocks:Should contain a reference to the parent AHB clock
> +- resets:Should contain a reference to the reset controller asserting
> + this device in reset
> +- #dma-cells : Should be 1, a single cell holding a line request number
> +
> +Example:
> + dma: dma-controller@01c02000 {
> + compatible = "allwinner,sun6i-a31-dma";
> + reg = <0x01c02000 0x1000>;
> + interrupts = <0 50 4>;
> + clocks = <_gates 6>;
> + resets = <_rst 6>;
> + #dma-cells = <1>;
> + };
> +
> +Clients:
> +
> +DMA clients connected to the A31 DMA controller must use the format
> +described in the dma.txt file, using a two-cell specifier for each
> +channel: a phandle plus one integer cells.
> +The two cells in order are:
> +
> +1. A phandle pointing to the DMA controller.
> +2. The port ID as specified in the datasheet
> +
> +Example:
> +spi2: spi@01c6a000 {
> + compatible = "allwinner,sun6i-a31-spi";
> + reg = <0x01c6a000 0x1000>;
> + interrupts = <0 67 4>;
> + clocks = <_gates 22>, <_clk>;
> + clock-names = "ahb", "mod";
> + dmas = < 25>, < 25>;
> + dma-names = "rx", "tx";
> + resets = <_rst 22>;
> +};
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 605b016bcea4..7923697eaa2e 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -351,6 +351,14 @@ config MOXART_DMA
> help
> Enable support for the MOXA ART SoC DMA controller.
>
> +config DMA_SUN6I
> + tristate "Allwinner A31 SoCs DMA support"
> + depends on ARCH_SUNXI
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Support for the DMA engine for Allwinner A31 SoCs.
> +
> config DMA_ENGINE
> bool
>
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index a029d0f4a1be..18cdbad1927c 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -44,3 +44,4 @@ obj-$(CONFIG_DMA_JZ4740) += dma-jz4740.o
> obj-$(CONFIG_TI_CPPI41) += cppi41.o
> obj-$(CONFIG_K3_DMA) += k3dma.o
> obj-$(CONFIG_MOXART_DMA) += moxart-dma.o
> +obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> new file mode 100644
> index ..a11040633b30
> --- /dev/null
> +++ b/drivers/dma/sun6i-dma.c
> @@ -0,0 +1,986 @@
> +/*
> + * Copyright (C) 2013-2014 Allwinner Tech Co., Ltd
> + * Author: Sugar
> + *
> + * Copyright (C) 2014 Maxime Ripard
> + * Maxime Ripard
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "virt-dma.h"
> +
> +/*
> + * There's 16 physical channels that can work in parallel.
> + *
> + * However we have 30 different endpoints for our requests.
> + *
> + * Since the channels are able to handle only an unidirectional
> + * transfer, we need to allocate more virtual channels so that
> + * everyone can grab one channel.
> + *
> + * Some devices can't work in both direction (mostly because it
> + *
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Mon, Mar 10, 2014 at 06:57:00PM +0100, Arnd Bergmann wrote: > On Monday 10 March 2014 17:51:56 Maxime Ripard wrote: > > > > > > Neither "pll6" nor "ahb1_mux" are listed in the DT binding. Also, why > > > is it the driver's business to set the parent? > > > > Those are global clocks, so it's not really part pof the driver > > binding itself. But I can add them. > > No better don't then. Can you change the clk_get() call to pass > NULL as the device pointer to clarify this in the source though? Ok. > > About the reparenting itself, other devices are actually fine having > > any parent they want, only the DMA is picky about it (at least, from > > what we know), so it made sense to me to put it into the driver > > itself. Where would you put it? > > Maybe Mike Turquette has an idea. We have in the past discussed > about cases where you want the default clock setting to be part > of the clock provider in some property. Could that work here? You mean in the DT? I guess it would yes. -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Mon, Mar 10, 2014 at 06:57:00PM +0100, Arnd Bergmann wrote: On Monday 10 March 2014 17:51:56 Maxime Ripard wrote: Neither pll6 nor ahb1_mux are listed in the DT binding. Also, why is it the driver's business to set the parent? Those are global clocks, so it's not really part pof the driver binding itself. But I can add them. No better don't then. Can you change the clk_get() call to pass NULL as the device pointer to clarify this in the source though? Ok. About the reparenting itself, other devices are actually fine having any parent they want, only the DMA is picky about it (at least, from what we know), so it made sense to me to put it into the driver itself. Where would you put it? Maybe Mike Turquette has an idea. We have in the past discussed about cases where you want the default clock setting to be part of the clock provider in some property. Could that work here? You mean in the DT? I guess it would yes. -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Mon, 2014-03-10 at 15:41 +0100, Maxime Ripard wrote:
The Allwinner A31 has a 16 channels DMA controller that it shares with the
newer A23. Although sharing some similarities with the DMA controller of the
older Allwinner SoCs, it's significantly different, I don't expect it to be
possible to share the driver for these two.
The A31 Controller is able to memory-to-memory or memory-to-device transfers
on
the 16 channels in parallel.
Since it's going to be next cycle of review, I add few more nitpicks
below.
Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
---
.../devicetree/bindings/dma/sun6i-dma.txt | 45 +
drivers/dma/Kconfig| 8 +
drivers/dma/Makefile | 1 +
drivers/dma/sun6i-dma.c| 986
+
4 files changed, 1040 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/sun6i-dma.txt
create mode 100644 drivers/dma/sun6i-dma.c
diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
new file mode 100644
index ..5d7c86d52665
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
@@ -0,0 +1,45 @@
+Allwinner A31 DMA Controller
+
+This driver follows the generic DMA bindings defined in dma.txt.
+
+Required properties:
+
+- compatible:Must be allwinner,sun6i-a31-dma
+- reg: Should contain the registers base address and length
+- interrupts:Should contain a reference to the interrupt used by
this device
+- clocks:Should contain a reference to the parent AHB clock
+- resets:Should contain a reference to the reset controller asserting
+ this device in reset
+- #dma-cells : Should be 1, a single cell holding a line request number
+
+Example:
+ dma: dma-controller@01c02000 {
+ compatible = allwinner,sun6i-a31-dma;
+ reg = 0x01c02000 0x1000;
+ interrupts = 0 50 4;
+ clocks = ahb1_gates 6;
+ resets = ahb1_rst 6;
+ #dma-cells = 1;
+ };
+
+Clients:
+
+DMA clients connected to the A31 DMA controller must use the format
+described in the dma.txt file, using a two-cell specifier for each
+channel: a phandle plus one integer cells.
+The two cells in order are:
+
+1. A phandle pointing to the DMA controller.
+2. The port ID as specified in the datasheet
+
+Example:
+spi2: spi@01c6a000 {
+ compatible = allwinner,sun6i-a31-spi;
+ reg = 0x01c6a000 0x1000;
+ interrupts = 0 67 4;
+ clocks = ahb1_gates 22, spi2_clk;
+ clock-names = ahb, mod;
+ dmas = dma 25, dma 25;
+ dma-names = rx, tx;
+ resets = ahb1_rst 22;
+};
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 605b016bcea4..7923697eaa2e 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -351,6 +351,14 @@ config MOXART_DMA
help
Enable support for the MOXA ART SoC DMA controller.
+config DMA_SUN6I
+ tristate Allwinner A31 SoCs DMA support
+ depends on ARCH_SUNXI
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support for the DMA engine for Allwinner A31 SoCs.
+
config DMA_ENGINE
bool
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index a029d0f4a1be..18cdbad1927c 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -44,3 +44,4 @@ obj-$(CONFIG_DMA_JZ4740) += dma-jz4740.o
obj-$(CONFIG_TI_CPPI41) += cppi41.o
obj-$(CONFIG_K3_DMA) += k3dma.o
obj-$(CONFIG_MOXART_DMA) += moxart-dma.o
+obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
new file mode 100644
index ..a11040633b30
--- /dev/null
+++ b/drivers/dma/sun6i-dma.c
@@ -0,0 +1,986 @@
+/*
+ * Copyright (C) 2013-2014 Allwinner Tech Co., Ltd
+ * Author: Sugar sh...@allwinnertech.com
+ *
+ * Copyright (C) 2014 Maxime Ripard
+ * Maxime Ripard maxime.rip...@free-electrons.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include linux/clk.h
+#include linux/delay.h
+#include linux/dmaengine.h
+#include linux/dmapool.h
+#include linux/interrupt.h
+#include linux/module.h
+#include linux/of_dma.h
+#include linux/platform_device.h
+#include linux/reset.h
+#include linux/slab.h
+#include linux/types.h
+
+#include virt-dma.h
+
+/*
+ * There's 16 physical channels that can work in parallel.
+ *
+ * However we have 30 different endpoints for our requests.
+ *
+ * Since the channels are able to handle only an unidirectional
+ * transfer, we need to allocate more virtual channels so that
+ * everyone can grab
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
Hi,
On Tue, Mar 11, 2014 at 09:52:55AM +, Shevchenko, Andriy wrote:
On Mon, 2014-03-10 at 15:41 +0100, Maxime Ripard wrote:
The Allwinner A31 has a 16 channels DMA controller that it shares with the
newer A23. Although sharing some similarities with the DMA controller of the
older Allwinner SoCs, it's significantly different, I don't expect it to be
possible to share the driver for these two.
The A31 Controller is able to memory-to-memory or memory-to-device
transfers on
the 16 channels in parallel.
Since it's going to be next cycle of review, I add few more nitpicks
below.
Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
---
.../devicetree/bindings/dma/sun6i-dma.txt | 45 +
drivers/dma/Kconfig| 8 +
drivers/dma/Makefile | 1 +
drivers/dma/sun6i-dma.c| 986
+
4 files changed, 1040 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/sun6i-dma.txt
create mode 100644 drivers/dma/sun6i-dma.c
diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
new file mode 100644
index ..5d7c86d52665
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
@@ -0,0 +1,45 @@
+Allwinner A31 DMA Controller
+
+This driver follows the generic DMA bindings defined in dma.txt.
+
+Required properties:
+
+- compatible: Must be allwinner,sun6i-a31-dma
+- reg: Should contain the registers base address and length
+- interrupts: Should contain a reference to the interrupt used by
this device
+- clocks: Should contain a reference to the parent AHB clock
+- resets: Should contain a reference to the reset controller asserting
+ this device in reset
+- #dma-cells : Should be 1, a single cell holding a line request number
+
+Example:
+ dma: dma-controller@01c02000 {
+ compatible = allwinner,sun6i-a31-dma;
+ reg = 0x01c02000 0x1000;
+ interrupts = 0 50 4;
+ clocks = ahb1_gates 6;
+ resets = ahb1_rst 6;
+ #dma-cells = 1;
+ };
+
+Clients:
+
+DMA clients connected to the A31 DMA controller must use the format
+described in the dma.txt file, using a two-cell specifier for each
+channel: a phandle plus one integer cells.
+The two cells in order are:
+
+1. A phandle pointing to the DMA controller.
+2. The port ID as specified in the datasheet
+
+Example:
+spi2: spi@01c6a000 {
+ compatible = allwinner,sun6i-a31-spi;
+ reg = 0x01c6a000 0x1000;
+ interrupts = 0 67 4;
+ clocks = ahb1_gates 22, spi2_clk;
+ clock-names = ahb, mod;
+ dmas = dma 25, dma 25;
+ dma-names = rx, tx;
+ resets = ahb1_rst 22;
+};
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 605b016bcea4..7923697eaa2e 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -351,6 +351,14 @@ config MOXART_DMA
help
Enable support for the MOXA ART SoC DMA controller.
+config DMA_SUN6I
+ tristate Allwinner A31 SoCs DMA support
+ depends on ARCH_SUNXI
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support for the DMA engine for Allwinner A31 SoCs.
+
config DMA_ENGINE
bool
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index a029d0f4a1be..18cdbad1927c 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -44,3 +44,4 @@ obj-$(CONFIG_DMA_JZ4740) += dma-jz4740.o
obj-$(CONFIG_TI_CPPI41) += cppi41.o
obj-$(CONFIG_K3_DMA) += k3dma.o
obj-$(CONFIG_MOXART_DMA) += moxart-dma.o
+obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
new file mode 100644
index ..a11040633b30
--- /dev/null
+++ b/drivers/dma/sun6i-dma.c
@@ -0,0 +1,986 @@
+/*
+ * Copyright (C) 2013-2014 Allwinner Tech Co., Ltd
+ * Author: Sugar sh...@allwinnertech.com
+ *
+ * Copyright (C) 2014 Maxime Ripard
+ * Maxime Ripard maxime.rip...@free-electrons.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include linux/clk.h
+#include linux/delay.h
+#include linux/dmaengine.h
+#include linux/dmapool.h
+#include linux/interrupt.h
+#include linux/module.h
+#include linux/of_dma.h
+#include linux/platform_device.h
+#include linux/reset.h
+#include linux/slab.h
+#include linux/types.h
+
+#include virt-dma.h
+
+/*
+ * There's 16 physical channels that can work in parallel.
+ *
+ * However we have 30 different endpoints for our requests.
+ *
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Tue, 2014-03-11 at 11:08 +0100, Maxime Ripard wrote:
[]
+ spin_lock_irq(sdev-lock);
+ for (pchan_idx = 0; pchan_idx NR_MAX_CHANNELS; pchan_idx++) {
+ pchan = sdev-pchans[pchan_idx];
+
+ if (pchan-vchan == NULL !list_empty(sdev-pending)) {
!pchan-vchan ...
Ok.
And you may decrease indentation level here if you use negative
condition.
Hmmm, I'm not following you here. What do you mean?
for () {
...
if (!(your current condition))
continue;
... (do something else with decreased indentation level)
}
+ vchan = list_first_entry(sdev-pending,
+ struct sun6i_vchan, node);
+
+ /* Remove from pending channels */
+ list_del_init(vchan-node);
+ pchan_alloc |= BIT(pchan_idx);
+
+ /* Mark this channel allocated */
+ pchan-vchan = vchan;
+ vchan-phy = pchan;
+ dev_dbg(sdev-slave.dev, pchan %u: alloc vchan %p\n,
+ pchan-idx, vchan-vc);
+ }
+ }
+ spin_unlock_irq(sdev-lock);
+
+ for (pchan_idx = 0; pchan_idx NR_MAX_CHANNELS; pchan_idx++) {
+ if (pchan_alloc BIT(pchan_idx)) {
Ditto.
Same here.
[]
+#ifdef DEBUG
+ dev_dbg(chan2dev(chan), First: %pad\n, txd-p_lli);
dev_dbg is aware of DEBUG. So, please, remove that #ifdef at all.
Yep, but the line just below isn't.
The ifdef here is not really to prevent the call to dev_dbg, but rather...
+ for (prev = txd-v_lli; prev != NULL; prev = prev-v_lli_next)
... this.
It doesn't matter since implementation of sun6i_dma_dump_lli is (and
actually should be) aware of DEBUG already, because it's dev_dbg based.
You may remove '!= NULL' part.
Ok.
+ sun6i_dma_dump_lli(vchan, prev);
+#endif
--
Andy Shevchenko andriy.shevche...@intel.com
Intel Finland Oy
-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Monday 10 March 2014 17:51:56 Maxime Ripard wrote: > > > > Neither "pll6" nor "ahb1_mux" are listed in the DT binding. Also, why > > is it the driver's business to set the parent? > > Those are global clocks, so it's not really part pof the driver > binding itself. But I can add them. No better don't then. Can you change the clk_get() call to pass NULL as the device pointer to clarify this in the source though? > About the reparenting itself, other devices are actually fine having > any parent they want, only the DMA is picky about it (at least, from > what we know), so it made sense to me to put it into the driver > itself. Where would you put it? Maybe Mike Turquette has an idea. We have in the past discussed about cases where you want the default clock setting to be part of the clock provider in some property. Could that work here? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
Hi Arnd,
On Mon, Mar 10, 2014 at 04:34:04PM +0100, Arnd Bergmann wrote:
> On Monday 10 March 2014 15:41:51 Maxime Ripard wrote:
>
> > +/*
> > + * Hardware representation of the LLI
> > + *
> > + * The hardware will be fed the physical address of this structure,
> > + * and read its content in order to start the transfer.
> > + */
> > +struct sun6i_dma_lli {
> > + u32 cfg;
> > + u32 src;
> > + u32 dst;
> > + u32 len;
> > + u32 para;
> > + u32 p_lli_next;
> > + struct sun6i_dma_lli*v_lli_next;
> > +} __packed;
>
> It looks strange to have a pointer in a hardware structure, since
> the pointer doesn't make sense to the device.
Actually, it's not used at all by the device itself. It's used only by
the driver to be able to follow that list whenever we need to
free/dump it.
> Also, the '__packed' attribute seems strange. Are you sure
> you want to reduce the alignment from 4 bytes to 1 byte?
What I wanted to make sure is that the structure is actually what I
declared above in memory, but it's true that it's completely useless
here.
> > +static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> > +{
> > + struct sun6i_dma_dev *sdev = dev_id;
> > + struct sun6i_vchan *vchan;
> > + struct sun6i_pchan *pchan;
> > + int i, j, ret = IRQ_NONE;
> > + u32 status;
> > +
> > + for (i = 0; i < 2; i++) {
> > + status = readl(sdev->base + DMA_IRQ_STAT(i));
> > + if (!status)
> > + continue;
> > +
> > + dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
> > + i ? "high" : "low", status);
> > +
> > + writel(status, sdev->base + DMA_IRQ_STAT(i));
> > +
> > + for (j = 0; (j < 8) && status; j++) {
> > + if (status & DMA_IRQ_QUEUE) {
> > + pchan = sdev->pchans + j;
> > + vchan = pchan->vchan;
> > +
> > + if (vchan) {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(>vc.lock,
> > + flags);
>
> Interrupts are already disabled here.
True.
> > + sdc->clk = devm_clk_get(>dev, NULL);
> > + if (IS_ERR(sdc->clk)) {
> > + dev_err(>dev, "No clock specified\n");
> > + return PTR_ERR(sdc->clk);
> > + }
> > +
> > + mux = devm_clk_get(>dev, "ahb1_mux");
> > + if (IS_ERR(mux)) {
> > + dev_err(>dev, "Couldn't get AHB1 Mux\n");
> > + return PTR_ERR(mux);
> > + }
> > +
> > + pll6 = devm_clk_get(>dev, "pll6");
> > + if (IS_ERR(pll6)) {
> > + dev_err(>dev, "Couldn't get PLL6\n");
> > + return PTR_ERR(pll6);
> > + }
> > +
> > + ret = clk_set_parent(mux, pll6);
> > + if (ret) {
> > + dev_err(>dev, "Couldn't reparent AHB1 on PLL6\n");
> > + return ret;
> > + }
>
> Neither "pll6" nor "ahb1_mux" are listed in the DT binding. Also, why
> is it the driver's business to set the parent?
Those are global clocks, so it's not really part pof the driver
binding itself. But I can add them.
About the reparenting itself, other devices are actually fine having
any parent they want, only the DMA is picky about it (at least, from
what we know), so it made sense to me to put it into the driver
itself. Where would you put it?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Monday 10 March 2014 15:41:51 Maxime Ripard wrote:
> +/*
> + * Hardware representation of the LLI
> + *
> + * The hardware will be fed the physical address of this structure,
> + * and read its content in order to start the transfer.
> + */
> +struct sun6i_dma_lli {
> + u32 cfg;
> + u32 src;
> + u32 dst;
> + u32 len;
> + u32 para;
> + u32 p_lli_next;
> + struct sun6i_dma_lli*v_lli_next;
> +} __packed;
It looks strange to have a pointer in a hardware structure, since
the pointer doesn't make sense to the device.
Also, the '__packed' attribute seems strange. Are you sure
you want to reduce the alignment from 4 bytes to 1 byte?
> +static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> +{
> + struct sun6i_dma_dev *sdev = dev_id;
> + struct sun6i_vchan *vchan;
> + struct sun6i_pchan *pchan;
> + int i, j, ret = IRQ_NONE;
> + u32 status;
> +
> + for (i = 0; i < 2; i++) {
> + status = readl(sdev->base + DMA_IRQ_STAT(i));
> + if (!status)
> + continue;
> +
> + dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
> + i ? "high" : "low", status);
> +
> + writel(status, sdev->base + DMA_IRQ_STAT(i));
> +
> + for (j = 0; (j < 8) && status; j++) {
> + if (status & DMA_IRQ_QUEUE) {
> + pchan = sdev->pchans + j;
> + vchan = pchan->vchan;
> +
> + if (vchan) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(>vc.lock,
> + flags);
Interrupts are already disabled here.
> + sdc->clk = devm_clk_get(>dev, NULL);
> + if (IS_ERR(sdc->clk)) {
> + dev_err(>dev, "No clock specified\n");
> + return PTR_ERR(sdc->clk);
> + }
> +
> + mux = devm_clk_get(>dev, "ahb1_mux");
> + if (IS_ERR(mux)) {
> + dev_err(>dev, "Couldn't get AHB1 Mux\n");
> + return PTR_ERR(mux);
> + }
> +
> + pll6 = devm_clk_get(>dev, "pll6");
> + if (IS_ERR(pll6)) {
> + dev_err(>dev, "Couldn't get PLL6\n");
> + return PTR_ERR(pll6);
> + }
> +
> + ret = clk_set_parent(mux, pll6);
> + if (ret) {
> + dev_err(>dev, "Couldn't reparent AHB1 on PLL6\n");
> + return ret;
> + }
Neither "pll6" nor "ahb1_mux" are listed in the DT binding. Also, why
is it the driver's business to set the parent?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Monday 10 March 2014 15:41:51 Maxime Ripard wrote:
+/*
+ * Hardware representation of the LLI
+ *
+ * The hardware will be fed the physical address of this structure,
+ * and read its content in order to start the transfer.
+ */
+struct sun6i_dma_lli {
+ u32 cfg;
+ u32 src;
+ u32 dst;
+ u32 len;
+ u32 para;
+ u32 p_lli_next;
+ struct sun6i_dma_lli*v_lli_next;
+} __packed;
It looks strange to have a pointer in a hardware structure, since
the pointer doesn't make sense to the device.
Also, the '__packed' attribute seems strange. Are you sure
you want to reduce the alignment from 4 bytes to 1 byte?
+static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
+{
+ struct sun6i_dma_dev *sdev = dev_id;
+ struct sun6i_vchan *vchan;
+ struct sun6i_pchan *pchan;
+ int i, j, ret = IRQ_NONE;
+ u32 status;
+
+ for (i = 0; i 2; i++) {
+ status = readl(sdev-base + DMA_IRQ_STAT(i));
+ if (!status)
+ continue;
+
+ dev_dbg(sdev-slave.dev, DMA irq status %s: 0x%x\n,
+ i ? high : low, status);
+
+ writel(status, sdev-base + DMA_IRQ_STAT(i));
+
+ for (j = 0; (j 8) status; j++) {
+ if (status DMA_IRQ_QUEUE) {
+ pchan = sdev-pchans + j;
+ vchan = pchan-vchan;
+
+ if (vchan) {
+ unsigned long flags;
+
+ spin_lock_irqsave(vchan-vc.lock,
+ flags);
Interrupts are already disabled here.
+ sdc-clk = devm_clk_get(pdev-dev, NULL);
+ if (IS_ERR(sdc-clk)) {
+ dev_err(pdev-dev, No clock specified\n);
+ return PTR_ERR(sdc-clk);
+ }
+
+ mux = devm_clk_get(pdev-dev, ahb1_mux);
+ if (IS_ERR(mux)) {
+ dev_err(pdev-dev, Couldn't get AHB1 Mux\n);
+ return PTR_ERR(mux);
+ }
+
+ pll6 = devm_clk_get(pdev-dev, pll6);
+ if (IS_ERR(pll6)) {
+ dev_err(pdev-dev, Couldn't get PLL6\n);
+ return PTR_ERR(pll6);
+ }
+
+ ret = clk_set_parent(mux, pll6);
+ if (ret) {
+ dev_err(pdev-dev, Couldn't reparent AHB1 on PLL6\n);
+ return ret;
+ }
Neither pll6 nor ahb1_mux are listed in the DT binding. Also, why
is it the driver's business to set the parent?
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
Hi Arnd,
On Mon, Mar 10, 2014 at 04:34:04PM +0100, Arnd Bergmann wrote:
On Monday 10 March 2014 15:41:51 Maxime Ripard wrote:
+/*
+ * Hardware representation of the LLI
+ *
+ * The hardware will be fed the physical address of this structure,
+ * and read its content in order to start the transfer.
+ */
+struct sun6i_dma_lli {
+ u32 cfg;
+ u32 src;
+ u32 dst;
+ u32 len;
+ u32 para;
+ u32 p_lli_next;
+ struct sun6i_dma_lli*v_lli_next;
+} __packed;
It looks strange to have a pointer in a hardware structure, since
the pointer doesn't make sense to the device.
Actually, it's not used at all by the device itself. It's used only by
the driver to be able to follow that list whenever we need to
free/dump it.
Also, the '__packed' attribute seems strange. Are you sure
you want to reduce the alignment from 4 bytes to 1 byte?
What I wanted to make sure is that the structure is actually what I
declared above in memory, but it's true that it's completely useless
here.
+static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
+{
+ struct sun6i_dma_dev *sdev = dev_id;
+ struct sun6i_vchan *vchan;
+ struct sun6i_pchan *pchan;
+ int i, j, ret = IRQ_NONE;
+ u32 status;
+
+ for (i = 0; i 2; i++) {
+ status = readl(sdev-base + DMA_IRQ_STAT(i));
+ if (!status)
+ continue;
+
+ dev_dbg(sdev-slave.dev, DMA irq status %s: 0x%x\n,
+ i ? high : low, status);
+
+ writel(status, sdev-base + DMA_IRQ_STAT(i));
+
+ for (j = 0; (j 8) status; j++) {
+ if (status DMA_IRQ_QUEUE) {
+ pchan = sdev-pchans + j;
+ vchan = pchan-vchan;
+
+ if (vchan) {
+ unsigned long flags;
+
+ spin_lock_irqsave(vchan-vc.lock,
+ flags);
Interrupts are already disabled here.
True.
+ sdc-clk = devm_clk_get(pdev-dev, NULL);
+ if (IS_ERR(sdc-clk)) {
+ dev_err(pdev-dev, No clock specified\n);
+ return PTR_ERR(sdc-clk);
+ }
+
+ mux = devm_clk_get(pdev-dev, ahb1_mux);
+ if (IS_ERR(mux)) {
+ dev_err(pdev-dev, Couldn't get AHB1 Mux\n);
+ return PTR_ERR(mux);
+ }
+
+ pll6 = devm_clk_get(pdev-dev, pll6);
+ if (IS_ERR(pll6)) {
+ dev_err(pdev-dev, Couldn't get PLL6\n);
+ return PTR_ERR(pll6);
+ }
+
+ ret = clk_set_parent(mux, pll6);
+ if (ret) {
+ dev_err(pdev-dev, Couldn't reparent AHB1 on PLL6\n);
+ return ret;
+ }
Neither pll6 nor ahb1_mux are listed in the DT binding. Also, why
is it the driver's business to set the parent?
Those are global clocks, so it's not really part pof the driver
binding itself. But I can add them.
About the reparenting itself, other devices are actually fine having
any parent they want, only the DMA is picky about it (at least, from
what we know), so it made sense to me to put it into the driver
itself. Where would you put it?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
Re: [PATCH v4 6/7] DMA: sun6i: Add driver for the Allwinner A31 DMA controller
On Monday 10 March 2014 17:51:56 Maxime Ripard wrote: Neither pll6 nor ahb1_mux are listed in the DT binding. Also, why is it the driver's business to set the parent? Those are global clocks, so it's not really part pof the driver binding itself. But I can add them. No better don't then. Can you change the clk_get() call to pass NULL as the device pointer to clarify this in the source though? About the reparenting itself, other devices are actually fine having any parent they want, only the DMA is picky about it (at least, from what we know), so it made sense to me to put it into the driver itself. Where would you put it? Maybe Mike Turquette has an idea. We have in the past discussed about cases where you want the default clock setting to be part of the clock provider in some property. Could that work here? Arnd -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
