Skip to contentSkip to conversation

dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH

PATCH Superseded Nobuhiro Iwamatsu 5 participants 8 messages Mar 12, 2009 – Mar 19, 2009

Subsystems (2)
SuperH dmaengine

v1 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Nobuhiro Iwamatsu

author
#0 lore raw

PATCH 1/1 dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH

This supports DMA-Engine driver for DMA of SuperH.
This supported all DMA channels, and it was tested in SH7722/SH7780. This can not use with SH DMA API and can control this in Kconfig.

Signed-off-by Nobuhiro Iwamatsu <[email protected]>
Cc Paul Mundt <[email protected]>
Cc Haavard Skinnemoen <[email protected]>
Cc Maciej Sosnowski <[email protected]>
Cc Dan Williams <[email protected]>
---
 arch/sh/drivers/dma/Kconfig  |   12 +-
 arch/sh/drivers/dma/Makefile |    3 +-
 arch/sh/include/asm/dma-sh.h |   11 +
 drivers/dma/Kconfig          |    9 +
 drivers/dma/Makefile         |    2 +
 drivers/dma/shdma.c          |  743 ++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/shdma.h          |   65 ++++
 7 files changed, 840 insertions(+), 5 deletions(-)
 create mode 100644 drivers/dma/shdma.c
 create mode 100644 drivers/dma/shdma.h
arch/sh/drivers/dma/Kconfig +9 -3
index 57d95fc..f556629 100644
@@ -1,14 +1,20 @@
 menu "DMA support"
 
-config SH_DMA_API
-	bool
 
 config SH_DMA
 	bool "SuperH on-chip DMA controller (DMAC) support"
 	depends on CPU_SH3 || CPU_SH4
-	select SH_DMA_API
 	default n
 
+config SH_DMA_API
+	depends on SH_DMA
+	bool "SuperH DMA API support"
+	default n
+	help
+	  SH_DMA_API always enabled DMA API of used SuperH.
+	  If you want to use DMA ENGINE, you must not enable this.
+	  Please enable DMA_ENGINE and SH_DMAE.
+
 config NR_ONCHIP_DMA_CHANNELS
 	int
 	depends on SH_DMA
arch/sh/drivers/dma/Makefile +1 -2
index ab956ad..c78ddd3 100644
@@ -2,7 +2,6 @@
 # Makefile for the SuperH DMA specific kernel interface routines under Linux.
 #
 
-obj-$(CONFIG_SH_DMA_API)	+= dma-api.o dma-sysfs.o
-obj-$(CONFIG_SH_DMA)		+= dma-sh.o
+obj-$(CONFIG_SH_DMA_API)	+= dma-sh.o dma-api.o dma-sysfs.o
 obj-$(CONFIG_SH_DREAMCAST)	+= dma-pvr2.o dma-g2.o
 obj-$(CONFIG_SH_DMABRG)		+= dmabrg.o
