Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754246AbbBTL0A (ORCPT ); Fri, 20 Feb 2015 06:26:00 -0500 Received: from eusmtp01.atmel.com ([212.144.249.242]:37059 "EHLO eusmtp01.atmel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753665AbbBTLZ6 (ORCPT ); Fri, 20 Feb 2015 06:25:58 -0500 Message-ID: <54E719C3.1000002@atmel.com> Date: Fri, 20 Feb 2015 12:25:55 +0100 From: Nicolas Ferre Organization: atmel User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Torsten Fleischer , , , , , Subject: Re: [PATCH v2 1/2] dma: at_hdmac: Fix calculation of the residual bytes References: <1424365673-7675-1-git-send-email-torfl6749@gmail.com> <1424365673-7675-2-git-send-email-torfl6749@gmail.com> In-Reply-To: <1424365673-7675-2-git-send-email-torfl6749@gmail.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.161.30.18] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14930 Lines: 463 Le 19/02/2015 18:07, Torsten Fleischer a ?crit : > From: Torsten Fleischer > > This patch fixes the following issues regarding to the calculation of the > residue: > > 1. The residue is always calculated for the current transfer even if the > cookie is associated to a pending transfer. > > 2. For scatter/gather DMA the calculation of the residue for the current > transfer doesn't include the bytes of the child descriptors that are already > transferred. > It only calculates the difference between the transfer's total length minus > the number of bytes that are already transferred for the current child > descriptor. > For example: There is a scatter/gather DMA transfer with a total length of > 1 MByte. Getting the residue several times while the transfer is running shows > something like that: > > 1: residue = 975584 > 2: residue = 1002766 > 3: residue = 992627 > 4: residue = 983767 > 5: residue = 985694 > 6: residue = 1008094 > 7: residue = 1009741 > 8: residue = 1011195 > > 3. The driver stores the residue but never resets it when starting a new > transfer. > For example: If there are two subsequent DMA transfers. The first one with > a total length of 1 MByte and the second one with a total length of 1 kByte. > Getting the residue for both transfers shows something like that: > > transfer 1: residue = 975584 > transfer 2: residue = 1048380 > > Changes from V1: > * Fixed coding style of the multi-line comments. > * Improved accuracy of the residue calculation when the transfer for the > first descriptor is active. > > Signed-off-by: Torsten Fleischer Torsten, Thanks a lot for your work and sorry for the delay. I just do an additional review after Ludovic's one. BTW, I think we can add his from the v1 of the series: Acked-by: Ludovic Desroches Anyway, I still have a few comments below... > --- > drivers/dma/at_hdmac.c | 155 +++++++++++++++++++++++--------------------- > drivers/dma/at_hdmac_regs.h | 11 ++-- > 2 files changed, 87 insertions(+), 79 deletions(-) > > diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c > index ca9dd26..f46d86d 100644 > --- a/drivers/dma/at_hdmac.c > +++ b/drivers/dma/at_hdmac.c > @@ -233,93 +233,104 @@ static void atc_dostart(struct at_dma_chan *atchan, struct at_desc *first) > } > > /* > - * atc_get_current_descriptors - > - * locate the descriptor which equal to physical address in DSCR > - * @atchan: the channel we want to start > - * @dscr_addr: physical descriptor address in DSCR > + * atc_get_desc_by_cookie - get the descriptor of a cookie > + * @atchan: the DMA channel > + * @cookie: the cookie to get the descriptor for > */ > -static struct at_desc *atc_get_current_descriptors(struct at_dma_chan *atchan, > - u32 dscr_addr) > +static struct at_desc *atc_get_desc_by_cookie(struct at_dma_chan *atchan, > + dma_cookie_t cookie) > { > - struct at_desc *desc, *_desc, *child, *desc_cur = NULL; > + struct at_desc *desc, *_desc; > > - list_for_each_entry_safe(desc, _desc, &atchan->active_list, desc_node) { > - if (desc->lli.dscr == dscr_addr) { > - desc_cur = desc; > - break; > - } > + list_for_each_entry_safe(desc, _desc, &atchan->queue, desc_node) { > + if (desc->txd.cookie == cookie) > + return desc; > + } > > - list_for_each_entry(child, &desc->tx_list, desc_node) { > - if (child->lli.dscr == dscr_addr) { > - desc_cur = child; > - break; > - } > - } > + list_for_each_entry_safe(desc, _desc, &atchan->active_list, desc_node) { > + if (desc->txd.cookie == cookie) > + return desc; > } > > - return desc_cur; > + return NULL; > } > > -/* > - * atc_get_bytes_left - > - * Get the number of bytes residue in dma buffer, > - * @chan: the channel we want to start > +/** > + * atc_get_bytes_left - get the number of bytes residue for a cookie > + * @chan: DMA channel > + * @cookie: transaction identifier to check status of > */ > -static int atc_get_bytes_left(struct dma_chan *chan) > +static int atc_get_bytes_left(struct dma_chan *chan, dma_cookie_t cookie) > { > struct at_dma_chan *atchan = to_at_dma_chan(chan); > - struct at_dma *atdma = to_at_dma(chan->device); > - int chan_id = atchan->chan_common.chan_id; > struct at_desc *desc_first = atc_first_active(atchan); > - struct at_desc *desc_cur; > + struct at_desc *desc; > int ret = 0, count = 0; > + u32 ctrla, dscr; > > /* > - * Initialize necessary values in the first time. > - * remain_desc record remain desc length. > + * If the cookie doesn't match to the currently running transfer then > + * we can return the total length of the associated DMA transfer, > + * because it is still queued. > */ > - if (atchan->remain_desc == 0) > - /* First descriptor embedds the transaction length */ > - atchan->remain_desc = desc_first->len; > + desc = atc_get_desc_by_cookie(atchan, cookie); > + if (desc == NULL) > + return -EINVAL; > + else if (desc != desc_first) > + return desc->total_len; > > - /* > - * This happens when current descriptor transfer complete. > - * The residual buffer size should reduce current descriptor length. > - */ > - if (unlikely(test_bit(ATC_IS_BTC, &atchan->status))) { > - clear_bit(ATC_IS_BTC, &atchan->status); > - desc_cur = atc_get_current_descriptors(atchan, > - channel_readl(atchan, DSCR)); > - if (!desc_cur) { > - ret = -EINVAL; > - goto out; > + /* cookie matches to the currently running transfer */ > + ret = desc_first->total_len; > + > + if (desc_first->lli.dscr) { > + /* hardware linked list transfer */ > + > + /* > + * Calculate the residue by removing the length of the child > + * descriptors already transferred from the total length. > + * To get the current child descriptor we can use the value of > + * the channel's DSCR register and compare it against the value > + * of the hardware linked list structure of each child > + * descriptor. > + */ > + > + ctrla = channel_readl(atchan, CTRLA); > + rmb(); /* ensure CTRLA is read before DSCR */ > + dscr = channel_readl(atchan, DSCR); > + > + /* for the first descriptor we can be more accurate */ > + if (desc_first->lli.dscr == dscr) { > + count = (ctrla & ATC_BTSIZE_MAX); > + ret -= count << ATC_REG_TO_SRC_WIDTH(ctrla); I see a little issue here. How are you sure that we must use the *src* width and not the *dst* width in this computation? As the register width is different depending on the transfer direction, I suspect the computation can be wrong in "slave_sg" case. So, I would recommend to keep the "tx_width" property in the descriptor and use it here. Apart from that, it seems that this computation or variants of it are done twice or maybe 3 times. So, do you think that we can extract a function from this code? > + return ret; > } > > - count = (desc_cur->lli.ctrla & ATC_BTSIZE_MAX) > - << desc_first->tx_width; > - if (atchan->remain_desc < count) { > - ret = -EINVAL; > - goto out; > + ret -= desc_first->len; > + list_for_each_entry(desc, &desc_first->tx_list, desc_node) { > + if (desc->lli.dscr == dscr) > + break; > + > + ret -= desc->len; > } > > - atchan->remain_desc -= count; > - ret = atchan->remain_desc; > - } else { > /* > - * Get residual bytes when current > - * descriptor transfer in progress. > + * For the last descriptor in the chain we can calculate > + * the remaining bytes using the channel's register. > */ > - count = (channel_readl(atchan, CTRLA) & ATC_BTSIZE_MAX) > - << (desc_first->tx_width); > - ret = atchan->remain_desc - count; > + if (!desc->lli.dscr) { > + ctrla = channel_readl(atchan, CTRLA); > + count = (desc->lli.ctrla & ATC_BTSIZE_MAX) - > + (ctrla & ATC_BTSIZE_MAX); > + > + ret = count << ATC_REG_TO_SRC_WIDTH(ctrla); Ditto. > + } > + } else { > + /* single transfer */ > + ctrla = channel_readl(atchan, CTRLA); > + count = (ctrla & ATC_BTSIZE_MAX) << ATC_REG_TO_SRC_WIDTH(ctrla); Ditto. > + ret -= count; > } > - /* > - * Check fifo empty. > - */ > - if (!(dma_readl(atdma, CHSR) & AT_DMA_EMPT(chan_id))) > - atc_issue_pending(chan); Yes, I've never understood why it was needed. > > -out: > return ret; > } > > @@ -534,8 +545,6 @@ static irqreturn_t at_dma_interrupt(int irq, void *dev_id) > /* Give information to tasklet */ > set_bit(ATC_IS_ERROR, &atchan->status); > } > - if (pending & AT_DMA_BTC(i)) > - set_bit(ATC_IS_BTC, &atchan->status); > tasklet_schedule(&atchan->tasklet); > ret = IRQ_HANDLED; > } > @@ -648,14 +657,14 @@ atc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, > desc->lli.ctrlb = ctrlb; > > desc->txd.cookie = 0; > + desc->len = xfer_count << src_width; > > atc_desc_chain(&first, &prev, desc); > } > > /* First descriptor of the chain embedds additional information */ > first->txd.cookie = -EBUSY; > - first->len = len; > - first->tx_width = src_width; Let's keep tx_width. > + first->total_len = len; > > /* set end-of-link to the last link descriptor of list*/ > set_desc_eol(desc); > @@ -747,6 +756,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > | ATC_SRC_WIDTH(mem_width) > | len >> mem_width; > desc->lli.ctrlb = ctrlb; > + desc->len = len; > > atc_desc_chain(&first, &prev, desc); > total_len += len; > @@ -787,6 +797,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > | ATC_DST_WIDTH(mem_width) > | len >> reg_width; > desc->lli.ctrlb = ctrlb; > + desc->len = len; > > atc_desc_chain(&first, &prev, desc); > total_len += len; > @@ -801,8 +812,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > > /* First descriptor of the chain embedds additional information */ > first->txd.cookie = -EBUSY; > - first->len = total_len; > - first->tx_width = reg_width; Let's keep tx_width: this is where tx_width can be different depending of the transfer direction. > + first->total_len = total_len; > > /* first link descriptor of list is responsible of flags */ > first->txd.flags = flags; /* client is in control of this ack */ > @@ -867,6 +877,7 @@ atc_dma_cyclic_fill_desc(struct dma_chan *chan, struct at_desc *desc, > | ATC_FC_MEM2PER > | ATC_SIF(atchan->mem_if) > | ATC_DIF(atchan->per_if); > + desc->len = period_len; > break; > > case DMA_DEV_TO_MEM: > @@ -878,6 +889,7 @@ atc_dma_cyclic_fill_desc(struct dma_chan *chan, struct at_desc *desc, > | ATC_FC_PER2MEM > | ATC_SIF(atchan->per_if) > | ATC_DIF(atchan->mem_if); > + desc->len = period_len; > break; > > default: > @@ -959,8 +971,7 @@ atc_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len, > > /* First descriptor of the chain embedds additional information */ > first->txd.cookie = -EBUSY; > - first->len = buf_len; > - first->tx_width = reg_width; Here again, let's keep tx_width. > + first->total_len = buf_len; > > return &first->txd; > > @@ -1091,7 +1102,7 @@ atc_tx_status(struct dma_chan *chan, > spin_lock_irqsave(&atchan->lock, flags); > > /* Get number of bytes left in the active transactions */ > - bytes = atc_get_bytes_left(chan); > + bytes = atc_get_bytes_left(chan, cookie); > > spin_unlock_irqrestore(&atchan->lock, flags); > > @@ -1187,7 +1198,6 @@ static int atc_alloc_chan_resources(struct dma_chan *chan) > > spin_lock_irqsave(&atchan->lock, flags); > atchan->descs_allocated = i; > - atchan->remain_desc = 0; Ok, this is not needed anymore: right. > list_splice(&tmp_list, &atchan->free_list); > dma_cookie_init(chan); > spin_unlock_irqrestore(&atchan->lock, flags); > @@ -1230,7 +1240,6 @@ static void atc_free_chan_resources(struct dma_chan *chan) > list_splice_init(&atchan->free_list, &list); > atchan->descs_allocated = 0; > atchan->status = 0; > - atchan->remain_desc = 0; > > dev_vdbg(chan2dev(chan), "free_chan_resources: done\n"); > } > diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h > index 2787aba..f458642 100644 > --- a/drivers/dma/at_hdmac_regs.h > +++ b/drivers/dma/at_hdmac_regs.h > @@ -112,11 +112,13 @@ > #define ATC_SRC_WIDTH_BYTE (0x0 << 24) > #define ATC_SRC_WIDTH_HALFWORD (0x1 << 24) > #define ATC_SRC_WIDTH_WORD (0x2 << 24) > +#define ATC_REG_TO_SRC_WIDTH(x) (((x) >> 24) & 0x3) Here... > #define ATC_DST_WIDTH_MASK (0x3 << 28) /* Destination Single Transfer Size */ > #define ATC_DST_WIDTH(x) ((x) << 28) > #define ATC_DST_WIDTH_BYTE (0x0 << 28) > #define ATC_DST_WIDTH_HALFWORD (0x1 << 28) > #define ATC_DST_WIDTH_WORD (0x2 << 28) > +#define ATC_REG_TO_DST_WIDTH(x) (((x) >> 28) & 0x3) ... and here, well, I'm not sure it can be statically called: so maybe these macros are not needed. > #define ATC_DONE (0x1 << 31) /* Tx Done (only written back in descriptor) */ > > /* Bitfields in CTRLB */ > @@ -181,8 +183,8 @@ struct at_lli { > * @at_lli: hardware lli structure > * @txd: support for the async_tx api > * @desc_node: node on the channed descriptors list > - * @len: total transaction bytecount Yes for the re-purpose of this. > - * @tx_width: transfer width Nack for this, sorry. > + * @len: descriptor byte count Ok. > + * @total_len: total transaction byte count Ok: this is clearer. > */ > struct at_desc { > /* FIRST values the hardware uses */ > @@ -193,7 +195,7 @@ struct at_desc { > struct dma_async_tx_descriptor txd; > struct list_head desc_node; > size_t len; > - u32 tx_width; > + size_t total_len; > }; > > static inline struct at_desc * > @@ -213,7 +215,6 @@ txd_to_at_desc(struct dma_async_tx_descriptor *txd) > enum atc_status { > ATC_IS_ERROR = 0, > ATC_IS_PAUSED = 1, > - ATC_IS_BTC = 2, Yes, it's seems not needed anymore. > ATC_IS_CYCLIC = 24, > }; > > @@ -231,7 +232,6 @@ enum atc_status { > * @save_cfg: configuration register that is saved on suspend/resume cycle > * @save_dscr: for cyclic operations, preserve next descriptor address in > * the cyclic list on suspend/resume cycle > - * @remain_desc: to save remain desc length > * @dma_sconfig: configuration for slave transfers, passed via DMA_SLAVE_CONFIG > * @lock: serializes enqueue/dequeue operations to descriptors lists > * @active_list: list of descriptors dmaengine is being running on > @@ -250,7 +250,6 @@ struct at_dma_chan { > struct tasklet_struct tasklet; > u32 save_cfg; > u32 save_dscr; > - u32 remain_desc; > struct dma_slave_config dma_sconfig; > > spinlock_t lock; > -- Nicolas Ferre -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/