From: "Sosnowski, Maciej" <maciej.sosnowski@intel.com> To: "iwamatsu.nobuhiro@renesas.com" <iwamatsu.nobuhiro@renesas.com> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "Williams, Dan J" <dan.j.williams@intel.com>, Paul Mundt <lethal@linux-sh.org>, Linux-sh <linux-sh@vger.kernel.org> Subject: RE: [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of Date: Wed, 18 Mar 2009 12:08:33 +0000 [thread overview] Message-ID: <129600E5E5FB004392DDC3FB599660D786B0DFC2@irsmsx504.ger.corp.intel.com> (raw) In-Reply-To: <49B8AF67.8080309@renesas.com> 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?
WARNING: multiple messages have this Message-ID (diff)
From: "Sosnowski, Maciej" <maciej.sosnowski@intel.com> To: "iwamatsu.nobuhiro@renesas.com" <iwamatsu.nobuhiro@renesas.com> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "Williams, Dan J" <dan.j.williams@intel.com>, Paul Mundt <lethal@linux-sh.org>, Linux-sh <linux-sh@vger.kernel.org> Subject: RE: [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH Date: Wed, 18 Mar 2009 12:08:33 +0000 [thread overview] Message-ID: <129600E5E5FB004392DDC3FB599660D786B0DFC2@irsmsx504.ger.corp.intel.com> (raw) In-Reply-To: <49B8AF67.8080309@renesas.com> 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?
next prev parent reply other threads:[~2009-03-18 12:08 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2009-03-12 6:44 [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH Nobuhiro Iwamatsu 2009-03-12 6:44 ` Nobuhiro Iwamatsu 2009-03-12 11:22 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA Matt Fleming 2009-03-12 11:22 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH Matt Fleming 2009-03-13 1:02 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of Nobuhiro Iwamatsu 2009-03-13 1:02 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH Nobuhiro Iwamatsu 2009-03-16 11:24 ` Paul Mundt 2009-03-16 11:24 ` Paul Mundt 2009-03-16 22:46 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of Dan Williams 2009-03-16 22:46 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH Dan Williams 2009-03-19 6:18 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of Nobuhiro Iwamatsu 2009-03-19 6:18 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH Nobuhiro Iwamatsu 2009-03-18 12:08 ` Sosnowski, Maciej [this message] 2009-03-18 12:08 ` Sosnowski, Maciej 2009-03-19 6:19 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of Nobuhiro Iwamatsu 2009-03-19 6:19 ` [PATCH] dmaengine: sh: Add support DMA-Engine driver for DMA of SuperH Nobuhiro Iwamatsu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=129600E5E5FB004392DDC3FB599660D786B0DFC2@irsmsx504.ger.corp.intel.com \ --to=maciej.sosnowski@intel.com \ --cc=dan.j.williams@intel.com \ --cc=iwamatsu.nobuhiro@renesas.com \ --cc=lethal@linux-sh.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sh@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.