Hello.
On 05-07-2013 20:12, Sebastian Andrzej Siewior wrote:
This is a first shot of the cppi41 DMA driver.
Where have you been when I submitted my drivers back in 2009? :-)
It is currently used by
a new musb dma-engine implementation. I have to test it somehow.
The state of the cpp41 (and the using dmaengine) is in very early stage.
I was able to send TX packets over DMA and enumerate an USB masstorage
device.
Things that need to be taken care of:
- RX path
- cancel of transfers
- dynamic descriptor allocation
- re-think the current device tree nodes.
Currently a node looks like:
&cppi41dma X Y Z Q
that means:
- X: dma channel
- Y: RX/TX
- Z: start queue
- Q: complete queue
Each value number is hardwired with the USB endpoint it is using. The
DMA channels are hardwired with USB endpoints and the start/complete
is hardwired with the DMA channel.
I add each DMA channel twice: once for RX the other for TX (that is why
I need the direction (Y) uppon lookup).
The RX/TX channel can be used simultaneously i.e. program & start RX,
program & start TX and TX can complete before RX.
Currently I think that it would be best to remove the queue logic from
the device and put it into the driver.
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
arch/arm/boot/dts/am33xx.dtsi | 50 +++
drivers/dma/Kconfig | 9 +
drivers/dma/Makefile | 1 +
drivers/dma/cppi41.c | 777 +++++++++++++++++++++++++++++++++++++++++
drivers/usb/musb/Kconfig | 4 +
drivers/usb/musb/Makefile | 1 +
drivers/usb/musb/musb_dma.h | 2 +-
drivers/usb/musb/musb_dmaeng.c | 294 ++++++++++++++++
MUSB DMA engine support can't be generic unfortunately. There are
some non-generic DMA registers in each MUSB implementation that used
CPPI 4.1.
8 files changed, 1137 insertions(+), 1 deletion(-)
create mode 100644 drivers/dma/cppi41.c
create mode 100644 drivers/usb/musb/musb_dmaeng.c
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index bb2298c..fc29b54 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -349,6 +349,18 @@
status = "disabled";
};
+ cppi41dma: dma@07402000 {
@47402000? maybe?
+ compatible = "ti,am3359-cppi41";
+ reg = <0x47400000 0x1000 /* usbss */
USB register are not a part of CPPI 4.1 DMA. They are not generic
and are different on e.g. DA8xx/OMAP-L1x. Besides this range is
conflicting with the next node.
+ 0x47402000 0x1000 /* controller */
+ 0x47403000 0x1000 /* scheduler */
+ 0x47404000 0x4000>; /* queue manager */
+ interrupts = <17>;
+ #dma-cells = <4>;
+ #dma-channels = <30>;
+ #dma-requests = <256>;
+ };
+
musb: usb@47400000 {
compatible = "ti,musb-am33xx";
reg = <0x47400000 0x1000>;
[...]
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 3215a3c..c19a593 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -305,6 +305,15 @@ config DMA_OMAP
select DMA_ENGINE
select DMA_VIRTUAL_CHANNELS
+config TI_CPPI41
+ tristate "AM33xx CPPI41 DMA support"
It shouldn't be specific to AM33xx.
+ depends on ARCH_OMAP
And shouldn't depend on ARCH_OMAP only, also on ARCH_DAVINCI_DA8XX.
[...]
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
new file mode 100644
index 0000000..80dcb56
--- /dev/null
+++ b/drivers/dma/cppi41.c
@@ -0,0 +1,777 @@
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/of_dma.h>
+#include <linux/of_irq.h>
+#include <linux/dmapool.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include "dmaengine.h"
+
+#define DESC_TYPE 27
+#define DESC_TYPE_HOST 0x10
+#define DESC_TYPE_TEARD 0x13
+
+#define TD_DESC_TX_RX 16
+#define TD_DESC_DMA_NUM 10
+
+/* USB SS */
+#define USBSS_IRQ_STATUS (0x28)
+#define USBSS_IRQ_ENABLER (0x2c)
+#define USBSS_IRQ_CLEARR (0x30)
These shouldn't be here. Why enclose them in () BTW?
+
+#define USBSS_IRQ_RX1 (1 << 11)
+#define USBSS_IRQ_TX1 (1 << 10)
+#define USBSS_IRQ_RX0 (1 << 9)
+#define USBSS_IRQ_TX0 (1 << 8)
+#define USBSS_IRQ_PD_COMP (1 << 2)
+
+#define USBSS_IRQ_RXTX1 (USBSS_IRQ_RX1 | USBSS_IRQ_TX1)
+#define USBSS_IRQ_RXTX0 (USBSS_IRQ_RX0 | USBSS_IRQ_TX0)
+#define USBSS_IRQ_RXTX01 (USBSS_IRQ_RXTX0 | USBSS_IRQ_RXTX1)
+
Neither these shouldn't be here.
+/* Queue manager */
+/* 4 KiB of memory for descriptors, 2 for each endpoint */
Endpoints shouldn't be mentioned. CPPI 4.1 is universal DMA engine,
not USB specific.
+struct cppi41_dd {
+ struct dma_device ddev;
+
+ void *qmgr_scratch;
+ dma_addr_t scratch_phys;
+
+ struct cppi41_desc *cd;
+ dma_addr_t descs_phys;
+ struct cppi41_channel *chan_busy[ALLOC_DECS_NUM];
+
+ void __iomem *usbss_mem;
Shouldn't be here.
+ void __iomem *ctrl_mem;
+ void __iomem *sched_mem;
+ void __iomem *qmgr_mem;
+ unsigned int irq;
Shouldn't be here either.
+static struct cppi41_channel *desc_to_chan(struct cppi41_dd *cdd, u32 desc)
+{
+ struct cppi41_channel *c;
+ u32 descs_size;
+ u32 desc_num;
+
+ descs_size = sizeof(struct cppi41_desc) * ALLOC_DECS_NUM;
+
+ if (!((desc >= cdd->descs_phys) &&
+ (desc < (cdd->descs_phys + descs_size)))) {
+ return NULL;
+ }
checkpatch.pl would complain about single statement in {}.
+static void cppi_writel(u32 val, void *__iomem *mem)
+{
+ writel(val, mem);
+}
+
+static u32 cppi_readl(void *__iomem *mem)
+{
+ u32 val;
+ val = readl(mem);
+ return val;
+}
I don't see much sense in these functions. Besides, they should
probably be using __raw_{read|write}().
+static irqreturn_t cppi41_irq(int irq, void *data)
+{
+ struct cppi41_dd *cdd = data;
+ struct cppi41_channel *c;
+ u32 status;
+ int i;
+
+ status = cppi_readl(cdd->usbss_mem + USBSS_IRQ_STATUS);
+ if (!(status & USBSS_IRQ_PD_COMP))
+ return IRQ_NONE;
No, this can't be here.
+static u32 get_host_pd0(u32 length)
+{
+ u32 reg;
+
+ reg = DESC_TYPE_HOST << DESC_TYPE;
+ reg |= length;
+
+ return reg;
+}
+
+static u32 get_host_pd1(struct cppi41_channel *c)
+{
+ u32 reg;
+
+ reg = 0;
+
+ return reg;
+}
+
+static u32 get_host_pd2(struct cppi41_channel *c)
+{
+ u32 reg;
+
+ reg = 5 << 26; /* USB TYPE */
The driver shouldn't be tied to USB at all.
+static int cppi41_dma_probe(struct platform_device *pdev)
+{
+ struct cppi41_dd *cdd;
+ int irq;
+ int ret;
+
+ cdd = kzalloc(sizeof(*cdd), GFP_KERNEL);
+ if (!cdd)
+ return -ENOMEM;
+
+ dma_cap_set(DMA_SLAVE, cdd->ddev.cap_mask);
+ cdd->ddev.device_alloc_chan_resources = cppi41_dma_alloc_chan_resources;
+ cdd->ddev.device_free_chan_resources = cppi41_dma_free_chan_resources;
+ cdd->ddev.device_tx_status = cppi41_dma_tx_status;
+ cdd->ddev.device_issue_pending = cppi41_dma_issue_pending;
+ cdd->ddev.device_prep_slave_sg = cppi41_dma_prep_slave_sg;
+ cdd->ddev.device_control = cppi41_dma_control;
+ cdd->ddev.dev = &pdev->dev;
+ INIT_LIST_HEAD(&cdd->ddev.channels);
+ cpp41_dma_info.dma_cap = cdd->ddev.cap_mask;
+
+ cdd->usbss_mem = of_iomap(pdev->dev.of_node, 0);
You should use the generic platform_get_resource()/
devm_ioremap_resource() functions.
+ irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ if (!irq)
+ goto err_irq;
+
+ cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
Shouldn't be here.
[...]
+static const struct of_device_id cppi41_dma_ids[] = {
+ { .compatible = "ti,am3359-cppi41", },
CPPI 4.1 driver should be generic, not SoC specific.
diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
index c8e67fd..bfe2993 100644
--- a/drivers/usb/musb/musb_dma.h
+++ b/drivers/usb/musb/musb_dma.h
@@ -71,7 +71,7 @@ struct musb_hw_ep;
#ifdef CONFIG_USB_TI_CPPI_DMA
#define is_cppi_enabled() 1
#else
-#define is_cppi_enabled() 0
+#define is_cppi_enabled() 1
What does that mean?
diff --git a/drivers/usb/musb/musb_dmaeng.c b/drivers/usb/musb/musb_dmaeng.c
new file mode 100644
index 0000000..c12f42a
--- /dev/null
+++ b/drivers/usb/musb/musb_dmaeng.c
@@ -0,0 +1,294 @@
[...]
+static struct dma_channel *cppi41_dma_channel_allocate(struct dma_controller
*c,
+ struct musb_hw_ep *hw_ep, u8 is_tx)
+{
+ struct cppi41_dma_controller *controller = container_of(c,
+ struct cppi41_dma_controller, controller);
+ struct cppi41_dma_channel *cppi41_channel = NULL;
+ struct musb *musb = controller->musb;
+ u8 ch_num = hw_ep->epnum - 1;
+
+ if (ch_num >= MUSB_DMA_NUM_CHANNELS)
+ return NULL;
+
+ if (!is_tx)
+ return NULL;
+ if (is_tx)
You've just returned on '!is_tx'.
[...]
+static void cppi41_release_all_dma_chans(struct cppi41_dma_controller *ctrl)
+{
+ struct dma_chan *dc;
+ int i;
+
+ for (i = 0; i < MUSB_DMA_NUM_CHANNELS; i++) {
+ dc = ctrl->tx_channel[i].dc;
+ if (dc)
+ dma_release_channel(dc);
+ dc = ctrl->rx_channel[i].dc;
+ if (dc)
+ dma_release_channel(dc);
+ }
+}
+
+static void cppi41_dma_controller_stop(struct cppi41_dma_controller
*controller)
+{
+ cppi41_release_all_dma_chans(controller);
Why not just expand it inline?
+}
+
+static int cppi41_dma_controller_start(struct cppi41_dma_controller
*controller)
+{
+ struct musb *musb = controller->musb;
+ struct device *dev = musb->controller;
+ struct device_node *np = dev->of_node;
+ struct cppi41_dma_channel *cppi41_channel;
+ int count;
+ int i;
+ int ret;
+ dma_cap_mask_t mask;
+
+ dma_cap_zero(mask);
+ dma_cap_set(DMA_SLAVE, mask);
You don't seem to use 'mask'.
[...]
+ musb_dma = &cppi41_channel->channel;
+ musb_dma->private_data = cppi41_channel;
+ musb_dma->status = MUSB_DMA_STATUS_FREE;
+ musb_dma->max_len = SZ_4M;
Really?
WBR, Sergei
--
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/
