Re: [RFC 1/2] usb/musb dma: add cppi41 dma driver

From: Sergei Shtylyov
Date: Sun Jul 07 2013 - 10:56:33 EST

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 <bigeasy@xxxxxxxxxxxxx>
---
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/