Messages in this thread | | | Date | Fri, 14 Nov 2008 17:34:52 +0100 | From | Nicolas Ferre <> | Subject | Re: [PATCH] dmaengine: at_hdmac: new driver for the Atmel AHB DMA Controller |
| |
Hi Dan,
Thanks a lot for your feedback.
Dan Williams : > On Fri, 2008-10-17 at 08:43 -0700, Nicolas Ferre wrote: >> This AHB DMA Controller (aka HDMA or DMAC on AT91 systems) is availlable on >> at91sam9rl chip. It will be used on other products in the future. >> >> This first release covers only the memory-to-memory tranfer type. This is the >> only tranfer type supported by this chip. >> On other products, it will be used also for peripheral DMA transfer (slave API >> support to come). >> >> I used dmatest client without problem in different configurations to test >> it. >> >> Full documentation for this controller can be found in the SAM9RL datasheet : >> http://www.atmel.com/dyn/products/product_card.asp?part_id=4243 >> >> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- > > Hi Nicolas, > > A few comments below. > > Also, checkpatch reported: > > total: 4 errors, 45 warnings, 1475 lines checked > > ...mostly 80 column warnings (some you may want to take a look at).
I reviewed this and manage to reduce most of 80 column warnings. An error remains but it is a space in "if" statement and it is for alignment purpose.
> Regards, > Dan > >> arch/arm/mach-at91/at91sam9rl_devices.c | 47 ++ >> drivers/dma/Kconfig | 8 + >> drivers/dma/Makefile | 1 + >> drivers/dma/at_hdmac.c | 989 +++++++++++++++++++++++++++++++ >> drivers/dma/at_hdmac_regs.h | 377 ++++++++++++ >> include/linux/at_hdmac.h | 26 + > > ...this header should be moved somewhere under arch/arm/include.
This is where dw_dmac.h resides. Moreover, if one day this IP is implemented on a different architecture, it will be good not to reach it through arch/arm path.
[..]
>> +/** >> + * atc_alloc_descriptor - allocate and return an initilized descriptor >> + * @chan: the channel to allocate descriptors for >> + * @gfp_flags: GFP allocation flags >> + */ >> +static struct at_desc *atc_alloc_descriptor(struct dma_chan *chan, >> + gfp_t gfp_flags) >> +{ >> + struct at_desc *desc = NULL; >> + struct at_dma *atdma = to_at_dma(chan->device); >> + dma_addr_t phys; >> + >> + desc = dma_pool_alloc(atdma->dma_desc_pool, gfp_flags, &phys); >> + if (desc) { >> + BUG_ON(phys & 0x3UL); /* descriptors have to be word aligned */ > > hmm, yes this is a bug but can't we trust that dma_pool_alloc does its > job correctly?
Indeed, it was mainly for debugging purpose, I remove it.
>> + memset(desc, 0, sizeof(struct at_desc)); >> + dma_async_tx_descriptor_init(&desc->txd, chan); >> + async_tx_ack(&desc->txd); > > the DMA_CTRL_ACK bit is under control of the client. It should be > read-only to the driver (except for extra descriptors that the driver > creates on behalf of the client).
This is precisely where the descriptors are been created so, I thought it should be ok to initialize this bit. Am I right ?
[..]
>> +static irqreturn_t at_dma_interrupt(int irq, void *dev_id) >> +{ >> + struct at_dma *atdma = (struct at_dma *)dev_id; >> + struct at_dma_chan *atchan; >> + int i; >> + u32 status, pending, imr; >> + int ret = IRQ_NONE; >> + >> + do { >> + imr = dma_readl(atdma, EBCIMR); >> + status = dma_readl(atdma, EBCISR); >> + pending = status & imr; >> + >> + if (!pending) >> + break; >> + >> + dev_vdbg(atdma->dma_common.dev, >> + "interrupt: status = 0x%08x, 0x%08x, 0x%08x\n", >> + status, imr, pending); >> + >> + for (i = 0; i < atdma->dma_common.chancnt; i++) { >> + atchan = &atdma->chan[i]; >> + if (pending & (AT_DMA_CBTC(i) | AT_DMA_ERR(i))) { >> + if (pending & AT_DMA_ERR(i)) { >> + /* >> + spin_lock(atchan->lock); >> + atchan->error_status = 1; >> + spin_unlock(atchan->lock); > > writing to an unsigned long should already be atomic, no?
On ARM yes, on other architectures, I do not know... Anyway, I removed those commented lines.
[..]
>> +/** >> + * atc_alloc_chan_resources - allocate resources for DMA channel >> + * @chan: allocate descriptor resources for this channel >> + * @client: current client requesting the channel be ready for requests >> + * >> + * return - the number of allocated descriptors >> + */ >> +static int atc_alloc_chan_resources(struct dma_chan *chan, >> + struct dma_client *client) >> +{ >> + struct at_dma_chan *atchan = to_at_dma_chan(chan); >> + struct at_dma *atdma = to_at_dma(chan->device); >> + struct at_desc *desc; >> + int i; >> + LIST_HEAD(tmp_list); >> + >> + dev_vdbg(&chan->dev, "alloc_chan_resources\n"); >> +
[TAG]
>> + /* ASSERT: channel is idle */ >> + if (atc_chan_is_enabled(atchan)) { >> + dev_dbg(&chan->dev, "DMA channel not idle ?\n"); >> + return -EIO; >> + }
[/TAG]
>> + >> + /* have we already been set up? */ >> + if (!list_empty(&atchan->free_list)) >> + return atchan->descs_allocated; >> + >> + /* Allocate initial pool of descriptors */ >> + for (i = 0; i < INIT_NR_DESCS_PER_CHANNEL; i++) { >> + desc = atc_alloc_descriptor(chan, GFP_KERNEL); >> + if (!desc) { >> + dev_err(atdma->dma_common.dev, >> + "Only %d initial descriptors\n", i); >> + break; >> + } >> + list_add_tail(&desc->desc_node, &tmp_list); >> + } >> + >> + spin_lock_bh(&atchan->lock); >> + atchan->descs_allocated = i; >> + list_splice(&tmp_list, &atchan->free_list); >> + atchan->completed_cookie = chan->cookie = 1; >> + spin_unlock_bh(&atchan->lock); >> + >> + /* channel parameters */ >> + channel_writel(atchan, CFG, ATC_DEFAULT_CFG); >> + >> + tasklet_init(&atchan->tasklet, atc_tasklet, (unsigned long)atchan); > > This routine may be called while the channel is already active, > potentially causing tasklet_init() to be called while a tasklet is > pending. Can this move to at_dma_probe()?
Oh, really ? In [TAG] above, I protect the call of this function when channel is enabled. Is the code at [TAG] ok ?
Ok, so I move all this.
>> + /* clear any pending interrupt */ >> + while (dma_readl(atdma, EBCISR)) >> + cpu_relax(); >> + atc_enable_irq(atchan); > > ditto.
Ok.
I will regenerate a new patch as soon as you acknowledge my comments.
Thanks for your help, kind regards, -- Nicolas Ferre
| |