Re: [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH

[Nobuhiro Iwamatsu <iwamatsu.nobuhiro@renesas.com>]

Hi, Dan.

Thank you for your comments.

2009/3/17 Dan Williams <dan.j.williams@intel.com>:
 > On Wed, Mar 11, 2009 at 11:44 PM, Nobuhiro Iwamatsu
 > <iwamatsu.nobuhiro@renesas.com> 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 <iwamatsu.nobuhiro@renesas.com>
 >> Cc: Paul Mundt <lethal@linux-sh.org>
 >> Cc: Haavard Skinnemoen <hskinnemoen@atmel.com>
 >> Cc: Maciej Sosnowski <maciej.sosnowski@intel.com>
 >> Cc: Dan Williams <dan.j.williams@intel.com>
 >> ---
 >>  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,
 >
 > 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?

 >
 > 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
-- 
Nobuhiro Iwamatsu