RE: [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA ofSuperH

[Sosnowski, Maciej]

iwamatsu.nobuhiro@xxxxxxxxxxx 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@xxxxxxxxxxx>
> Cc: Paul Mundt <lethal@xxxxxxxxxxxx>
> Cc: Haavard Skinnemoen <hskinnemoen@xxxxxxxxx>
> Cc: Maciej Sosnowski <maciej.sosnowski@xxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
>  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

> 
> +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.

> +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.

> +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.

> +	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?

> +	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?

> +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);
> +
> +	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?

> +	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.

> +	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?


--
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/