arch/sh/include/asm/dma-sh.h +11 -0
index 0c8f8e1..c356df4 100644
@@ -115,4 +115,15 @@ static u32 dma_base_addr[] __maybe_unused = {
 #define CHCR    0x0C
 #define DMAOR	0x40
 
+/* for dma engine */
+/* mode */
+#define SHDMA_MIX_IRQ	(1 << 1)
+#define SHDMA_DMAOR1	(1 << 2)
+#define SHDMA_DMAE1		(1 << 3)
+
+struct sh_dmae_pdata {
+	unsigned long int2b3;
+	unsigned int mode;
+};
+
 #endif /* __DMA_SH_H */
drivers/dma/Kconfig +9 -0
index 7f9e1ec..af74ff3 100644
@@ -81,6 +81,15 @@ config MX3_IPU_IRQS
 	  To avoid bloating the irq_desc[] array we allocate a sufficient
 	  number of IRQ slots and map them dynamically to specific sources.
 
+config SH_DMAE
+	tristate "Renesas SuperH DMAC support"
+	depends on SUPERH && SH_DMA
+	depends on !SH_DMA_API
+	select DMA_ENGINE
+	help
+	  There is SH_DMA_API which is another DMA implementation in SuperH.
+	  When you want to use this, please enable SH_DMA_API.
+
 config DMA_ENGINE
 	bool
drivers/dma/Makefile +2 -0
index 2e5dc96..0d2f3e9 100644
@@ -8,3 +8,5 @@ obj-$(CONFIG_FSL_DMA) += fsldma.o
 obj-$(CONFIG_MV_XOR) += mv_xor.o
 obj-$(CONFIG_DW_DMAC) += dw_dmac.o
 obj-$(CONFIG_MX3_IPU) += ipu/
+obj-$(CONFIG_SH_DMAE) += shdma.o
+
drivers/dma/shdma.c new file +741 -0

741 lines not on this page · raw message

drivers/dma/shdma.h new file +65 -0

65 lines not on this page · raw message

Signature · 1 line
1.6.2
Reviewed-by on v1 1/1
#1 lore raw
Quoted 4 lines · Nobuhiro Iwamatsu

This supports DMA-Engine driver for DMA of SuperH.
This supported all DMA channels, and it was tested in SH7722/SH7780. This can not use with SH DMA API and can control this in Kconfig.

I was working on support for this, but fortunately for me you beat me to a finished implementation ;-)

The code looks good to me. Just a few (very very) minor nits.

Reviewed-by Matt Fleming <[email protected]>

[...]

