[PATCH 2/4] sh: separate DMA headers, used by the legacy and the

[PATCH 2/4] sh: separate DMA headers, used by the legacy and the

[Guennadi Liakhovetski]

Separate SH DMA headers into ones, commonly used by both drivers, and ones,
specific to each of them. This will make the future development of the
dmaengine driver easier.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 arch/sh/include/asm/dma-common.h           |   53 +++++++++++++
 arch/sh/include/asm/dma-sh.h               |   97 +-----------------------
 arch/sh/include/asm/sh-dmaengine.h         |   71 +++++++++++++++++
 arch/sh/include/asm/siu.h                  |    2 +-
 arch/sh/include/cpu-sh3/cpu/dma-register.h |   40 ++++++++++
 arch/sh/include/cpu-sh3/cpu/dma.h          |   27 -------
 arch/sh/include/cpu-sh4/cpu/dma-register.h |  116 ++++++++++++++++++++++++++++
 arch/sh/include/cpu-sh4/cpu/dma-sh4a.h     |   62 ---------------
 arch/sh/include/cpu-sh4/cpu/dma.h          |   36 +--------
 arch/sh/kernel/cpu/sh4a/setup-sh7722.c     |    2 +-
 arch/sh/kernel/cpu/sh4a/setup-sh7724.c     |    2 +-
 arch/sh/kernel/cpu/sh4a/setup-sh7780.c     |    2 +-
 arch/sh/kernel/cpu/sh4a/setup-sh7785.c     |    2 +-
 drivers/dma/shdma.c                        |    5 +-
 drivers/dma/shdma.h                        |    4 +-
 sound/soc/sh/Kconfig                       |    4 +
 sound/soc/sh/siu.h                         |    2 +-
 sound/soc/sh/siu_pcm.c                     |    2 +-
 18 files changed, 298 insertions(+), 231 deletions(-)
 create mode 100644 arch/sh/include/asm/dma-common.h
 create mode 100644 arch/sh/include/asm/sh-dmaengine.h
 create mode 100644 arch/sh/include/cpu-sh3/cpu/dma-register.h
 create mode 100644 arch/sh/include/cpu-sh4/cpu/dma-register.h

diff --git a/arch/sh/include/asm/dma-common.h b/arch/sh/include/asm/dma-common.h
new file mode 100644
index 0000000..a3456f4
--- /dev/null
+++ b/arch/sh/include/asm/dma-common.h
@@ -0,0 +1,53 @@
+/*
+ * Common header for the legacy SH DMA driver and the new dmaengine driver
+ *
+ * extracted from arch/sh/include/asm/dma-sh.h:
+ *
+ * Copyright (C) 2000  Takashi YOSHII
+ * Copyright (C) 2003  Paul Mundt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+#ifndef __DMA_COMMON_H
+#define __DMA_COMMON_H
+
+/* DMA register */
+#define SAR     0x00
+#define DAR     0x04
+#define TCR     0x08
+#define CHCR    0x0C
+#define DMAOR	0x40
+
+/* DMAOR definitions */
+#define DMAOR_AE	0x00000004
+#define DMAOR_NMIF	0x00000002
+#define DMAOR_DME	0x00000001
+
+/* Definitions for the SuperH DMAC */
+#define REQ_L	0x00000000
+#define REQ_E	0x00080000
+#define RACK_H	0x00000000
+#define RACK_L	0x00040000
+#define ACK_R	0x00000000
+#define ACK_W	0x00020000
+#define ACK_H	0x00000000
+#define ACK_L	0x00010000
+#define DM_INC	0x00004000
+#define DM_DEC	0x00008000
+#define DM_FIX	0x0000c000
+#define SM_INC	0x00001000
+#define SM_DEC	0x00002000
+#define SM_FIX	0x00003000
+#define RS_IN	0x00000200
+#define RS_OUT	0x00000300
+#define TS_BLK	0x00000040
+#define TM_BUR	0x00000020
+#define CHCR_DE 0x00000001
+#define CHCR_TE 0x00000002
+#define CHCR_IE 0x00000004
+
+#include <cpu/dma-register.h>
+
+#endif
diff --git a/arch/sh/include/asm/dma-sh.h b/arch/sh/include/asm/dma-sh.h
index 238df83..675a9d5 100644
--- a/arch/sh/include/asm/dma-sh.h
+++ b/arch/sh/include/asm/dma-sh.h
@@ -11,7 +11,7 @@
 #ifndef __DMA_SH_H
 #define __DMA_SH_H
 
-#include <asm/dma.h>
+#include <asm/dma-common.h>
 #include <cpu/dma.h>
 
 /* DMAOR contorl: The DMAOR access size is different by CPU.*/
@@ -53,34 +53,6 @@ static int dmte_irq_map[] __maybe_unused = {
 #endif
 };
 
-/* Definitions for the SuperH DMAC */
-#define REQ_L	0x00000000
-#define REQ_E	0x00080000
-#define RACK_H	0x00000000
-#define RACK_L	0x00040000
-#define ACK_R	0x00000000
-#define ACK_W	0x00020000
-#define ACK_H	0x00000000
-#define ACK_L	0x00010000
-#define DM_INC	0x00004000
-#define DM_DEC	0x00008000
-#define DM_FIX	0x0000c000
-#define SM_INC	0x00001000
-#define SM_DEC	0x00002000
-#define SM_FIX	0x00003000
-#define RS_IN	0x00000200
-#define RS_OUT	0x00000300
-#define TS_BLK	0x00000040
-#define TM_BUR	0x00000020
-#define CHCR_DE 0x00000001
-#define CHCR_TE 0x00000002
-#define CHCR_IE 0x00000004
-
-/* DMAOR definitions */
-#define DMAOR_AE	0x00000004
-#define DMAOR_NMIF	0x00000002
-#define DMAOR_DME	0x00000001
-
 /*
  * Define the default configuration for dual address memory-memory transfer.
  * The 0x400 value represents auto-request, external->external.
@@ -111,71 +83,4 @@ static u32 dma_base_addr[] __maybe_unused = {
 #endif
 };
 
-/* DMA register */
-#define SAR     0x00
-#define DAR     0x04
-#define TCR     0x08
-#define CHCR    0x0C
-#define DMAOR	0x40
-
-/*
- * for dma engine
- *
- * SuperH DMA mode
- */
-#define SHDMA_MIX_IRQ	(1 << 1)
-#define SHDMA_DMAOR1	(1 << 2)
-#define SHDMA_DMAE1	(1 << 3)
-
-enum sh_dmae_slave_chan_id {
-	SHDMA_SLAVE_SCIF0_TX,
-	SHDMA_SLAVE_SCIF0_RX,
-	SHDMA_SLAVE_SCIF1_TX,
-	SHDMA_SLAVE_SCIF1_RX,
-	SHDMA_SLAVE_SCIF2_TX,
-	SHDMA_SLAVE_SCIF2_RX,
-	SHDMA_SLAVE_SCIF3_TX,
-	SHDMA_SLAVE_SCIF3_RX,
-	SHDMA_SLAVE_SCIF4_TX,
-	SHDMA_SLAVE_SCIF4_RX,
-	SHDMA_SLAVE_SCIF5_TX,
-	SHDMA_SLAVE_SCIF5_RX,
-	SHDMA_SLAVE_SIUA_TX,
-	SHDMA_SLAVE_SIUA_RX,
-	SHDMA_SLAVE_SIUB_TX,
-	SHDMA_SLAVE_SIUB_RX,
-	SHDMA_SLAVE_NUMBER,	/* Must stay last */
-};
-
-struct sh_dmae_slave_config {
-	enum sh_dmae_slave_chan_id	slave_id;
-	dma_addr_t			addr;
-	u32				chcr;
-	char				mid_rid;
-};
-
-struct sh_dmae_channel {
-	unsigned int	offset;
-	int		irq;
-	unsigned int	dmars;
-	unsigned int	dmars_bit;
-};
-
-struct sh_dmae_pdata {
-	unsigned int mode;
-	struct sh_dmae_slave_config *slave;
-	int slave_num;
-	struct sh_dmae_channel *channel;
-	int channel_num;
-	unsigned int dmaor_offset;
-};
-
-struct device;
-
-struct sh_dmae_slave {
-	enum sh_dmae_slave_chan_id	slave_id; /* Set by the platform */
-	struct device			*dma_dev; /* Set by the platform */
-	struct sh_dmae_slave_config	*config;  /* Set by the driver */
-};
-
 #endif /* __DMA_SH_H */
