Hi,
This series allows to check DMA transfer residue (number of bytes left
to send/receive) by modifying pl330_tx_status() function, when struct
dma_tx_state is filled with needed data. It also introduces DMA_PAUSE
feature, which allows to halt DMA transfer before termination and read
residue without risk of data loss.
This features are needed for proper implementation of DMA transfers,
particulary for serial drivers when transfer sizes are unknown and
requests on DMA channels are terminated before transfer completion
very ofter (it's becouse we terminate then in timeout interrupt to
avoid latency which is usually undesirable).
Best regards,
Robert Baldyga
Robert Baldyga (2):
dma: pl330: improve pl330_tx_status() function
dma: pl330: add DMA_PAUSE feature
drivers/dma/pl330.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 73 insertions(+), 1 deletion(-)
--
1.9.1
This patch adds possibility to read residue of DMA transfer. It's useful
when we want to know how many bytes have been transfered before we
terminate channel. It can take place, for example, on timeout interrupt.
Signed-off-by: Lukasz Czerwinski <[email protected]>
Signed-off-by: Robert Baldyga <[email protected]>
---
drivers/dma/pl330.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index d5149aa..c32806d 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -437,6 +437,7 @@ struct dma_pl330_chan {
/* For D-to-M and M-to-D channels */
int burst_sz; /* the peripheral fifo width */
int burst_len; /* the number of burst */
+ int transfered;
dma_addr_t fifo_addr;
/* for cyclic capability */
@@ -500,6 +501,9 @@ struct dma_pl330_desc {
enum desc_status status;
+ int bytes_requested;
+ int direction;
+
/* The channel which currently holds this desc */
struct dma_pl330_chan *pchan;
@@ -2156,11 +2160,60 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
spin_unlock_irqrestore(&pch->lock, flags);
}
+int pl330_get_current_xferred_count(struct dma_pl330_chan *pch,
+ struct dma_pl330_desc *desc)
+{
+ u32 val, addr;
+ struct pl330_thread *thrd = pch->thread;
+ void __iomem *regs = thrd->dmac->base;
+
+ val = addr = 0;
+ switch (desc->direction) {
+ case DMA_MEM_TO_DEV:
+ val = readl(regs + SA(thrd->id));
+ addr = desc->px.src_addr;
+ break;
+ case DMA_DEV_TO_MEM:
+ val = readl(regs + DA(thrd->id));
+ addr = desc->px.dst_addr;
+ break;
+ default:
+ break;
+ }
+ return val - addr;
+}
+
static enum dma_status
pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
struct dma_tx_state *txstate)
{
- return dma_cookie_status(chan, cookie, txstate);
+ enum dma_status ret;
+ unsigned long flags;
+ struct dma_pl330_desc *desc;
+ struct dma_pl330_chan *pch = to_pchan(chan);
+ unsigned int bytes_transferred;
+ unsigned int residual;
+
+ /* Check in pending list */
+ spin_lock_irqsave(&pch->lock, flags);
+ list_for_each_entry(desc, &pch->work_list, node) {
+ if (desc->txd.cookie == cookie) {
+ bytes_transferred =
+ pl330_get_current_xferred_count(pch, desc);
+ residual = desc->bytes_requested -
+ bytes_transferred % desc->bytes_requested;
+ dma_set_residue(txstate, residual);
+ ret = desc->status;
+ spin_unlock_irqrestore(&pch->lock, flags);
+ return ret;
+ }
+ }
+ spin_unlock_irqrestore(&pch->lock, flags);
+
+ ret = dma_cookie_status(chan, cookie, txstate);
+ dma_set_residue(txstate, pch->transfered);
+
+ return ret;
}
static void pl330_issue_pending(struct dma_chan *chan)
@@ -2421,10 +2474,13 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
break;
}
+ desc->direction = direction;
desc->rqtype = direction;
desc->rqcfg.brst_size = pch->burst_sz;
desc->rqcfg.brst_len = 1;
+ desc->bytes_requested = period_len;
fill_px(&desc->px, dst, src, period_len);
+ pch->transfered = 0;
if (!first)
first = desc;
@@ -2554,9 +2610,12 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
sg_dma_address(sg), addr, sg_dma_len(sg));
}
+ desc->direction = direction;
desc->rqcfg.brst_size = pch->burst_sz;
desc->rqcfg.brst_len = 1;
desc->rqtype = direction;
+ desc->bytes_requested = sg_dma_len(sg);
+ pch->transfered = 0;
}
/* Return the last desc in the chain */
--
1.9.1
DMA_PAUSE command is used for halting DMA transfer on chosen channel.
It can be useful when we want to safely read residue before terminating
all requests on channel. Otherwise there can be situation when some data
is transferred before channel termination but after reading residue,
which obviously results with data loss. To avoid this situation we can
pause channel, read residue and then terminate all requests.
This scenario is common, for example, in serial port drivers.
Signed-off-by: Robert Baldyga <[email protected]>
---
drivers/dma/pl330.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index c32806d..9c64421 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2115,6 +2115,26 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned
list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);
spin_unlock_irqrestore(&pch->lock, flags);
break;
+ case DMA_PAUSE:
+ /*
+ * We don't support DMA_RESUME command because of hardware
+ * limitations, so after pausing the channel we cannot restore
+ * it to active state. We have to terminate channel and setup
+ * DMA transfer again. This pause feature was implemented to
+ * allow safely read residue before channel termination.
+ */
+ spin_lock_irqsave(&pch->lock, flags);
+
+ spin_lock(&pl330->lock);
+ _stop(pch->thread);
+ spin_unlock(&pl330->lock);
+
+ pch->thread->req[0].desc = NULL;
+ pch->thread->req[1].desc = NULL;
+ pch->thread->req_running = -1;
+
+ spin_unlock_irqrestore(&pch->lock, flags);
+ break;
case DMA_SLAVE_CONFIG:
slave_config = (struct dma_slave_config *)arg;
--
1.9.1
On 11/25/2014 01:25 PM, Robert Baldyga wrote:
[...]
> static enum dma_status
> pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> struct dma_tx_state *txstate)
> {
> - return dma_cookie_status(chan, cookie, txstate);
> + enum dma_status ret;
> + unsigned long flags;
> + struct dma_pl330_desc *desc;
> + struct dma_pl330_chan *pch = to_pchan(chan);
> + unsigned int bytes_transferred;
> + unsigned int residual;
> +
> + /* Check in pending list */
> + spin_lock_irqsave(&pch->lock, flags);
> + list_for_each_entry(desc, &pch->work_list, node) {
> + if (desc->txd.cookie == cookie) {
> + bytes_transferred =
> + pl330_get_current_xferred_count(pch, desc);
> + residual = desc->bytes_requested -
> + bytes_transferred % desc->bytes_requested;
> + dma_set_residue(txstate, residual);
> + ret = desc->status;
> + spin_unlock_irqrestore(&pch->lock, flags);
> + return ret;
I don't think this has the correct semantics. The expected behavior is that
you pass a cookie of a descriptor and tx_status tells you how many bytes for
the whole descriptor are still left to be transferred. What you implemented
tells you how many bytes are still left for the current segment of the
active descriptor. This will only work fine for descriptors that only have
one segment. But this will definitely break audio playback using the PL330
where you have more than one segment per descriptor.
> + }
> + }
> + spin_unlock_irqrestore(&pch->lock, flags);
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + dma_set_residue(txstate, pch->transfered);
> +
pch->transfered is always 0?
On 25.11.2014 13:25, Robert Baldyga wrote:
> DMA_PAUSE command is used for halting DMA transfer on chosen channel.
> It can be useful when we want to safely read residue before terminating
> all requests on channel. Otherwise there can be situation when some data
> is transferred before channel termination but after reading residue,
> which obviously results with data loss. To avoid this situation we can
> pause channel, read residue and then terminate all requests.
> This scenario is common, for example, in serial port drivers.
>
> Signed-off-by: Robert Baldyga <[email protected]>
> ---
> drivers/dma/pl330.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index c32806d..9c64421 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2115,6 +2115,26 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned
> list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);
> spin_unlock_irqrestore(&pch->lock, flags);
> break;
> + case DMA_PAUSE:
pm_runtime_get_sync() here? If transfer is ongoing this shouldn't be
needed but if this is called after all transfers complete then device
will be suspended.
> + /*
> + * We don't support DMA_RESUME command because of hardware
> + * limitations, so after pausing the channel we cannot restore
> + * it to active state. We have to terminate channel and setup
> + * DMA transfer again. This pause feature was implemented to
> + * allow safely read residue before channel termination.
> + */
> + spin_lock_irqsave(&pch->lock, flags);
> +
> + spin_lock(&pl330->lock);
> + _stop(pch->thread);
> + spin_unlock(&pl330->lock);
> +
> + pch->thread->req[0].desc = NULL;
> + pch->thread->req[1].desc = NULL;
> + pch->thread->req_running = -1;
> +
> + spin_unlock_irqrestore(&pch->lock, flags);
symmetric pm_runtime_put + autosuspend.
Best regards,
Krzysztof
On 25.11.2014 13:25, Robert Baldyga wrote:
> This patch adds possibility to read residue of DMA transfer. It's useful
> when we want to know how many bytes have been transfered before we
> terminate channel. It can take place, for example, on timeout interrupt.
>
> Signed-off-by: Lukasz Czerwinski <[email protected]>
> Signed-off-by: Robert Baldyga <[email protected]>
> ---
> drivers/dma/pl330.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index d5149aa..c32806d 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -437,6 +437,7 @@ struct dma_pl330_chan {
> /* For D-to-M and M-to-D channels */
> int burst_sz; /* the peripheral fifo width */
> int burst_len; /* the number of burst */
> + int transfered;
> dma_addr_t fifo_addr;
>
> /* for cyclic capability */
> @@ -500,6 +501,9 @@ struct dma_pl330_desc {
>
> enum desc_status status;
>
> + int bytes_requested;
> + int direction;
> +
> /* The channel which currently holds this desc */
> struct dma_pl330_chan *pchan;
>
> @@ -2156,11 +2160,60 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
> spin_unlock_irqrestore(&pch->lock, flags);
> }
>
> +int pl330_get_current_xferred_count(struct dma_pl330_chan *pch,
> + struct dma_pl330_desc *desc)
> +{
> + u32 val, addr;
> + struct pl330_thread *thrd = pch->thread;
> + void __iomem *regs = thrd->dmac->base;
> +
> + val = addr = 0;
> + switch (desc->direction) {
> + case DMA_MEM_TO_DEV:
> + val = readl(regs + SA(thrd->id));
> + addr = desc->px.src_addr;
> + break;
> + case DMA_DEV_TO_MEM:
> + val = readl(regs + DA(thrd->id));
> + addr = desc->px.dst_addr;
> + break;
> + default:
> + break;
> + }
> + return val - addr;
> +}
> +
> static enum dma_status
> pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> struct dma_tx_state *txstate)
> {
> - return dma_cookie_status(chan, cookie, txstate);
> + enum dma_status ret;
> + unsigned long flags;
> + struct dma_pl330_desc *desc;
> + struct dma_pl330_chan *pch = to_pchan(chan);
> + unsigned int bytes_transferred;
> + unsigned int residual;
> +
> + /* Check in pending list */
> + spin_lock_irqsave(&pch->lock, flags);
> + list_for_each_entry(desc, &pch->work_list, node) {
> + if (desc->txd.cookie == cookie) {
> + bytes_transferred =
> + pl330_get_current_xferred_count(pch, desc);
As in 2nd patch - what if this get called after all transfers complete?
You're touching the device here so pm_runtime_get_sync()/put/autosuspend
may be needed.
Best regards,
Krzysztof
> + residual = desc->bytes_requested -
> + bytes_transferred % desc->bytes_requested;
> + dma_set_residue(txstate, residual);
> + ret = desc->status;
> + spin_unlock_irqrestore(&pch->lock, flags);
> + return ret;
> + }
> + }
> + spin_unlock_irqrestore(&pch->lock, flags);
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + dma_set_residue(txstate, pch->transfered);
> +
> + return ret;
> }
>
> static void pl330_issue_pending(struct dma_chan *chan)
> @@ -2421,10 +2474,13 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
> break;
> }
>
> + desc->direction = direction;
> desc->rqtype = direction;
> desc->rqcfg.brst_size = pch->burst_sz;
> desc->rqcfg.brst_len = 1;
> + desc->bytes_requested = period_len;
> fill_px(&desc->px, dst, src, period_len);
> + pch->transfered = 0;
>
> if (!first)
> first = desc;
> @@ -2554,9 +2610,12 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> sg_dma_address(sg), addr, sg_dma_len(sg));
> }
>
> + desc->direction = direction;
> desc->rqcfg.brst_size = pch->burst_sz;
> desc->rqcfg.brst_len = 1;
> desc->rqtype = direction;
> + desc->bytes_requested = sg_dma_len(sg);
> + pch->transfered = 0;
> }
>
> /* Return the last desc in the chain */
>
Hi Robert,
I tested your patches on exynos5420 peach-pit with audio playback and
got some underrun error.
I posted a similar patch in the below link.
https://patchwork.kernel.org/patch/5384551/
Please do review the same.
Thanks
Padma
> On 25.11.2014 13:25, Robert Baldyga wrote:
>> This patch adds possibility to read residue of DMA transfer. It's useful
>> when we want to know how many bytes have been transfered before we
>> terminate channel. It can take place, for example, on timeout interrupt.
>>
>> Signed-off-by: Lukasz Czerwinski <[email protected]>
>> Signed-off-by: Robert Baldyga <[email protected]>
>> ---
>> drivers/dma/pl330.c | 61
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 60 insertions(+), 1 deletion(-)
>>