Messages in this thread | | | From | "Sosnowski, Maciej" <> | Date | Wed, 18 Mar 2009 12:08:33 +0000 | Subject | RE: [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH |
| |
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,
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?
| |