On Thu, Aug 15, 2013 at 01:54:28PM +0800, zhangfei gao wrote:

On Tue, Aug 13, 2013 at 7:20 PM, Vinod Koul [off-list ref] wrote:

quoted

On Fri, Jun 28, 2013 at 08:39:13PM +0800, Zhangfei Gao wrote:

quoted

Add dmaengine driver for hisilicon k3 platform based on virt_dma

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Tested-by: Kai Yang <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---

quoted

+#define DRIVER_NAME          "k3-dma"
+#define DMA_ALIGN            3
+#define DMA_MAX_SIZE         0x1ffc
+
+#define INT_STAT             0x00
+#define INT_TC1                      0x04
+#define INT_ERR1             0x0c
+#define INT_ERR2             0x10
+#define INT_TC1_MASK         0x18
+#define INT_ERR1_MASK                0x20
+#define INT_ERR2_MASK                0x24
+#define INT_TC1_RAW          0x600
+#define INT_ERR1_RAW         0x608
+#define INT_ERR2_RAW         0x610
+#define CH_PRI                       0x688
+#define CH_STAT                      0x690
+#define CX_CUR_CNT           0x704
+#define CX_LLI                       0x800
+#define CX_CNT                       0x810
+#define CX_SRC                       0x814
+#define CX_DST                       0x818
+#define CX_CONFIG            0x81c
+#define AXI_CONFIG           0x820
+#define DEF_AXI_CONFIG               0x201201
+
+#define CX_LLI_CHAIN_EN              0x2
+#define CCFG_EN                      0x1
+#define CCFG_MEM2PER         (0x1 << 2)
+#define CCFG_PER2MEM         (0x2 << 2)
+#define CCFG_SRCINCR         (0x1 << 31)
+#define CCFG_DSTINCR         (0x1 << 30)

I see these are not namespace aptly and can collide..

OK,
How about change name to
#define CX_CFG                 0x81c

since you are calling your driver K3_DMA it would be apt to namespace all of the
above with K3_INT_STAT to K3_CFG_EN and so on...

#define CX_CFG_EN              0x1
#define CX_CFG_MEM2PER         (0x1 << 2)
~

quoted

quoted

+             switch (width) {
+             case DMA_SLAVE_BUSWIDTH_1_BYTE:
+                     val = 0;
+                     break;
+             case DMA_SLAVE_BUSWIDTH_2_BYTES:
+                     val = 1;
+                     break;
+             case DMA_SLAVE_BUSWIDTH_4_BYTES:
+                     val = 2;
+                     break;
+             case DMA_SLAVE_BUSWIDTH_8_BYTES:
+                     val = 3;

DMA_SLAVE_BUSWIDTHS are 1, 2, 4, 8...
so if you can do val = ffs(width) as well?

Good idea, will use __ffs(width) here.

quoted

quoted

+     case DMA_PAUSE:
+             dev_dbg(d->slave.dev, "vchan %p: pause\n", &c->vc);
+             if (c->status == DMA_IN_PROGRESS) {
+                     c->status = DMA_PAUSED;
+                     if (p) {
+                             k3_dma_pause_dma(p, false);
+                     } else {
+                             spin_lock(&d->lock);
+                             list_del_init(&c->node);
+                             spin_unlock(&d->lock);
+                     }

why do we need the else part here?

Since asynchronous mode is supported.
Desc is submitted to list but may not get physical channel to run.

But when you pause you pause the running channel. You dont pause a descriptor.
So whatever you are trying to imply doesnt make sense to me.

~Vinod