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

[Nobuhiro Iwamatsu]

Hi, Maciej.

Thank you for your check.

2009/3/18 Sosnowski, Maciej <maciej.sosnowski@xxxxxxxxx>:
> > 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.
OK, I rewrite this help.

> >
>> >> +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.
I will fix these.

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

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

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

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

Your indication is right.
I fix this.

I fix your point, I resend.

Best regards,
 Nobuhiro

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