arch/sh/include/asm/dma-sh.h static u32 dma_base_addr[] __maybe_unused = { · line 120

Quoted 4 lines
arch/sh/include/asm/dma-sh.h +3 -0
+/* for dma engine */
+/* mode */
+#define SHDMA_MIX_IRQ	(1 << 1)

The proper multi-line kernel comment style is

	/*
	 *
	 */

though this comment can all fit on one line.

drivers/dma/shdma.c line 71

Quoted 9 lines
drivers/dma/shdma.c new file +9 -0
+
+static void dmae_init(struct sh_dmae_chan *sh_chan)
+{
+	u32 chcr = RS_DEFAULT; /* default is DUAL mode */
+	sh_dmae_writel(sh_chan, chcr, CHCR);
+}
+
+/*
+ * Reset DMA controler

controler -> controller

Good work!

author
#2 lore raw

Hi, Matt.

Thank you for your review.

Quoted 21 lines · Matt Fleming
Quoted 4 lines · Nobuhiro Iwamatsu

This supports DMA-Engine driver for DMA of SuperH.
This supported all DMA channels, and it was tested in SH7722/SH7780. This can not use with SH DMA API and can control this in Kconfig.

I was working on support for this, but fortunately for me you beat me to a finished implementation ;-)

The code looks good to me. Just a few (very very) minor nits.

Reviewed-by Matt Fleming <[email protected]>

[...]

arch/sh/include/asm/dma-sh.h static u32 dma_base_addr[] __maybe_unused = { · line 120

Quoted 3 lines
arch/sh/include/asm/dma-sh.h +3 -0
+/* for dma engine */
+/* mode */
+#define SHDMA_MIX_IRQ	(1 << 1)

The proper multi-line kernel comment style is

Quoted 6 lines
	/*
	 *
	 */

though this comment can all fit on one line.

OK, I fix it.

Quoted 12 lines

drivers/dma/shdma.c line 71

Quoted 9 lines
drivers/dma/shdma.c new file +9 -0
+
+static void dmae_init(struct sh_dmae_chan *sh_chan)
+{
+	u32 chcr = RS_DEFAULT; /* default is DUAL mode */
+	sh_dmae_writel(sh_chan, chcr, CHCR);
+}
+
+/*
+ * Reset DMA controler

controler -> controller

Oops, thank you.

I fix your check point and resend patch.

Best regards,
Nobuhiro

#3 lore raw
In reply to Nobuhiro Iwamatsu
Quoted 9 lines · Nobuhiro Iwamatsu

This supports DMA-Engine driver for DMA of SuperH.
This supported all DMA channels, and it was tested in SH7722/SH7780. This can not use with SH DMA API and can control this in Kconfig.

Signed-off-by Nobuhiro Iwamatsu <[email protected]>
Cc Paul Mundt <[email protected]>
Cc Haavard Skinnemoen <[email protected]>
Cc Maciej Sosnowski <[email protected]>
Cc Dan Williams <[email protected]>

The updated version looks ok to me. If no one else has any issues with it, and someone from the dmaengine side wants to provide an Acked-by, I can take it through my tree.

#4 lore raw
In reply to Nobuhiro Iwamatsu
Quoted 17 lines · Nobuhiro Iwamatsu

This supports DMA-Engine driver for DMA of SuperH.
This supported all DMA channels, and it was tested in SH7722/SH7780. This can not use with SH DMA API and can control this in Kconfig.

Signed-off-by Nobuhiro Iwamatsu <[email protected]>
Cc Paul Mundt <[email protected]>
Cc Haavard Skinnemoen <[email protected]>
Cc Maciej Sosnowski <[email protected]>
Cc Dan Williams <[email protected]>

---
 arch/sh/drivers/dma/Kconfig  |   12 +- arch/sh/drivers/dma/Makefile |    3 +- arch/sh/include/asm/dma-sh.h |   11 +
 drivers/dma/Kconfig          |    9 +
 drivers/dma/Makefile         |    2 +
 drivers/dma/shdma.c          |  743 ++++++++++++++++++++++++++++++++++++++++++ drivers/dma/shdma.h          |   65 ++++

Quoted 3 lines
 7 files changed, 840 insertions(+), 5 deletions(-)

 create mode 100644 drivers/dma/shdma.c
 create mode 100644 drivers/dma/shdma.h

Hi,

I have not finished a full review but one problem jumps out, the use of spin_lock_irqsave to protect against channel/descriptor manipulations. The highest level of protection that net_dma and async_tx assume is spin_lock_bh. It seems like the pieces of sh_dmae_interrupt() that touch the descriptor can be moved to the tasklet, then the locks can be downgraded.

Your other patch, to set the alignment in dmatest, makes me wonder if this engine can handle unaligned accesses? If it can not then set the DMA_PRIVATE capability bit at device registration time to prevent net_dma and other "public" clients from using these channels. Public clients assume that there are no alignment constraints.

Thanks,
Dan

author
#5 lore raw

Hi, Dan.

Thank you for your comments.

2009/3/17 Dan Williams <[email protected]>:

Quoted 23 lines
Quoted 9 lines · Nobuhiro Iwamatsu

This supports DMA-Engine driver for DMA of SuperH.
This supported all DMA channels, and it was tested in SH7722/SH7780. This can not use with SH DMA API and can control this in Kconfig.

Signed-off-by Nobuhiro Iwamatsu <[email protected]>
Cc Paul Mundt <[email protected]>
Cc Haavard Skinnemoen <[email protected]>
Cc Maciej Sosnowski <[email protected]>
Cc Dan Williams <[email protected]>
Quoted 11 lines
---
 arch/sh/drivers/dma/Kconfig  |   12 +-
 arch/sh/drivers/dma/Makefile |    3 +-
 arch/sh/include/asm/dma-sh.h |   11 +
 drivers/dma/Kconfig          |    9 +
 drivers/dma/Makefile         |    2 +
 drivers/dma/shdma.c          |  743 ++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/shdma.h          |   65 ++++
 7 files changed, 840 insertions(+), 5 deletions(-)
 create mode 100644 drivers/dma/shdma.c
 create mode 100644 drivers/dma/shdma.h
Quoted 8 lines

Hi,

I have not finished a full review but one problem jumps out, the use of spin_lock_irqsave to protect against channel/descriptor manipulations. The highest level of protection that net_dma and async_tx assume is spin_lock_bh. It seems like the pieces of sh_dmae_interrupt() that touch the descriptor can be moved to the tasklet, then the locks can be downgraded.

Because a dmaengine core is not equivalent to the interrupt that is severer than spin_lock_bh, is this to rearrange it in tasklet?

Quoted 7 lines

Your other patch, to set the alignment in dmatest, makes me wonder if this engine can handle unaligned accesses? If it can not then set the DMA_PRIVATE capability bit at device registration time to prevent net_dma and other "public" clients from using these channels. Public clients assume that there are no alignment constraints.

I thought to add it because the patch wanted to measure the transaction speed with the thing which
was not considered to be aligned address / data size. Depending on a device using DMA, there is the thing forcing aligning it of forwarded data.
DMAC of SH has a register appointing transfer data size. I control it by the following functions.

+struct sh_dmae_chan {
+ dma_cookie_t completed_cookie; /* The maximum cookie completed */
+ spinlock_t desc_lock; /* Descriptor operation lock */
+ struct list_head ld_queue; /* Link descriptors queue */
+ struct dma_chan common; /* DMA common channel */
+ struct device *dev; /* Channel device */
+ struct resource reg; /* Resource for register */
+ struct tasklet_struct tasklet;
+ int id; /* Raw id of this channel */
+ char dev_id[16]; /* unique name per DMAC of channel */
+
+ /* Set chcr */
+ int (*set_chcr)(struct sh_dmae_chan *sh_chan, u32 regs);

This set up call from a device using dmaengine, and data appoint aligning it.

Best regards,
Nobuhiro

Signature · 1 line
Nobuhiro Iwamatsu
#6 lore raw
In reply to Nobuhiro Iwamatsu
Quoted 9 lines · [email protected] wrote:

This supports DMA-Engine driver for DMA of SuperH.
This supported all DMA channels, and it was tested in SH7722/SH7780. This can not use with SH DMA API and can control this in Kconfig.

Signed-off-by Nobuhiro Iwamatsu <[email protected]>
Cc Paul Mundt <[email protected]>
Cc Haavard Skinnemoen <[email protected]>
Cc Maciej Sosnowski <[email protected]>
Cc Dan Williams <[email protected]>
Quoted 11 lines
---
 arch/sh/drivers/dma/Kconfig  |   12 +-
 arch/sh/drivers/dma/Makefile |    3 +-
 arch/sh/include/asm/dma-sh.h |   11 +
 drivers/dma/Kconfig          |    9 +
 drivers/dma/Makefile         |    2 +
 drivers/dma/shdma.c          |  743  ++++++++++++++++++++++++++++++++++++++++++ 
 drivers/dma/shdma.h        |   65 ++++
 7 files changed, 840 insertions(+), 5 deletions(-)
 create mode 100644 drivers/dma/shdma.c
 create mode 100644 drivers/dma/shdma.h

Hi,

My comments/questions below inline.

Regards,
Maciej

drivers/dma/Kconfig config MX3_IPU_IRQS · line 92

Quoted 10 lines
drivers/dma/Kconfig +9 -0
+config SH_DMAE
+	tristate "Renesas SuperH DMAC support"
+	depends on SUPERH && SH_DMA
+	depends on !SH_DMA_API
+	select DMA_ENGINE
+	help
+	  There is SH_DMA_API which is another DMA implementation in SuperH.
+	  When you want to use this, please enable SH_DMA_API.
+

This help comment may be confusing. It is not quite clear if "this" refers to SH_DMA_API or SH_DMAE. I suggest rephrasing it.

drivers/dma/shdma.c line 127

Quoted 7 lines
drivers/dma/shdma.c new file +7 -0
+static void dmae_start(struct sh_dmae_chan *sh_chan)
+{
+    u32 chcr = sh_dmae_readl(sh_chan, CHCR);
+
+    chcr |= (CHCR_DE|CHCR_IE);
+	sh_dmae_writel(sh_chan, chcr, CHCR);
+}

Whitespace issues to be cleaned.

drivers/dma/shdma.c line 134

Quoted 6 lines
drivers/dma/shdma.c new file +6 -0
+static void dmae_halt(struct sh_dmae_chan *sh_chan)
+{
+    u32 chcr = sh_dmae_readl(sh_chan, CHCR);
+    chcr &= ~(CHCR_DE | CHCR_TE | CHCR_IE);
+	sh_dmae_writel(sh_chan, chcr, CHCR);
+}

Again whitespace issues.

drivers/dma/shdma.c line 307

Quoted 5 lines
drivers/dma/shdma.c new file +5 -0
+	sh_chan->set_chcr = dmae_set_chcr;
+	sh_chan->set_dmars = dmae_set_dmars;
+
+	return first ? &first->async_tx : NULL;
+}