diff --git a/arch/sh/include/asm/sh-dmaengine.h b/arch/sh/include/asm/sh-dmaengine.h
new file mode 100644
index 0000000..e127ce5
--- /dev/null
+++ b/arch/sh/include/asm/sh-dmaengine.h
@@ -0,0 +1,71 @@
+/*
+ * Header for the new SH dmaengine driver
+ *
+ * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef SH_DMAENGINE_H
+#define SH_DMAENGINE_H
+
+#include <asm/dma-common.h>
+
+#define SH_DMAC_MAX_CHANNELS	6
+
+/* SuperH DMA mode */
+#define SHDMA_MIX_IRQ	(1 << 0)
+
+enum sh_dmae_slave_chan_id {
+	SHDMA_SLAVE_SCIF0_TX,
+	SHDMA_SLAVE_SCIF0_RX,
+	SHDMA_SLAVE_SCIF1_TX,
+	SHDMA_SLAVE_SCIF1_RX,
+	SHDMA_SLAVE_SCIF2_TX,
+	SHDMA_SLAVE_SCIF2_RX,
+	SHDMA_SLAVE_SCIF3_TX,
+	SHDMA_SLAVE_SCIF3_RX,
+	SHDMA_SLAVE_SCIF4_TX,
+	SHDMA_SLAVE_SCIF4_RX,
+	SHDMA_SLAVE_SCIF5_TX,
+	SHDMA_SLAVE_SCIF5_RX,
+	SHDMA_SLAVE_SIUA_TX,
+	SHDMA_SLAVE_SIUA_RX,
+	SHDMA_SLAVE_SIUB_TX,
+	SHDMA_SLAVE_SIUB_RX,
+	SHDMA_SLAVE_NUMBER,	/* Must stay last */
+};
+
+struct sh_dmae_slave_config {
+	enum sh_dmae_slave_chan_id	slave_id;
+	dma_addr_t			addr;
+	u32				chcr;
+	char				mid_rid;
+};
+
+struct sh_dmae_channel {
+	unsigned int	offset;
+	int		irq;
+	unsigned int	dmars;
+	unsigned int	dmars_bit;
+};
+
+struct sh_dmae_pdata {
+	unsigned int mode;
+	struct sh_dmae_slave_config *slave;
+	int slave_num;
+	struct sh_dmae_channel *channel;
+	int channel_num;
+};
+
+struct device;
+
+/* Used by slave DMA clients to request DMA to/from a specific peripheral */
+struct sh_dmae_slave {
+	enum sh_dmae_slave_chan_id	slave_id; /* Set by the platform */
+	struct device			*dma_dev; /* Set by the platform */
+	struct sh_dmae_slave_config	*config;  /* Set by the driver */
+};
+
+#endif
diff --git a/arch/sh/include/asm/siu.h b/arch/sh/include/asm/siu.h
index 57565a3..1a384aa 100644
--- a/arch/sh/include/asm/siu.h
+++ b/arch/sh/include/asm/siu.h
@@ -11,7 +11,7 @@
 #ifndef ASM_SIU_H
 #define ASM_SIU_H
 
-#include <asm/dma-sh.h>
+#include <asm/sh-dmaengine.h>
 
 struct device;
 
diff --git a/arch/sh/include/cpu-sh3/cpu/dma-register.h b/arch/sh/include/cpu-sh3/cpu/dma-register.h
new file mode 100644
index 0000000..ed9c469
--- /dev/null
+++ b/arch/sh/include/cpu-sh3/cpu/dma-register.h
@@ -0,0 +1,40 @@
+/*
+ * SH3 CPU-specific DMA definitions, used by both DMA drivers
+ *
+ * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef DMA_REGISTER_H
+#define DMA_REGISTER_H
+
+#define CHCR_TS_LOW_MASK	0x18
+#define CHCR_TS_LOW_SHIFT	3
+#define CHCR_TS_HIGH_MASK	0
+#define CHCR_TS_HIGH_SHIFT	0
+
+#define DMAOR_INIT	DMAOR_DME
+
+/*
+ * The SuperH DMAC supports a number of transmit sizes, we list them here,
+ * with their respective values as they appear in the CHCR registers.
+ */
+enum {
+	XMIT_SZ_8BIT,
+	XMIT_SZ_16BIT,
+	XMIT_SZ_32BIT,
+	XMIT_SZ_128BIT,
+};
+
+#define TS_SHIFT {			\
+	[XMIT_SZ_8BIT]		= 0,	\
+	[XMIT_SZ_16BIT]		= 1,	\
+	[XMIT_SZ_32BIT]		= 2,	\
+	[XMIT_SZ_128BIT]	= 4,	\
+}
+
+#define TS_INDEX2VAL(i)	(((i) & 3) << CHCR_TS_LOW_SHIFT)
+
+#endif
diff --git a/arch/sh/include/cpu-sh3/cpu/dma.h b/arch/sh/include/cpu-sh3/cpu/dma.h
index 207811a..24e28b9 100644
--- a/arch/sh/include/cpu-sh3/cpu/dma.h
+++ b/arch/sh/include/cpu-sh3/cpu/dma.h
@@ -20,31 +20,4 @@
 #define TS_32		0x00000010
 #define TS_128		0x00000018
 
-#define CHCR_TS_LOW_MASK	0x18
-#define CHCR_TS_LOW_SHIFT	3
-#define CHCR_TS_HIGH_MASK	0
-#define CHCR_TS_HIGH_SHIFT	0
-
-#define DMAOR_INIT	DMAOR_DME
-
-/*
- * The SuperH DMAC supports a number of transmit sizes, we list them here,
- * with their respective values as they appear in the CHCR registers.
- */
-enum {
-	XMIT_SZ_8BIT,
-	XMIT_SZ_16BIT,
-	XMIT_SZ_32BIT,
-	XMIT_SZ_128BIT,
-};
-
-#define TS_SHIFT {			\
-	[XMIT_SZ_8BIT]		= 0,	\
-	[XMIT_SZ_16BIT]		= 1,	\
-	[XMIT_SZ_32BIT]		= 2,	\
-	[XMIT_SZ_128BIT]	= 4,	\
-}
-
-#define TS_INDEX2VAL(i)	(((i) & 3) << CHCR_TS_LOW_SHIFT)
-
 #endif /* __ASM_CPU_SH3_DMA_H */
