Messages in this thread | | | Date | Wed, 06 Apr 2011 14:50:18 +0200 | From | Nicolas Ferre <> | Subject | Re: [PATCH 2/5] dmaengine: at_hdmac: add cyclic DMA operation support |
| |
Le 06/04/2011 12:08, Koul, Vinod : > On Fri, 2011-04-01 at 19:19 +0200, Nicolas Ferre wrote: >> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- >> drivers/dma/at_hdmac.c | 188 +++++++++++++++++++++++++++++++++++++++--- >> drivers/dma/at_hdmac_regs.h | 14 +++- >> 2 files changed, 186 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c >> index 13050e6..fe9e1de 100644 >> --- a/drivers/dma/at_hdmac.c >> +++ b/drivers/dma/at_hdmac.c >> @@ -14,6 +14,8 @@ >> * The driver has currently been tested with the Atmel AT91SAM9RL >> * and AT91SAM9G45 series. >> */ >> +#define DEBUG 12 >> +#define VERBOSE_DEBUG 12 > Please don't define here and use debug options available for build
Yes, sure sorry.
>> +/** >> + * atc_handle_cyclic - at the end of a period, run callback function >> + * @atchan: channel used for cyclic operations >> + * >> + * Called with atchan->lock held and bh disabled >> + */ >> +static void atc_handle_cyclic(struct at_dma_chan *atchan) >> +{ >> + struct at_desc *first = atc_first_active(atchan); >> + struct dma_async_tx_descriptor *txd = &first->txd; >> + dma_async_tx_callback callback = txd->callback; >> + void *param = txd->callback_param; >> + >> + dev_vdbg(chan2dev(&atchan->chan_common), >> + "new cyclic period llp 0x%08x\n", >> + channel_readl(atchan, DSCR)); >> + >> + if (callback) >> + callback(param); >> +} > You dont seem to be doing much expect calling callback, so doesn't it > make sense to write so much code for just calling callback?
I do not totally follow you here: you mean that I should reduce the amount of variables in this function or is it a more global comment about my way of handling cyclic operations?
>> >> /*-- IRQ & Tasklet ---------------------------------------------------*/ >> >> @@ -434,8 +459,10 @@ static void atc_tasklet(unsigned long data) >> } >> >> spin_lock(&atchan->lock); >> - if (test_and_clear_bit(0, &atchan->error_status)) >> + if (test_and_clear_bit(ATC_IS_ERROR, &atchan->status)) >> atc_handle_error(atchan); >> + else if (test_bit(ATC_IS_CYCLIC, &atchan->status)) >> + atc_handle_cyclic(atchan); >> else >> atc_advance_work(atchan); >> >> @@ -469,7 +496,7 @@ static irqreturn_t at_dma_interrupt(int irq, void *dev_id) >> /* Disable channel on AHB error */ >> dma_writel(atdma, CHDR, atchan->mask); >> /* Give information to tasklet */ >> - set_bit(0, &atchan->error_status); >> + set_bit(ATC_IS_ERROR, &atchan->status); >> } >> tasklet_schedule(&atchan->tasklet); >> ret = IRQ_HANDLED; >> @@ -759,6 +786,127 @@ err_desc_get: >> return NULL; >> } >> >> +/** >> + * atc_dma_cyclic_prep - prepare the cyclic DMA transfer >> + * @chan: the DMA channel to prepare >> + * @buf_addr: physical DMA address where the buffer starts >> + * @buf_len: total number of bytes for the entire buffer >> + * @period_len: number of bytes for each period >> + * @direction: transfer direction, to or from device >> + */ >> +static struct dma_async_tx_descriptor * >> +atc_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len, >> + size_t period_len, enum dma_data_direction direction) >> +{ >> + struct at_dma_chan *atchan = to_at_dma_chan(chan); >> + struct at_dma_slave *atslave = chan->private; >> + struct at_desc *first = NULL; >> + struct at_desc *prev = NULL; >> + unsigned long was_cyclic; >> + unsigned int periods = buf_len / period_len; >> + unsigned int reg_width; >> + u32 ctrla; >> + u32 ctrlb; >> + unsigned int i; >> + >> + dev_vdbg(chan2dev(chan), "prep_dma_cyclic: %s buf@0x%08x - %d (%d/%d)\n", >> + direction == DMA_TO_DEVICE ? "TO DEVICE" : "FROM DEVICE", >> + buf_addr, >> + periods, buf_len, period_len); >> + >> + if (unlikely(!atslave || !buf_len || !period_len)) { >> + dev_dbg(chan2dev(chan), "prep_dma_cyclic: length is zero!\n"); >> + return NULL; >> + } >> + >> + was_cyclic = test_and_set_bit(ATC_IS_CYCLIC, &atchan->status); >> + if (was_cyclic) { >> + dev_dbg(chan2dev(chan), "prep_dma_cyclic: channel in use!\n"); >> + return NULL; >> + } >> + >> + reg_width = atslave->reg_width; >> + >> + /* Check for too big/unaligned periods and unaligned DMA buffer */ >> + if (period_len > (ATC_BTSIZE_MAX << reg_width)) >> + goto err_out; >> + if (unlikely(period_len & ((1 << reg_width) - 1))) >> + goto err_out; >> + if (unlikely(buf_addr & ((1 << reg_width) - 1))) >> + goto err_out; >> + if (unlikely(!(direction & (DMA_TO_DEVICE | DMA_FROM_DEVICE)))) >> + goto err_out; >> + >> + /* prepare common CRTLA/CTRLB values */ >> + ctrla = ATC_DEFAULT_CTRLA | atslave->ctrla >> + | ATC_DST_WIDTH(reg_width) >> + | ATC_SRC_WIDTH(reg_width) >> + | period_len >> reg_width; >> + ctrlb = ATC_DEFAULT_CTRLB; >> + >> + /* build cyclic linked list */ >> + for (i = 0; i < periods; i++) { > would help making code look cleaner if you split this one to seprate > function
Ok, I will see what I can do... and will post a v2 soon.
>> + struct at_desc *desc; >> + >> + desc = atc_desc_get(atchan); >> + if (!desc) >> + goto err_desc_get; >> + >> + switch (direction) { >> + case DMA_TO_DEVICE: >> + desc->lli.saddr = buf_addr + (period_len * i); >> + desc->lli.daddr = atslave->tx_reg; >> + desc->lli.ctrla = ctrla; >> + desc->lli.ctrlb = ctrlb >> + | ATC_DST_ADDR_MODE_FIXED >> + | ATC_SRC_ADDR_MODE_INCR >> + | ATC_FC_MEM2PER; >> + break; >> + >> + case DMA_FROM_DEVICE: >> + desc->lli.saddr = atslave->rx_reg; >> + desc->lli.daddr = buf_addr + (period_len * i); >> + desc->lli.ctrla = ctrla; >> + desc->lli.ctrlb = ctrlb >> + | ATC_DST_ADDR_MODE_INCR >> + | ATC_SRC_ADDR_MODE_FIXED >> + | ATC_FC_PER2MEM; >> + break; >> + >> + default: >> + return NULL; >> + } >> + >> + if (!first) { >> + first = desc; >> + } else { >> + /* inform the HW lli about chaining */ >> + prev->lli.dscr = desc->txd.phys; >> + /* insert the link descriptor to the LD ring */ >> + list_add_tail(&desc->desc_node, >> + &first->tx_list); >> + } >> + prev = desc; >> + } >> + >> + /* lets make a cyclic list */ >> + prev->lli.dscr = first->txd.phys; >> + >> + /* First descriptor of the chain embedds additional information */ >> + first->txd.cookie = -EBUSY; >> + first->len = buf_len; >> + >> + return &first->txd; >> + >> +err_desc_get: >> + dev_err(chan2dev(chan), "not enough descriptors available\n"); >> + atc_desc_put(atchan, first); >> +err_out: >> + clear_bit(ATC_IS_CYCLIC, &atchan->status); >> + return NULL; >> +} >> + >> + >> static int atc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, >> unsigned long arg) >> { >> @@ -793,6 +941,9 @@ static int atc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, >> list_for_each_entry_safe(desc, _desc, &list, desc_node) >> atc_chain_complete(atchan, desc); >> >> + /* if channel dedicated to cyclic operations, free it */ >> + clear_bit(ATC_IS_CYCLIC, &atchan->status); >> + >> spin_unlock_bh(&atchan->lock); >> >> return 0; >> @@ -853,6 +1004,10 @@ static void atc_issue_pending(struct dma_chan *chan) >> >> dev_vdbg(chan2dev(chan), "issue_pending\n"); >> >> + /* Not needed for cyclic transfers */ >> + if (test_bit(ATC_IS_CYCLIC, &atchan->status)) >> + return; >> + >> spin_lock_bh(&atchan->lock); >> if (!atc_chan_is_enabled(atchan)) { >> atc_advance_work(atchan); >> @@ -1092,10 +1247,15 @@ static int __init at_dma_probe(struct platform_device *pdev) >> if (dma_has_cap(DMA_MEMCPY, atdma->dma_common.cap_mask)) >> atdma->dma_common.device_prep_dma_memcpy = atc_prep_dma_memcpy; >> >> - if (dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask)) { >> + if (dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask)) >> atdma->dma_common.device_prep_slave_sg = atc_prep_slave_sg; >> + >> + if (dma_has_cap(DMA_CYCLIC, atdma->dma_common.cap_mask)) >> + atdma->dma_common.device_prep_dma_cyclic = atc_prep_dma_cyclic; >> + >> + if (dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask) || >> + dma_has_cap(DMA_CYCLIC, atdma->dma_common.cap_mask)) >> atdma->dma_common.device_control = atc_control; >> - } >> >> dma_writel(atdma, EN, AT_DMA_ENABLE); >> >> diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h >> index 8303306..c79a9e0 100644 >> --- a/drivers/dma/at_hdmac_regs.h >> +++ b/drivers/dma/at_hdmac_regs.h >> @@ -181,12 +181,22 @@ txd_to_at_desc(struct dma_async_tx_descriptor *txd) >> /*-- Channels --------------------------------------------------------*/ >> >> /** >> + * atc_status - information bits stored in channel status flag >> + * >> + * Manipulated with atomic operations. >> + */ >> +enum atc_status { >> + ATC_IS_ERROR = 0, >> + ATC_IS_CYCLIC = 24, >> +}; >> + >> +/** >> * struct at_dma_chan - internal representation of an Atmel HDMAC channel >> * @chan_common: common dmaengine channel object members >> * @device: parent device >> * @ch_regs: memory mapped register base >> * @mask: channel index in a mask >> - * @error_status: transmit error status information from irq handler >> + * @status: transmit status information from irq/prep* functions >> * to tasklet (use atomic operations) >> * @tasklet: bottom half to finish transaction work >> * @lock: serializes enqueue/dequeue operations to descriptors lists >> @@ -201,7 +211,7 @@ struct at_dma_chan { >> struct at_dma *device; >> void __iomem *ch_regs; >> u8 mask; >> - unsigned long error_status; >> + unsigned long status; >> struct tasklet_struct tasklet; >> >> spinlock_t lock;
Thanks for your review, best regards, -- Nicolas Ferre
| |