Why both set_chcr and set_dmars are set in prep_memcpy?
I guess it would be more efficient to set them in a per channel call, like sh_dmae_chan_probe. BTW, I do not see any of these two calls used anywhere.
Are they really needed here?

drivers/dma/shdma.c line 377

Quoted 9 lines
drivers/dma/shdma.c new file +9 -0
+	spin_unlock_irqrestore(&sh_chan->desc_lock, flags);
+
+	if (ld_node != &sh_chan->ld_queue) {
+		/* Get the ld start address from ld_queue */
+		hw = to_sh_desc(ld_node)->hw;
+		dmae_set_reg(sh_chan, hw);
+		dmae_start(sh_chan);
+	}
+}

Shouldn't this part be kept locked?

Quoted 6 lines
drivers/dma/shdma.c new file +6 -0
+static irqreturn_t sh_dmae_interrupt(int irq, void *data)
+{
+	irqreturn_t ret = IRQ_NONE;
+	struct sh_dmae_chan *sh_chan = (struct sh_dmae_chan *)data;
+	u32 chcr = sh_dmae_readl(sh_chan, CHCR);
+

drivers/dma/shdma.c line 426

Quoted 10 lines
drivers/dma/shdma.c new file +10 -0
+	if (chcr & CHCR_TE) {
+		struct sh_desc *desc, *cur_desc = NULL;
+		u32 sar_buf = sh_dmae_readl(sh_chan, SAR);
+
+		list_for_each_entry(desc, &sh_chan->ld_queue, node) {
+			if ((desc->hw.sar + desc->hw.tcr) = sar_buf) {
+				cur_desc = desc;
+				break;
+			}
+		}

Again, don't you need to lock list_for... to protect ld_queue?

drivers/dma/shdma.c line 636

Quoted 11 lines
drivers/dma/shdma.c new file +11 -0
+	shdev->dev = &pdev->dev;
+	INIT_LIST_HEAD(&shdev->common.channels);
+
+	dma_cap_set(DMA_MEMCPY, shdev->common.cap_mask);
+	shdev->common.device_alloc_chan_resources
+		= sh_dmae_alloc_chan_resources;
+	shdev->common.device_free_chan_resources = sh_dmae_free_chan_resources;
+	shdev->common.device_prep_dma_memcpy = sh_dmae_prep_memcpy;
+	shdev->common.device_is_tx_complete = sh_dmae_is_complete;
+	shdev->common.device_issue_pending = sh_dmae_memcpy_issue_pending;
+	shdev->common.dev = &pdev->dev;

shdev->dev seems redundant as you already keep the device in shdev->common.dev.

drivers/dma/shdma.c line 656

Quoted 10 lines
drivers/dma/shdma.c new file +10 -0
+	for (ecnt = 0 ; ecnt < ARRAY_SIZE(eirq); ecnt++) {
+		err = request_irq(eirq[ecnt], sh_dmae_err,
+			irqflags, "DMAC Address Error", shdev);
+		if (err) {
+			dev_err(&pdev->dev, "DMA device request_irq"
+				"erro (irq %d) with return %d\n",
+				eirq[ecnt], err);
+			goto eirq_err;
+		}
+	}

Don't you need to keep it in
#if defined(CONFIG_CPU_SH4)
as sh_dmae_err definition is?

author
#7 lore raw

Hi, Maciej.

Thank you for your check.

2009/3/18 Sosnowski, Maciej <[email protected]>:

Quoted 30 lines
Quoted 30 lines
Quoted 4 lines · [email protected] wrote:
Quoted 4 lines

This supports DMA-Engine driver for DMA of SuperH.
This supported all DMA channels, and it was tested in SH7722/SH7780. This can not use with SH DMA API and can control this in Kconfig.

Quoted 16 lines
Quoted 5 lines
Signed-off-by Nobuhiro Iwamatsu <[email protected]>
Cc Paul Mundt <[email protected]>
Cc Haavard Skinnemoen <[email protected]>
Cc Maciej Sosnowski <[email protected]>
Cc Dan Williams <[email protected]>
Quoted 11 lines
---
 arch/sh/drivers/dma/Kconfig  |   12 +-
 arch/sh/drivers/dma/Makefile |    3 +-
 arch/sh/include/asm/dma-sh.h |   11 +
 drivers/dma/Kconfig          |    9 +
 drivers/dma/Makefile         |    2 +
 drivers/dma/shdma.c          |  743  ++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/shdma.h        |   65 ++++
 7 files changed, 840 insertions(+), 5 deletions(-)
 create mode 100644 drivers/dma/shdma.c
 create mode 100644 drivers/dma/shdma.h

Hi,

My comments/questions below inline.

Regards,
Maciej

Quoted 1 line
Quoted 1 line
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/dma/Kconfig +4 -0
+config SH_DMAE
+     tristate "Renesas SuperH DMAC support"
+     depends on SUPERH && SH_DMA
+     depends on !SH_DMA_API
Quoted 8 lines
Quoted 5 lines

drivers/dma/Kconfig config MX3_IPU_IRQS · line 92

Quoted 5 lines
drivers/dma/Kconfig +5 -0
+     select DMA_ENGINE
+     help
+       There is SH_DMA_API which is another DMA implementation in SuperH.
+       When you want to use this, please enable SH_DMA_API.
+

This help comment may be confusing. It is not quite clear if "this" refers to SH_DMA_API or SH_DMAE. I suggest rephrasing it.

OK, I rewrite this help.

Quoted 11 lines
Quoted 11 lines
Quoted 8 lines

drivers/dma/shdma.c line 127

Quoted 8 lines
drivers/dma/shdma.c new file +7 -0
+static void dmae_start(struct sh_dmae_chan *sh_chan)
+{
+    u32 chcr = sh_dmae_readl(sh_chan, CHCR);
+
+    chcr |= (CHCR_DE|CHCR_IE);
+     sh_dmae_writel(sh_chan, chcr, CHCR);
+}

Whitespace issues to be cleaned.

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines

drivers/dma/shdma.c line 134

Quoted 6 lines
drivers/dma/shdma.c new file +6 -0
+static void dmae_halt(struct sh_dmae_chan *sh_chan)
+{
+    u32 chcr = sh_dmae_readl(sh_chan, CHCR);
+    chcr &= ~(CHCR_DE | CHCR_TE | CHCR_IE);
+     sh_dmae_writel(sh_chan, chcr, CHCR);
+}

Again whitespace issues.

I will fix these.

Quoted 9 lines
Quoted 9 lines
Quoted 6 lines

drivers/dma/shdma.c line 307

Quoted 6 lines
drivers/dma/shdma.c new file +5 -0
+     sh_chan->set_chcr = dmae_set_chcr;
+     sh_chan->set_dmars = dmae_set_dmars;
+
+     return first ? &first->async_tx : NULL;
+}

Why both set_chcr and set_dmars are set in prep_memcpy?
I guess it would be more efficient to set them in a per channel call, like sh_dmae_chan_probe.

It is not surely necessary to set these in prep_memcpy. I fix this.

Quoted 2 lines
Quoted 2 lines

BTW, I do not see any of these two calls used anywhere. Are they really needed here?

DMAC of superH has register that control DMA transfer size and other. I use these to initialize DMAC.
For example, it is DMA data size or a transfer method.

For example, driver uses it as follows.
----
struct dma_chan *chan = dma_request_channel();
....
sh_chan = to_sh_chan(chan);
sh_chan->set_dmars();
----

I made it such an implementation to operate a function peculiar to DMAC, but is there the method that, besides, is good?
Please teach it if there is it.

Quoted 12 lines
Quoted 12 lines
Quoted 10 lines

drivers/dma/shdma.c line 377

Quoted 10 lines
drivers/dma/shdma.c new file +9 -0
+     spin_unlock_irqrestore(&sh_chan->desc_lock, flags);
+
+     if (ld_node != &sh_chan->ld_queue) {
+             /* Get the ld start address from ld_queue */
+             hw = to_sh_desc(ld_node)->hw;
+             dmae_set_reg(sh_chan, hw);
+             dmae_start(sh_chan);
+     }
+}