diff --git a/arch/sh/include/cpu-sh4/cpu/dma-register.h b/arch/sh/include/cpu-sh4/cpu/dma-register.h
new file mode 100644
index 0000000..a065a2d
--- /dev/null
+++ b/arch/sh/include/cpu-sh4/cpu/dma-register.h
@@ -0,0 +1,116 @@
+/*
+ * SH4 CPU-specific DMA definitions, used by both DMA drivers
+ *
+ * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef DMA_REGISTER_H
+#define DMA_REGISTER_H
+
+/* SH7751/7760/7780 DMA IRQ sources */
+
+#ifdef CONFIG_CPU_SH4A
+
+#define DMAOR_INIT	DMAOR_DME
+
+#if defined(CONFIG_CPU_SUBTYPE_SH7343) || \
+	defined(CONFIG_CPU_SUBTYPE_SH7730)
+#define CHCR_TS_LOW_MASK	0x00000018
+#define CHCR_TS_LOW_SHIFT	3
+#define CHCR_TS_HIGH_MASK	0
+#define CHCR_TS_HIGH_SHIFT	0
+#elif defined(CONFIG_CPU_SUBTYPE_SH7722)
+#define CHCR_TS_LOW_MASK	0x00000018
+#define CHCR_TS_LOW_SHIFT	3
+#define CHCR_TS_HIGH_MASK	0x00300000
+#define CHCR_TS_HIGH_SHIFT	20
+#elif defined(CONFIG_CPU_SUBTYPE_SH7763) || \
+	defined(CONFIG_CPU_SUBTYPE_SH7764)
+#define CHCR_TS_LOW_MASK	0x00000018
+#define CHCR_TS_LOW_SHIFT	3
+#define CHCR_TS_HIGH_MASK	0
+#define CHCR_TS_HIGH_SHIFT	0
+#elif defined(CONFIG_CPU_SUBTYPE_SH7723)
+#define CHCR_TS_LOW_MASK	0x00000018
+#define CHCR_TS_LOW_SHIFT	3
+#define CHCR_TS_HIGH_MASK	0
+#define CHCR_TS_HIGH_SHIFT	0
+#elif defined(CONFIG_CPU_SUBTYPE_SH7724)
+#define CHCR_TS_LOW_MASK	0x00000018
+#define CHCR_TS_LOW_SHIFT	3
+#define CHCR_TS_HIGH_MASK	0x00600000
+#define CHCR_TS_HIGH_SHIFT	21
+#elif defined(CONFIG_CPU_SUBTYPE_SH7780)
+#define CHCR_TS_LOW_MASK	0x00000018
+#define CHCR_TS_LOW_SHIFT	3
+#define CHCR_TS_HIGH_MASK	0
+#define CHCR_TS_HIGH_SHIFT	0
+#else /* SH7785 */
+#define CHCR_TS_LOW_MASK	0x00000018
+#define CHCR_TS_LOW_SHIFT	3
+#define CHCR_TS_HIGH_MASK	0
+#define CHCR_TS_HIGH_SHIFT	0
+#endif
+
+/* Transmit sizes and respective CHCR register values */
+enum {
+	XMIT_SZ_8BIT		= 0,
+	XMIT_SZ_16BIT		= 1,
+	XMIT_SZ_32BIT		= 2,
+	XMIT_SZ_64BIT		= 7,
+	XMIT_SZ_128BIT		= 3,
+	XMIT_SZ_256BIT		= 4,
+	XMIT_SZ_128BIT_BLK	= 0xb,
+	XMIT_SZ_256BIT_BLK	= 0xc,
+};
+
+/* log2(size / 8) - used to calculate number of transfers */
+#define TS_SHIFT {			\
+	[XMIT_SZ_8BIT]		= 0,	\
+	[XMIT_SZ_16BIT]		= 1,	\
+	[XMIT_SZ_32BIT]		= 2,	\
+	[XMIT_SZ_64BIT]		= 3,	\
+	[XMIT_SZ_128BIT]	= 4,	\
+	[XMIT_SZ_256BIT]	= 5,	\
+	[XMIT_SZ_128BIT_BLK]	= 4,	\
+	[XMIT_SZ_256BIT_BLK]	= 5,	\
+}
+
+#define TS_INDEX2VAL(i)	((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
+			 ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
+
+#else /* CONFIG_CPU_SH4A */
+
+#define DMAOR_INIT	(0x8000 | DMAOR_DME)
+
+#define CHCR_TS_LOW_MASK	0x70
+#define CHCR_TS_LOW_SHIFT	4
+#define CHCR_TS_HIGH_MASK	0
+#define CHCR_TS_HIGH_SHIFT	0
+
+/* Transmit sizes and respective CHCR register values */
+enum {
+	XMIT_SZ_8BIT	= 1,
+	XMIT_SZ_16BIT	= 2,
+	XMIT_SZ_32BIT	= 3,
+	XMIT_SZ_64BIT	= 0,
+	XMIT_SZ_256BIT	= 4,
+};
+
+/* log2(size / 8) - used to calculate number of transfers */
+#define TS_SHIFT {			\
+	[XMIT_SZ_8BIT]		= 0,	\
+	[XMIT_SZ_16BIT]		= 1,	\
+	[XMIT_SZ_32BIT]		= 2,	\
+	[XMIT_SZ_64BIT]		= 3,	\
+	[XMIT_SZ_256BIT]	= 5,	\
+}
+
+#define TS_INDEX2VAL(i)	(((i) & 7) << CHCR_TS_LOW_SHIFT)
+
+#endif /* CONFIG_CPU_SH4A */
+
+#endif
diff --git a/arch/sh/include/cpu-sh4/cpu/dma-sh4a.h b/arch/sh/include/cpu-sh4/cpu/dma-sh4a.h
index e734ea4..9647e68 100644
--- a/arch/sh/include/cpu-sh4/cpu/dma-sh4a.h
+++ b/arch/sh/include/cpu-sh4/cpu/dma-sh4a.h
@@ -8,20 +8,12 @@
 #define DMAE0_IRQ	78	/* DMA Error IRQ*/
 #define SH_DMAC_BASE0	0xFE008020
 #define SH_DMARS_BASE0	0xFE009000
-#define CHCR_TS_LOW_MASK	0x00000018
-#define CHCR_TS_LOW_SHIFT	3
-#define CHCR_TS_HIGH_MASK	0
-#define CHCR_TS_HIGH_SHIFT	0
 #elif defined(CONFIG_CPU_SUBTYPE_SH7722)
 #define DMTE0_IRQ	48
 #define DMTE4_IRQ	76
 #define DMAE0_IRQ	78	/* DMA Error IRQ*/
 #define SH_DMAC_BASE0	0xFE008020
 #define SH_DMARS_BASE0	0xFE009000
-#define CHCR_TS_LOW_MASK	0x00000018
-#define CHCR_TS_LOW_SHIFT	3
-#define CHCR_TS_HIGH_MASK	0x00300000
-#define CHCR_TS_HIGH_SHIFT	20
 #elif defined(CONFIG_CPU_SUBTYPE_SH7763) || \
 	defined(CONFIG_CPU_SUBTYPE_SH7764)
 #define DMTE0_IRQ	34
@@ -29,10 +21,6 @@
 #define DMAE0_IRQ	38
 #define SH_DMAC_BASE0	0xFF608020
 #define SH_DMARS_BASE0	0xFF609000
-#define CHCR_TS_LOW_MASK	0x00000018
-#define CHCR_TS_LOW_SHIFT	3
-#define CHCR_TS_HIGH_MASK	0
-#define CHCR_TS_HIGH_SHIFT	0
 #elif defined(CONFIG_CPU_SUBTYPE_SH7723)
 #define DMTE0_IRQ	48	/* DMAC0A*/
 #define DMTE4_IRQ	76	/* DMAC0B */
@@ -46,10 +34,6 @@
 #define SH_DMAC_BASE0	0xFE008020
 #define SH_DMAC_BASE1	0xFDC08020
 #define SH_DMARS_BASE0	0xFDC09000
-#define CHCR_TS_LOW_MASK	0x00000018
-#define CHCR_TS_LOW_SHIFT	3
-#define CHCR_TS_HIGH_MASK	0
-#define CHCR_TS_HIGH_SHIFT	0
 #elif defined(CONFIG_CPU_SUBTYPE_SH7724)
 #define DMTE0_IRQ	48	/* DMAC0A*/
 #define DMTE4_IRQ	76	/* DMAC0B */
@@ -64,10 +48,6 @@
 #define SH_DMAC_BASE1	0xFDC08020
 #define SH_DMARS_BASE0	0xFE009000
 #define SH_DMARS_BASE1	0xFDC09000
-#define CHCR_TS_LOW_MASK	0x00000018
-#define CHCR_TS_LOW_SHIFT	3
-#define CHCR_TS_HIGH_MASK	0x00600000
-#define CHCR_TS_HIGH_SHIFT	21
 #elif defined(CONFIG_CPU_SUBTYPE_SH7780)
 #define DMTE0_IRQ	34
 #define DMTE4_IRQ	44
@@ -80,10 +60,6 @@
 #define SH_DMAC_BASE0	0xFC808020
 #define SH_DMAC_BASE1	0xFC818020
 #define SH_DMARS_BASE0	0xFC809000
-#define CHCR_TS_LOW_MASK	0x00000018
-#define CHCR_TS_LOW_SHIFT	3
-#define CHCR_TS_HIGH_MASK	0
-#define CHCR_TS_HIGH_SHIFT	0
 #else /* SH7785 */
 #define DMTE0_IRQ	33
 #define DMTE4_IRQ	37
@@ -97,10 +73,6 @@
 #define SH_DMAC_BASE0	0xFC808020
 #define SH_DMAC_BASE1	0xFCC08020
 #define SH_DMARS_BASE0	0xFC809000
-#define CHCR_TS_LOW_MASK	0x00000018
-#define CHCR_TS_LOW_SHIFT	3
-#define CHCR_TS_HIGH_MASK	0
-#define CHCR_TS_HIGH_SHIFT	0
 #endif
 
 #define REQ_HE		0x000000C0
@@ -108,38 +80,4 @@
 #define REQ_LE		0x00000040
 #define TM_BURST	0x00000020
 
-/*
- * The SuperH DMAC supports a number of transmit sizes, we list them here,
- * with their respective values as they appear in the CHCR registers.
- *
- * Defaults to a 64-bit transfer size.
- */
-enum {
-	XMIT_SZ_8BIT		= 0,
-	XMIT_SZ_16BIT		= 1,
-	XMIT_SZ_32BIT		= 2,
-	XMIT_SZ_64BIT		= 7,
-	XMIT_SZ_128BIT		= 3,
-	XMIT_SZ_256BIT		= 4,
-	XMIT_SZ_128BIT_BLK	= 0xb,
-	XMIT_SZ_256BIT_BLK	= 0xc,
-};
-
-/*
- * The DMA count is defined as the number of bytes to transfer.
- */
-#define TS_SHIFT {			\
-	[XMIT_SZ_8BIT]		= 0,	\
-	[XMIT_SZ_16BIT]		= 1,	\
-	[XMIT_SZ_32BIT]		= 2,	\
-	[XMIT_SZ_64BIT]		= 3,	\
-	[XMIT_SZ_128BIT]	= 4,	\
-	[XMIT_SZ_256BIT]	= 5,	\
-	[XMIT_SZ_128BIT_BLK]	= 4,	\
-	[XMIT_SZ_256BIT_BLK]	= 5,	\
-}
-
-#define TS_INDEX2VAL(i)	((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
-			 ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
-
 #endif /* __ASM_SH_CPU_SH4_DMA_SH7780_H */
diff --git a/arch/sh/include/cpu-sh4/cpu/dma.h b/arch/sh/include/cpu-sh4/cpu/dma.h
index 114a369..ca747e9 100644
--- a/arch/sh/include/cpu-sh4/cpu/dma.h
+++ b/arch/sh/include/cpu-sh4/cpu/dma.h
@@ -5,9 +5,8 @@
 
 #ifdef CONFIG_CPU_SH4A
 
-#define DMAOR_INIT	(DMAOR_DME)
-
 #include <cpu/dma-sh4a.h>
+
 #else /* CONFIG_CPU_SH4A */
 /*
  * SH7750/SH7751/SH7760
@@ -17,7 +16,6 @@
 #define DMTE6_IRQ	46
 #define DMAE0_IRQ	38
 
-#define DMAOR_INIT	(0x8000|DMAOR_DME)
 #define SH_DMAC_BASE0	0xffa00000
 #define SH_DMAC_BASE1	0xffa00070
 /* Definitions for the SuperH DMAC */
@@ -27,40 +25,8 @@
 #define TS_32		0x00000030
 #define TS_64		0x00000000
 
-#define CHCR_TS_LOW_MASK	0x70
-#define CHCR_TS_LOW_SHIFT	4
-#define CHCR_TS_HIGH_MASK	0
-#define CHCR_TS_HIGH_SHIFT	0
-
 #define DMAOR_COD	0x00000008
 
-/*
- * The SuperH DMAC supports a number of transmit sizes, we list them here,
- * with their respective values as they appear in the CHCR registers.
- *
- * Defaults to a 64-bit transfer size.
- */
-enum {
-	XMIT_SZ_8BIT	= 1,
-	XMIT_SZ_16BIT	= 2,
-	XMIT_SZ_32BIT	= 3,
-	XMIT_SZ_64BIT	= 0,
-	XMIT_SZ_256BIT	= 4,
-};
-
-/*
- * The DMA count is defined as the number of bytes to transfer.
- */
-#define TS_SHIFT {			\
-	[XMIT_SZ_8BIT]		= 0,	\
-	[XMIT_SZ_16BIT]		= 1,	\
-	[XMIT_SZ_32BIT]		= 2,	\
-	[XMIT_SZ_64BIT]		= 3,	\
-	[XMIT_SZ_256BIT]	= 5,	\
-}
-
-#define TS_INDEX2VAL(i)	(((i) & 7) << CHCR_TS_LOW_SHIFT)
-
 #endif
 
 #endif /* __ASM_CPU_SH4_DMA_H */
diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7722.c b/arch/sh/kernel/cpu/sh4a/setup-sh7722.c
index c1335e5..5d7dc19 100644
--- a/arch/sh/kernel/cpu/sh4a/setup-sh7722.c
+++ b/arch/sh/kernel/cpu/sh4a/setup-sh7722.c
@@ -17,7 +17,7 @@
 #include <linux/sh_timer.h>
 #include <asm/clock.h>
 #include <asm/mmzone.h>
-#include <asm/dma-sh.h>
+#include <asm/sh-dmaengine.h>
 #include <asm/siu.h>
 #include <cpu/sh7722.h>
 
diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7724.c b/arch/sh/kernel/cpu/sh4a/setup-sh7724.c
index 460d147..b692e61 100644
--- a/arch/sh/kernel/cpu/sh4a/setup-sh7724.c
+++ b/arch/sh/kernel/cpu/sh4a/setup-sh7724.c
@@ -23,7 +23,7 @@
 #include <linux/notifier.h>
 #include <asm/suspend.h>
 #include <asm/clock.h>
-#include <asm/dma-sh.h>
+#include <asm/sh-dmaengine.h>
 #include <asm/mmzone.h>
 #include <cpu/sh7724.h>
 
diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7780.c b/arch/sh/kernel/cpu/sh4a/setup-sh7780.c
index 851de27..99c6b77 100644
--- a/arch/sh/kernel/cpu/sh4a/setup-sh7780.c
+++ b/arch/sh/kernel/cpu/sh4a/setup-sh7780.c
@@ -13,7 +13,7 @@
 #include <linux/io.h>
 #include <linux/serial_sci.h>
 #include <linux/sh_timer.h>