Shouldn't this part be kept locked?

Oops, It mistake.
I fix this.

Quoted 45 lines
Quoted 45 lines
Quoted 17 lines
Quoted 7 lines
drivers/dma/shdma.c new file +6 -0
+static irqreturn_t sh_dmae_interrupt(int irq, void *data)
+{
+     irqreturn_t ret = IRQ_NONE;
+     struct sh_dmae_chan *sh_chan = (struct sh_dmae_chan *)data;
+     u32 chcr = sh_dmae_readl(sh_chan, CHCR);
+

drivers/dma/shdma.c line 426

Quoted 10 lines
drivers/dma/shdma.c new file +10 -0
+     if (chcr & CHCR_TE) {
+             struct sh_desc *desc, *cur_desc = NULL;
+             u32 sar_buf = sh_dmae_readl(sh_chan, SAR);
+
+             list_for_each_entry(desc, &sh_chan->ld_queue, node) {
+                     if ((desc->hw.sar + desc->hw.tcr) = sar_buf) {
+                             cur_desc = desc;
+                             break;
+                     }
+             }

Again, don't you need to lock list_for... to protect ld_queue?

Quoted 11 lines

drivers/dma/shdma.c line 636

Quoted 11 lines
drivers/dma/shdma.c new file +11 -0
+     shdev->dev = &pdev->dev;
+     INIT_LIST_HEAD(&shdev->common.channels);
+
+     dma_cap_set(DMA_MEMCPY, shdev->common.cap_mask);
+     shdev->common.device_alloc_chan_resources
+             = sh_dmae_alloc_chan_resources;
+     shdev->common.device_free_chan_resources = sh_dmae_free_chan_resources;
+     shdev->common.device_prep_dma_memcpy = sh_dmae_prep_memcpy;
+     shdev->common.device_is_tx_complete = sh_dmae_is_complete;
+     shdev->common.device_issue_pending = sh_dmae_memcpy_issue_pending;
+     shdev->common.dev = &pdev->dev;

shdev->dev seems redundant as you already keep the device in shdev->common.dev.

Quoted 10 lines
Quoted 10 lines
drivers/dma/shdma.c new file +10 -0
+     for (ecnt = 0 ; ecnt < ARRAY_SIZE(eirq); ecnt++) {
+             err = request_irq(eirq[ecnt], sh_dmae_err,
+                     irqflags, "DMAC Address Error", shdev);
+             if (err) {
+                     dev_err(&pdev->dev, "DMA device request_irq"
+                             "erro (irq %d) with return %d\n",
+                             eirq[ecnt], err);
+                     goto eirq_err;
+             }
+     }
Quoted 3 lines
Quoted 3 lines

Don't you need to keep it in
#if defined(CONFIG_CPU_SH4)
as sh_dmae_err definition is?

Your indication is right.
I fix this.

I fix your point, I resend.

Best regards,
Nobuhiro

-- Nobuhiro Iwamatsu