-#include <asm/dma-sh.h>
+#include <asm/sh-dmaengine.h>
 
 static struct plat_sci_port scif0_platform_data = {
 	.mapbase	= 0xffe00000,
diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7785.c b/arch/sh/kernel/cpu/sh4a/setup-sh7785.c
index e3abc2d..ad55a6c 100644
--- a/arch/sh/kernel/cpu/sh4a/setup-sh7785.c
+++ b/arch/sh/kernel/cpu/sh4a/setup-sh7785.c
@@ -14,7 +14,7 @@
 #include <linux/io.h>
 #include <linux/mm.h>
 #include <linux/sh_timer.h>
-#include <asm/dma-sh.h>
+#include <asm/sh-dmaengine.h>
 #include <asm/mmzone.h>
 
 static struct plat_sci_port scif0_platform_data = {
diff --git a/drivers/dma/shdma.c b/drivers/dma/shdma.c
index 7242776..3498cab 100644
--- a/drivers/dma/shdma.c
+++ b/drivers/dma/shdma.c
@@ -24,8 +24,7 @@
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
-#include <cpu/dma.h>
-#include <asm/dma-sh.h>
+#include <asm/sh-dmaengine.h>
 #include "shdma.h"
 
 /* DMA descriptor control */
@@ -46,7 +45,7 @@ enum sh_dmae_desc_status {
  * If you want to change mode, you need to change RS_DEFAULT of value.
  * (ex 1byte burst mode -> (RS_DUAL & ~TS_32)
  */
-#define RS_DEFAULT  (RS_DUAL)
+#define RS_DEFAULT  (DM_INC | SM_INC | 0x400 | TS_INDEX2VAL(XMIT_SZ_32BIT))
 
 /* A bitmask with bits enough for enum sh_dmae_slave_chan_id */
 static unsigned long sh_dmae_slave_used[BITS_TO_LONGS(SHDMA_SLAVE_NUMBER)];
diff --git a/drivers/dma/shdma.h b/drivers/dma/shdma.h
index a99c385..d910398 100644
--- a/drivers/dma/shdma.h
+++ b/drivers/dma/shdma.h
@@ -17,6 +17,8 @@
 #include <linux/interrupt.h>
 #include <linux/list.h>
 
+#include <asm/sh-dmaengine.h>
+
 #define SH_DMA_TCR_MAX 0x00FFFFFF	/* 16MB */
 
 struct sh_dmae_regs {
@@ -54,7 +56,7 @@ struct sh_dmae_chan {
 
 struct sh_dmae_device {
 	struct dma_device common;
-	struct sh_dmae_chan *chan[MAX_DMA_CHANNELS];
+	struct sh_dmae_chan *chan[SH_DMAC_MAX_CHANNELS];
 	struct sh_dmae_pdata *pdata;
 	u32 __iomem *chan_reg;
 	u16 __iomem *dmars;
diff --git a/sound/soc/sh/Kconfig b/sound/soc/sh/Kconfig
index a86696b..73aba28 100644
--- a/sound/soc/sh/Kconfig
+++ b/sound/soc/sh/Kconfig
@@ -26,9 +26,13 @@ config SND_SOC_SH4_FSI
 	help
 	  This option enables FSI sound support
 
+# for some reason "depends on !SH_DMA_API" doesn't work. So, have to
+# "select DMA_ENGINE", then drivers can also be built with SH_DMA_API enabled,
+# but, certainly, cannot be used.
 config SND_SOC_SH4_SIU
 	tristate
 	depends on (SUPERH || ARCH_SHMOBILE) && HAVE_CLK
+	select DMA_ENGINE
 	select DMADEVICES
 	select SH_DMAE
 
diff --git a/sound/soc/sh/siu.h b/sound/soc/sh/siu.h
index 9cc04ab..fc9a03f 100644
--- a/sound/soc/sh/siu.h
+++ b/sound/soc/sh/siu.h
@@ -72,7 +72,7 @@ struct siu_firmware {
 #include <linux/interrupt.h>
 #include <linux/io.h>
 
-#include <asm/dma-sh.h>
+#include <asm/sh-dmaengine.h>
 
 #include <sound/core.h>
 #include <sound/pcm.h>
diff --git a/sound/soc/sh/siu_pcm.c b/sound/soc/sh/siu_pcm.c
index c5efc30..c307bb9 100644
--- a/sound/soc/sh/siu_pcm.c
+++ b/sound/soc/sh/siu_pcm.c
@@ -32,7 +32,7 @@
 #include <sound/pcm_params.h>
 #include <sound/soc-dai.h>
 
-#include <asm/dma-sh.h>
+#include <asm/sh-dmaengine.h>
 #include <asm/siu.h>
 
 #include "siu.h"
-- 
1.6.2.4

Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers

[Paul Mundt]

On Wed, Feb 03, 2010 at 04:03:50PM +0100, Guennadi Liakhovetski wrote:
> diff --git a/arch/sh/include/asm/dma-common.h b/arch/sh/include/asm/dma-common.h
> new file mode 100644
> index 0000000..a3456f4
> --- /dev/null
> +++ b/arch/sh/include/asm/dma-common.h
[snip]

> +#define CHCR_DE 0x00000001
> +#define CHCR_TE 0x00000002
> +#define CHCR_IE 0x00000004
> +
> +#include <cpu/dma-register.h>
> +
> +#endif

The easier option would be to rename dma-common.h to dma-register.h,
which would bring it in line with what it's actually pulling in from cpu/.

Even then, these definitions are specific to Hitachi/Renesas-style
on-chip DMACs, so they're not really common across the architecture (this
is why we have dma-sh.h instead of stashing everything in dma.h, although
in retrospect I could have chosen a better name).

> diff --git a/arch/sh/include/asm/sh-dmaengine.h b/arch/sh/include/asm/sh-dmaengine.h
> new file mode 100644
> index 0000000..e127ce5
> --- /dev/null
> +++ b/arch/sh/include/asm/sh-dmaengine.h
> @@ -0,0 +1,71 @@
> +/*
> + * Header for the new SH dmaengine driver
> + *
> + * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef SH_DMAENGINE_H
> +#define SH_DMAENGINE_H
> +

Lets just call this asm/dmaengine.h for now. If more dmaengine drivers
come along that want to fight over the namespace, we can shuffle things
around as necessary then.

> +#include <asm/dma-common.h>
> +
> +#define SH_DMAC_MAX_CHANNELS	6
> +
> +/* SuperH DMA mode */
> +#define SHDMA_MIX_IRQ	(1 << 0)
> +
With the IRQs being passed through platform data, there shouldn't be any
need for this anymore. If the driver wants to use it internally for
differentiating between muxed vs split out IRQs that's fine, but the
platform data shouldn't care.

> diff --git a/arch/sh/include/cpu-sh3/cpu/dma-register.h b/arch/sh/include/cpu-sh3/cpu/dma-register.h
> new file mode 100644
> index 0000000..ed9c469
> --- /dev/null
> +++ b/arch/sh/include/cpu-sh3/cpu/dma-register.h
> @@ -0,0 +1,40 @@
> +/*
> + * SH3 CPU-specific DMA definitions, used by both DMA drivers
> + *
> + * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef DMA_REGISTER_H
> +#define DMA_REGISTER_H
> +
> +#define CHCR_TS_LOW_MASK	0x18
> +#define CHCR_TS_LOW_SHIFT	3
> +#define CHCR_TS_HIGH_MASK	0
> +#define CHCR_TS_HIGH_SHIFT	0
> +
> +#define DMAOR_INIT	DMAOR_DME
> +
These should all be part of the platform data.

> +/*
> + * The SuperH DMAC supports a number of transmit sizes, we list them here,
> + * with their respective values as they appear in the CHCR registers.
> + */
> +enum {
> +	XMIT_SZ_8BIT,
> +	XMIT_SZ_16BIT,
> +	XMIT_SZ_32BIT,
> +	XMIT_SZ_128BIT,
> +};
> +
> +#define TS_SHIFT {			\
> +	[XMIT_SZ_8BIT]		= 0,	\
> +	[XMIT_SZ_16BIT]		= 1,	\
> +	[XMIT_SZ_32BIT]		= 2,	\
> +	[XMIT_SZ_128BIT]	= 4,	\
> +}
> +
> +#define TS_INDEX2VAL(i)	(((i) & 3) << CHCR_TS_LOW_SHIFT)
> +
If we compare the simple case with .. 

> +/* Transmit sizes and respective CHCR register values */
> +enum {
> +	XMIT_SZ_8BIT		= 0,
> +	XMIT_SZ_16BIT		= 1,
> +	XMIT_SZ_32BIT		= 2,
> +	XMIT_SZ_64BIT		= 7,
> +	XMIT_SZ_128BIT		= 3,
> +	XMIT_SZ_256BIT		= 4,
> +	XMIT_SZ_128BIT_BLK	= 0xb,
> +	XMIT_SZ_256BIT_BLK	= 0xc,
> +};
> +
> +/* log2(size / 8) - used to calculate number of transfers */
> +#define TS_SHIFT {			\
> +	[XMIT_SZ_8BIT]		= 0,	\
> +	[XMIT_SZ_16BIT]		= 1,	\
> +	[XMIT_SZ_32BIT]		= 2,	\
> +	[XMIT_SZ_64BIT]		= 3,	\
> +	[XMIT_SZ_128BIT]	= 4,	\
> +	[XMIT_SZ_256BIT]	= 5,	\
> +	[XMIT_SZ_128BIT_BLK]	= 4,	\
> +	[XMIT_SZ_256BIT_BLK]	= 5,	\
> +}
> +
> +#define TS_INDEX2VAL(i)	((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
> +			 ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
> +
We see that the complex case is a superset, suggesting that it's
worthwhile to make this common.

This XMIT_SZ_xxx enum should be moved in to the upper level so all CPUs
have visibility of it, and at the same time should be converted to shifts
to permit platform data to have an xmit_sz_valid bitmap or something
similar.

Beyond that, if the xmit sizes are ordered logically by size rather than
by CHCR values then TS_SHIFT can go away and the entire thing can be
calculated dynamically, we would just need a helper routine for
converting a size mask to a CHCR one.

After the above items are done, we should be in a position to kill off
all of the CPU DMA headers.

Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and

[Guennadi Liakhovetski]

Hi Paul

Thanks for the review. A general question first: many of the headers in 
this patch are DMAC specific, not just SuperH DMA specific, are there no 
other DMA controllers on DH? Can there ever be any other ones? If so, 
maybe we have to choose more specific names, than just dma-register.h or 
dmaengine.h... However, so far I don't see any other SH DMA controller 
implementations in the kernel - apart from device-specific ones (like 
video in / out).

On Thu, 4 Feb 2010, Paul Mundt wrote:

> On Wed, Feb 03, 2010 at 04:03:50PM +0100, Guennadi Liakhovetski wrote:
> > diff --git a/arch/sh/include/asm/dma-common.h b/arch/sh/include/asm/dma-common.h
> > new file mode 100644
> > index 0000000..a3456f4
> > --- /dev/null
> > +++ b/arch/sh/include/asm/dma-common.h
> [snip]
> 
> > +#define CHCR_DE 0x00000001
> > +#define CHCR_TE 0x00000002
> > +#define CHCR_IE 0x00000004
> > +
> > +#include <cpu/dma-register.h>
> > +
> > +#endif
> 
> The easier option would be to rename dma-common.h to dma-register.h,
> which would bring it in line with what it's actually pulling in from cpu/.

ok

> Even then, these definitions are specific to Hitachi/Renesas-style
> on-chip DMACs, so they're not really common across the architecture (this
> is why we have dma-sh.h instead of stashing everything in dma.h, although
> in retrospect I could have chosen a better name).

Sorry, don't understand. How does putting a file under arch/sh in the "sh" 
namespace (dma-sh.h) add any information about its scope?

> > diff --git a/arch/sh/include/asm/sh-dmaengine.h b/arch/sh/include/asm/sh-dmaengine.h
> > new file mode 100644
> > index 0000000..e127ce5
> > --- /dev/null
> > +++ b/arch/sh/include/asm/sh-dmaengine.h
> > @@ -0,0 +1,71 @@
> > +/*
> > + * Header for the new SH dmaengine driver
> > + *
> > + * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef SH_DMAENGINE_H
> > +#define SH_DMAENGINE_H
> > +
> 
> Lets just call this asm/dmaengine.h for now. If more dmaengine drivers
> come along that want to fight over the namespace, we can shuffle things
> around as necessary then.

ok

> 
> > +#include <asm/dma-common.h>
> > +
> > +#define SH_DMAC_MAX_CHANNELS	6
> > +
> > +/* SuperH DMA mode */
> > +#define SHDMA_MIX_IRQ	(1 << 0)
> > +
> With the IRQs being passed through platform data, there shouldn't be any
> need for this anymore. If the driver wants to use it internally for
> differentiating between muxed vs split out IRQs that's fine, but the
> platform data shouldn't care.

Actually, do you have an idea why some platforms want or have to use this 
shared IRQ scheme? I see 7780 and 7785 using it, although they do have an 
IRQ assigned to each channel, the only exception seems to be 7780, which 
shares 1 DMA Address Error IRQ between both controllers, but this flag 
also multiplexes all channel IRQs... How would you prefer to decide, when 
to share IRQs without this flag? Check if IRQs of channel 0 and channel 1 
are equal? Or just set the IRQF_SHARED flag always?

> > diff --git a/arch/sh/include/cpu-sh3/cpu/dma-register.h b/arch/sh/include/cpu-sh3/cpu/dma-register.h
> > new file mode 100644
> > index 0000000..ed9c469
> > --- /dev/null
> > +++ b/arch/sh/include/cpu-sh3/cpu/dma-register.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * SH3 CPU-specific DMA definitions, used by both DMA drivers
> > + *
> > + * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef DMA_REGISTER_H
> > +#define DMA_REGISTER_H
> > +
> > +#define CHCR_TS_LOW_MASK	0x18
> > +#define CHCR_TS_LOW_SHIFT	3
> > +#define CHCR_TS_HIGH_MASK	0
> > +#define CHCR_TS_HIGH_SHIFT	0
> > +
> > +#define DMAOR_INIT	DMAOR_DME
> > +
> These should all be part of the platform data.

Well, I've spent quite a bit of time thinking about it, and even at first 
implemented it like that... But then I decided, that this is something, 
that the driver itself can know. I decided, that the proper separation 
between platform data and platform-specific driver knowledge would be - 
everything, that concerns controller's integration into the system, like 
address ranges, interrupt numbers, belongs to platform data, but if 
internal register layout differs between platforms, I thought, that should 
be driver's internal knowledge. But if you disagree I can switch it over, 
no problem. Besides, at least so far, this is not per-controller, but 
per-platform specifics.

Besides, these macros are also used by the legacy driver, so, we would end 
up with a duplication. They are also used to define TS_INDEX2VAL()...

> > +/*
> > + * The SuperH DMAC supports a number of transmit sizes, we list them here,
> > + * with their respective values as they appear in the CHCR registers.
> > + */
> > +enum {
> > +	XMIT_SZ_8BIT,
> > +	XMIT_SZ_16BIT,
> > +	XMIT_SZ_32BIT,
> > +	XMIT_SZ_128BIT,
> > +};
> > +
> > +#define TS_SHIFT {			\
> > +	[XMIT_SZ_8BIT]		= 0,	\
> > +	[XMIT_SZ_16BIT]		= 1,	\
> > +	[XMIT_SZ_32BIT]		= 2,	\
> > +	[XMIT_SZ_128BIT]	= 4,	\
> > +}
> > +
> > +#define TS_INDEX2VAL(i)	(((i) & 3) << CHCR_TS_LOW_SHIFT)
> > +
> If we compare the simple case with .. 
> 
> > +/* Transmit sizes and respective CHCR register values */
> > +enum {
> > +	XMIT_SZ_8BIT		= 0,
> > +	XMIT_SZ_16BIT		= 1,
> > +	XMIT_SZ_32BIT		= 2,
> > +	XMIT_SZ_64BIT		= 7,
> > +	XMIT_SZ_128BIT		= 3,
> > +	XMIT_SZ_256BIT		= 4,
> > +	XMIT_SZ_128BIT_BLK	= 0xb,
> > +	XMIT_SZ_256BIT_BLK	= 0xc,
> > +};
> > +
> > +/* log2(size / 8) - used to calculate number of transfers */
> > +#define TS_SHIFT {			\
> > +	[XMIT_SZ_8BIT]		= 0,	\
> > +	[XMIT_SZ_16BIT]		= 1,	\
> > +	[XMIT_SZ_32BIT]		= 2,	\
> > +	[XMIT_SZ_64BIT]		= 3,	\
> > +	[XMIT_SZ_128BIT]	= 4,	\
> > +	[XMIT_SZ_256BIT]	= 5,	\
> > +	[XMIT_SZ_128BIT_BLK]	= 4,	\
> > +	[XMIT_SZ_256BIT_BLK]	= 5,	\
> > +}
> > +
> > +#define TS_INDEX2VAL(i)	((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
> > +			 ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
> > +
> We see that the complex case is a superset, suggesting that it's
> worthwhile to make this common.

Not for all, SH4 has it different, there 0 means 64-byte transfers. But 
that's for the enum. The array itself is _obviously_ identical - it's just 
the bit-shift for each transfer size - just as my own comment is saying;) 
So, yes, that one can be made common.

> This XMIT_SZ_xxx enum should be moved in to the upper level so all CPUs
> have visibility of it, and at the same time should be converted to shifts
> to permit platform data to have an xmit_sz_valid bitmap or something
> similar.

I think, you misunderstood it. The enum lists CHCR TS bitfield values, so, 
we cannot arbitrarily define it. As for validity check - this array is 
only used at one 

> Beyond that, if the xmit sizes are ordered logically by size rather than
> by CHCR values then TS_SHIFT can go away and the entire thing can be
> calculated dynamically, we would just need a helper routine for
> converting a size mask to a CHCR one.
> 
> After the above items are done, we should be in a position to kill off
> all of the CPU DMA headers.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers

[Paul Mundt]

On Fri, Feb 05, 2010 at 10:33:55AM +0100, Guennadi Liakhovetski wrote:
> Thanks for the review. A general question first: many of the headers in 
> this patch are DMAC specific, not just SuperH DMA specific, are there no 
> other DMA controllers on DH? Can there ever be any other ones? If so, 
> maybe we have to choose more specific names, than just dma-register.h or 
> dmaengine.h... However, so far I don't see any other SH DMA controller 
> implementations in the kernel - apart from device-specific ones (like 
> video in / out).
> 
In general all of the on-chip DMACs for system DMA are variations on the
ancient Hitachi DMAC. Some contain more complex features than others (ie,
native scatter-gather, chained transfers, etc, etc.) but they're all
effectively rolled from the same block.

The hordes of other DMACs that find their way in to the design end up
being fairly specific, and will never be suitable for a generic
framework. The corner cases are engines that cascade off of fixed on-chip
DMAC channels and behave like conventional ISA DMA, but no one has built
a system like that since the Dreamcast.

> On Thu, 4 Feb 2010, Paul Mundt wrote:
> > Even then, these definitions are specific to Hitachi/Renesas-style
> > on-chip DMACs, so they're not really common across the architecture (this
> > is why we have dma-sh.h instead of stashing everything in dma.h, although
> > in retrospect I could have chosen a better name).
> 
> Sorry, don't understand. How does putting a file under arch/sh in the "sh" 
> namespace (dma-sh.h) add any information about its scope?
> 
It's a bit confusing, but that particular block has always been fairly
standard SH IP. Having said that, it's not implemented by all of the
CPUs, and it's a wholly optional component for the architecture. Others,
like the ST parts, have their own controllers that have little in common
with the generic SH one. All of the Hitachi/Renesas SH parts evolved from
the old Hitachi DMAC. 

> > > +#define SHDMA_MIX_IRQ	(1 << 0)
> > > +
> > With the IRQs being passed through platform data, there shouldn't be any
> > need for this anymore. If the driver wants to use it internally for
> > differentiating between muxed vs split out IRQs that's fine, but the
> > platform data shouldn't care.
> 
> Actually, do you have an idea why some platforms want or have to use this 
> shared IRQ scheme? I see 7780 and 7785 using it, although they do have an 
> IRQ assigned to each channel, the only exception seems to be 7780, which 
> shares 1 DMA Address Error IRQ between both controllers, but this flag 
> also multiplexes all channel IRQs... How would you prefer to decide, when 
> to share IRQs without this flag? Check if IRQs of channel 0 and channel 1 
> are equal? Or just set the IRQF_SHARED flag always?
> 
The shared interrupt scheme is to deal with CPUs that have multiple IRQ
vectors backed by a single masking source. Given that there's no ability
to mask one vector without disabling all of them, we only request_irq()
for each unique masking source and make it shared for the cases where
there are multiple vectors sharing it. This will always be CPU-specific,
so you need to check the intc tables for a specific subtype to figure out
what it can and can't support.

Comparing the interrupt numbers is not sufficient, since they'll be
different. The IRQs are all unique, and are not muxed, they just can't be
masked individually. We could pass the IRQ flow information in through
the IRQ resource (ie, IORESOURCE_IRQ | IRQF_SHARED), but more than likely
most CPUs are going to have this issue, so just using IRQF_SHARED as a
default should be fine, too.

> > > +#define CHCR_TS_LOW_MASK	0x18
> > > +#define CHCR_TS_LOW_SHIFT	3
> > > +#define CHCR_TS_HIGH_MASK	0
> > > +#define CHCR_TS_HIGH_SHIFT	0
> > > +
> > > +#define DMAOR_INIT	DMAOR_DME
> > > +
> > These should all be part of the platform data.
> 
> Well, I've spent quite a bit of time thinking about it, and even at first 
> implemented it like that... But then I decided, that this is something, 
> that the driver itself can know. I decided, that the proper separation 
> between platform data and platform-specific driver knowledge would be - 
> everything, that concerns controller's integration into the system, like 
> address ranges, interrupt numbers, belongs to platform data, but if 
> internal register layout differs between platforms, I thought, that should 
> be driver's internal knowledge. But if you disagree I can switch it over, 
> no problem. Besides, at least so far, this is not per-controller, but 
> per-platform specifics.
> 
Internal register layout will never vary between platforms because the
controller itself is wholly a property of the SoC. Different on-chip
DMACs might have different layouts, but again, all of that is known at
the time that the CPU definitions are being written up. Other than things
like pinmux, the platform itself has very little say about what goes on
with the DMACs.

There are corner cases like the Dreamcast that use reserved bits in
certain system registers to get cascade working, but these should be
treated as the abominations that they are rather than penalizing systems
that aren't crap.

The pros of having all of this as platform data far outweigh any cons.
The few systems that have a problem with this quite frankly brought it on
themselves, and is nothing you should be concerning yourself with.

> Besides, these macros are also used by the legacy driver, so, we would end 
> up with a duplication. They are also used to define TS_INDEX2VAL()...
> 
Duplication is fine. The legacy driver is its own thing entirely, nothing
in the new driver should be pandering to it. The new driver gives us a
chance to dump all of the legacy garbage and do things right, the fringe
systems that are using the legacy bits can live with duplication. They're
also unlikely to ever support the new driver.

> > > +enum {
> > > +	XMIT_SZ_8BIT		= 0,
> > > +	XMIT_SZ_16BIT		= 1,
> > > +	XMIT_SZ_32BIT		= 2,
> > > +	XMIT_SZ_64BIT		= 7,
> > > +	XMIT_SZ_128BIT		= 3,
> > > +	XMIT_SZ_256BIT		= 4,
> > > +	XMIT_SZ_128BIT_BLK	= 0xb,
> > > +	XMIT_SZ_256BIT_BLK	= 0xc,
> > > +};
> > > +
> > > +/* log2(size / 8) - used to calculate number of transfers */
> > > +#define TS_SHIFT {			\
> > > +	[XMIT_SZ_8BIT]		= 0,	\
> > > +	[XMIT_SZ_16BIT]		= 1,	\
> > > +	[XMIT_SZ_32BIT]		= 2,	\
> > > +	[XMIT_SZ_64BIT]		= 3,	\
> > > +	[XMIT_SZ_128BIT]	= 4,	\
> > > +	[XMIT_SZ_256BIT]	= 5,	\
> > > +	[XMIT_SZ_128BIT_BLK]	= 4,	\
> > > +	[XMIT_SZ_256BIT_BLK]	= 5,	\
> > > +}
> > > +
> > > +#define TS_INDEX2VAL(i)	((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
> > > +			 ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
> > > +
> > We see that the complex case is a superset, suggesting that it's
> > worthwhile to make this common.
> 
> Not for all, SH4 has it different, there 0 means 64-byte transfers. But 
> that's for the enum. The array itself is _obviously_ identical - it's just 
> the bit-shift for each transfer size - just as my own comment is saying;) 
> So, yes, that one can be made common.
> 
That's ok, as long as it's just one or two CPU families that have such
variations we can just ifdef them in place. If it gets out of hand beyond
that, then that too should be shoved in to platform data.

> > This XMIT_SZ_xxx enum should be moved in to the upper level so all CPUs
> > have visibility of it, and at the same time should be converted to shifts
> > to permit platform data to have an xmit_sz_valid bitmap or something
> > similar.
> 
> I think, you misunderstood it. The enum lists CHCR TS bitfield values, so, 
> we cannot arbitrarily define it. As for validity check - this array is 
> only used at one 
> 
I think you misunderstood my comment. My point is that since the TS_SHIFT
is effectively common we could just represent that with a bitmap and then
convert the valid sizes to bit positions for indicating which of the
values is valid for that CPU. A simple wrapper around that to map it out
to the proper CHCR layout would then take care of handling CPU-specific
differences dynamically, which in turn would permit us to kill off all of
this crap from the headers.

Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and

[Guennadi Liakhovetski]

On Mon, 8 Feb 2010, Paul Mundt wrote:

> On Fri, Feb 05, 2010 at 10:33:55AM +0100, Guennadi Liakhovetski wrote:

...

> > > > +#define SHDMA_MIX_IRQ	(1 << 0)
> > > > +
> > > With the IRQs being passed through platform data, there shouldn't be any
> > > need for this anymore. If the driver wants to use it internally for
> > > differentiating between muxed vs split out IRQs that's fine, but the
> > > platform data shouldn't care.
> > 
> > Actually, do you have an idea why some platforms want or have to use this 
> > shared IRQ scheme? I see 7780 and 7785 using it, although they do have an 
> > IRQ assigned to each channel, the only exception seems to be 7780, which 
> > shares 1 DMA Address Error IRQ between both controllers, but this flag 
> > also multiplexes all channel IRQs... How would you prefer to decide, when 
> > to share IRQs without this flag? Check if IRQs of channel 0 and channel 1 
> > are equal? Or just set the IRQF_SHARED flag always?
> > 
> The shared interrupt scheme is to deal with CPUs that have multiple IRQ
> vectors backed by a single masking source. Given that there's no ability
> to mask one vector without disabling all of them, we only request_irq()
> for each unique masking source and make it shared for the cases where
> there are multiple vectors sharing it. This will always be CPU-specific,
> so you need to check the intc tables for a specific subtype to figure out
> what it can and can't support.
> 
> Comparing the interrupt numbers is not sufficient, since they'll be
> different. The IRQs are all unique, and are not muxed, they just can't be
> masked individually. We could pass the IRQ flow information in through
> the IRQ resource (ie, IORESOURCE_IRQ | IRQF_SHARED), but more than likely
> most CPUs are going to have this issue, so just using IRQF_SHARED as a
> default should be fine, too.

The point is, that the original driver checked for the SHDMA_MIX_IRQ flag, 
and if it was set, it used only one IRQ number for all channels and the 
error interrupt. So, it affects not only whether IRQF_SHARED is used, but 
also what IRQs are requested. If we use different IRQ numbers in platform 
data, then we need a way to tell the driver, that only the first one has 
to be used. Otherwise we can either specify the same IRQ number already in 
platform data, or we do request those different IRQs, in which case the 
intc driver will know to use the same mask bit, but then we don't need 
IRQF_SHARED?

> > > > +#define CHCR_TS_LOW_MASK	0x18
> > > > +#define CHCR_TS_LOW_SHIFT	3
> > > > +#define CHCR_TS_HIGH_MASK	0
> > > > +#define CHCR_TS_HIGH_SHIFT	0
> > > > +
> > > > +#define DMAOR_INIT	DMAOR_DME
> > > > +
> > > These should all be part of the platform data.
> > 
> > Well, I've spent quite a bit of time thinking about it, and even at first 
> > implemented it like that... But then I decided, that this is something, 
> > that the driver itself can know. I decided, that the proper separation 
> > between platform data and platform-specific driver knowledge would be - 
> > everything, that concerns controller's integration into the system, like 
> > address ranges, interrupt numbers, belongs to platform data, but if 
> > internal register layout differs between platforms, I thought, that should 
> > be driver's internal knowledge. But if you disagree I can switch it over, 
> > no problem. Besides, at least so far, this is not per-controller, but 
> > per-platform specifics.
> > 
> Internal register layout will never vary between platforms because the
> controller itself is wholly a property of the SoC. Different on-chip
> DMACs might have different layouts, but again, all of that is known at
> the time that the CPU definitions are being written up.

Yes, sorry, s/per-platform/per-SoC/. And no, I wasn't aware, that 
different DMAC instances on one SoC can have different register layouts. 
Just like SCIFs then;)

> Other than things
> like pinmux, the platform itself has very little say about what goes on
> with the DMACs.
> 
> There are corner cases like the Dreamcast that use reserved bits in
> certain system registers to get cascade working, but these should be
> treated as the abominations that they are rather than penalizing systems
> that aren't crap.
> 
> The pros of having all of this as platform data far outweigh any cons.
> The few systems that have a problem with this quite frankly brought it on
> themselves, and is nothing you should be concerning yourself with.
> 
> > Besides, these macros are also used by the legacy driver, so, we would end 
> > up with a duplication. They are also used to define TS_INDEX2VAL()...
> > 
> Duplication is fine. The legacy driver is its own thing entirely, nothing
> in the new driver should be pandering to it. The new driver gives us a
> chance to dump all of the legacy garbage and do things right, the fringe
> systems that are using the legacy bits can live with duplication. They're
> also unlikely to ever support the new driver.
> 
> > > > +enum {
> > > > +	XMIT_SZ_8BIT		= 0,
> > > > +	XMIT_SZ_16BIT		= 1,
> > > > +	XMIT_SZ_32BIT		= 2,
> > > > +	XMIT_SZ_64BIT		= 7,
> > > > +	XMIT_SZ_128BIT		= 3,
> > > > +	XMIT_SZ_256BIT		= 4,
> > > > +	XMIT_SZ_128BIT_BLK	= 0xb,
> > > > +	XMIT_SZ_256BIT_BLK	= 0xc,
> > > > +};
> > > > +
> > > > +/* log2(size / 8) - used to calculate number of transfers */
> > > > +#define TS_SHIFT {			\
> > > > +	[XMIT_SZ_8BIT]		= 0,	\
> > > > +	[XMIT_SZ_16BIT]		= 1,	\
> > > > +	[XMIT_SZ_32BIT]		= 2,	\
> > > > +	[XMIT_SZ_64BIT]		= 3,	\
> > > > +	[XMIT_SZ_128BIT]	= 4,	\
> > > > +	[XMIT_SZ_256BIT]	= 5,	\
> > > > +	[XMIT_SZ_128BIT_BLK]	= 4,	\
> > > > +	[XMIT_SZ_256BIT_BLK]	= 5,	\
> > > > +}
> > > > +
> > > > +#define TS_INDEX2VAL(i)	((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
> > > > +			 ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
> > > > +
> > > We see that the complex case is a superset, suggesting that it's
> > > worthwhile to make this common.
> > 
> > Not for all, SH4 has it different, there 0 means 64-byte transfers. But 
> > that's for the enum. The array itself is _obviously_ identical - it's just 
> > the bit-shift for each transfer size - just as my own comment is saying;) 
> > So, yes, that one can be made common.
> > 
> That's ok, as long as it's just one or two CPU families that have such
> variations we can just ifdef them in place. If it gets out of hand beyond
> that, then that too should be shoved in to platform data.
> 
> > > This XMIT_SZ_xxx enum should be moved in to the upper level so all CPUs
> > > have visibility of it, and at the same time should be converted to shifts
> > > to permit platform data to have an xmit_sz_valid bitmap or something
> > > similar.
> > 
> > I think, you misunderstood it. The enum lists CHCR TS bitfield values, so, 
> > we cannot arbitrarily define it. As for validity check - this array is 
> > only used at one 
> > 
> I think you misunderstood my comment. My point is that since the TS_SHIFT
> is effectively common we could just represent that with a bitmap and then
> convert the valid sizes to bit positions for indicating which of the
> values is valid for that CPU. A simple wrapper around that to map it out
> to the proper CHCR layout would then take care of handling CPU-specific
> differences dynamically, which in turn would permit us to kill off all of
> this crap from the headers.

Well, let's see: TS_INDEX2VAL() (and indirectly CHCR_TS_*_SHIFT), and 
XMIT_SZ_* are used to initialise .chcr field in platform arrays of struct 
sh_dmae_slave_config. So, unless we sant to calculate those fields at 
run-time, these have to stay macros. I can add a validity bitmask, but 
let's otherwise preserve macros, shall we? But if you say register layout 
can be different on different controllers, shall we also be prepared for 
different CHCR internal structure? Then yes, we have to pass all that info 
in platform data, including the ts_shift array. Shall we do that? And keep 
all the macros local to platform code.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers

[Paul Mundt]

On Mon, Feb 08, 2010 at 10:05:23AM +0100, Guennadi Liakhovetski wrote:
> On Mon, 8 Feb 2010, Paul Mundt wrote:
> > The shared interrupt scheme is to deal with CPUs that have multiple IRQ
> > vectors backed by a single masking source. Given that there's no ability
> > to mask one vector without disabling all of them, we only request_irq()
> > for each unique masking source and make it shared for the cases where
> > there are multiple vectors sharing it. This will always be CPU-specific,
> > so you need to check the intc tables for a specific subtype to figure out
> > what it can and can't support.
> > 
> > Comparing the interrupt numbers is not sufficient, since they'll be
> > different. The IRQs are all unique, and are not muxed, they just can't be
> > masked individually. We could pass the IRQ flow information in through
> > the IRQ resource (ie, IORESOURCE_IRQ | IRQF_SHARED), but more than likely
> > most CPUs are going to have this issue, so just using IRQF_SHARED as a
> > default should be fine, too.
> 
> The point is, that the original driver checked for the SHDMA_MIX_IRQ flag, 
> and if it was set, it used only one IRQ number for all channels and the 
> error interrupt. So, it affects not only whether IRQF_SHARED is used, but 
> also what IRQs are requested. If we use different IRQ numbers in platform 
> data, then we need a way to tell the driver, that only the first one has 
> to be used. Otherwise we can either specify the same IRQ number already in 
> platform data, or we do request those different IRQs, in which case the 
> intc driver will know to use the same mask bit, but then we don't need 
> IRQF_SHARED?
> 
I thought that's what I said? We only request_irq() once for a unique
masking source, yes. I don't see precisely what is difficult about this,
there are multiple IRQ resources that can be passed in, if we only get a
single one then we can infer that we only request_irq() once, and at the
same time we can derive the flow control information from the resource
flags.

Whether or not IRQF_SHARED needs to be used or not depends on the CPU
configuration. Just because the DMA events happen to share a masking
source doesn't mean that they are the only ones using it. Most of masking
sources relate to a bit range in a masking register somewhere, and
whatever those bit positions relate to is up to the hardware designers.
Many of the DMA IRQs will share a mask range with IRQs that have no
relationship to the DMA engine at all, so we have no choice but to permit
sharing of the mask source in those cases.

Only the CPU can know what sort of configuration it requires, which is
why the CPU setup code is the only place where the IRQ flow control
information should be specified. Infering too much from the driver side
is going to be pretty fragile, since the driver isn't going to have
enough information to really make flow control decisions on its own.

> > Internal register layout will never vary between platforms because the
> > controller itself is wholly a property of the SoC. Different on-chip
> > DMACs might have different layouts, but again, all of that is known at
> > the time that the CPU definitions are being written up.
> 
> Yes, sorry, s/per-platform/per-SoC/. And no, I wasn't aware, that 
> different DMAC instances on one SoC can have different register layouts. 
> Just like SCIFs then;)
> 
Fortunately they're a bit more consistent than SCI/SCIF! I don't think
any of the mass produced CPUs have had to suffer through this mixing and
matching thing yet, but it's certainly something we should try and be
prepared for.

> > I think you misunderstood my comment. My point is that since the TS_SHIFT
> > is effectively common we could just represent that with a bitmap and then
> > convert the valid sizes to bit positions for indicating which of the
> > values is valid for that CPU. A simple wrapper around that to map it out
> > to the proper CHCR layout would then take care of handling CPU-specific
> > differences dynamically, which in turn would permit us to kill off all of
> > this crap from the headers.
> 
> Well, let's see: TS_INDEX2VAL() (and indirectly CHCR_TS_*_SHIFT), and 
> XMIT_SZ_* are used to initialise .chcr field in platform arrays of struct 
> sh_dmae_slave_config. So, unless we sant to calculate those fields at 
> run-time, these have to stay macros.

Yes, this is why I mentioned several times that these should be
calculated dynamically. ie, runtime. Given that these macros are already
used in runtime calculation, it really doesn't matter if we make the CHCR
calculation a bit heavier, at the end of the day the ability to kill off
all of those macros makes things a lot more flexible and easier to
maintain going forward.

Having written most of those macros initially, I can assure you that they
were never intended to deal with subtype variations. The original DMA
code had precisely 2 variations, the SH-3 DMA controller and the SH-4.
The Dreamcast built on top of that, and then as soon as SH-4A started,
everyone decided to have a field day with the DMA blocks. None of the
original code was ever intended to deal with the sort of subtype
variation that we have to deal with today, and almost all of that code
predates things like the driver model and all of the DMA interfaces
besides the ISA one.

You really would have been better off just rm -fr'ing all of the old code
before starting out with this series, as you seem to have built up some
sort of attachment to legacy crap that wants to be killed off completely.
These macros are not something we want to preserve unless we really need
to. Making things easier on the legacy code and avoiding some duplication
to that extent is not sufficient grounds for keeping the macros alive.

> But if you say register layout can be different on different
> controllers, shall we also be prepared for different CHCR internal
> structure? Then yes, we have to pass all that info in platform data,
> including the ts_shift array. Shall we do that? And keep all the macros
> local to platform code.
> 
That's probably going to be the easiest solution, given that even with
CPU families we see variations between subtypes. The case of individual
CPU subtypes mixing and matching different versions of the same block is
a corner case, but if we can dynamically support something like that,
then we're in pretty good shape for any abomination the hardware folks
decide to throw at us.

You should be happy you got stuck with one of the moderately sane CPUs.. :-)