* [4/6] dma: tegra: add accurate reporting of dma state
From: Ben Dooks @ 2018-10-31 16:03 UTC (permalink / raw)
To: dan.j.williams, vkoul; +Cc: ldewangan, dmaengine, linux-tegra, Ben Dooks
The tx_status callback does not report the state of the transfer
beyond complete segments. This causes problems with users such as
ALSA when applications want to know accurately how much data has
been moved.
This patch addes a function tegra_dma_update_residual() to query
the hardware and modify the residual information accordinly. It
takes into account any hardware issues when trying to read the
state, such as delays between finishing a buffer and signalling
the interrupt.
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
drivers/dma/tegra20-apb-dma.c | 94 ++++++++++++++++++++++++++++++++---
1 file changed, 87 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 4f7d1e576d03..3fa3a1ac4f57 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -802,12 +802,96 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
return 0;
}
+static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *sg_req,
+ struct tegra_dma_desc *dma_desc,
+ unsigned int residual)
+{
+ unsigned long status = 0x0;
+ unsigned long wcount;
+ unsigned long ahbptr;
+ unsigned long tmp = 0x0;
+ unsigned int result;
+ int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
+ int done;
+
+ /* if we're not the current request, then don't alter the residual */
+ if (sg_req != list_first_entry(&tdc->pending_sg_req,
+ struct tegra_dma_sg_req, node)) {
+ result = residual;
+ ahbptr = 0xffffffff;
+ goto done;
+ }
+
+ /* loop until we have a reliable result for residual */
+ do {
+ ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
+ status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+ tmp = tdc_read(tdc, 0x08); /* total count for debug */
+
+ /* check status, if channel isn't busy then skip */
+ if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
+ result = residual;
+ break;
+ }
+
+ /* if we've got an interrupt pending on the channel, don't
+ * try and deal with the residue as the hardware has likely
+ * moved on to the next buffer. return all data moved.
+ */
+ if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
+ result = residual - sg_req->req_len;
+ break;
+ }
+
+ if (tdc->tdma->chip_data->support_separate_wcount_reg)
+ wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
+ else
+ wcount = status;
+
+ /* If the request is at the full point, then there is a
+ * chance that we have read the status register in the
+ * middle of the hardware reloading the next buffer.
+ *
+ * The sequence seems to be at the end of the buffer, to
+ * load the new word count before raising the EOC flag (or
+ * changing the ping-pong flag which could have also been
+ * used to determine a new buffer). This means there is a
+ * small window where we cannot determine zero-done for the
+ * current buffer, or moved to next buffer.
+ *
+ * If done shows 0, then retry the load, as it may hit the
+ * above hardware race. We will either get a new value which
+ * is from the first buffer, or we get an EOC (new buffer)
+ * or both a new value and an EOC...
+ */
+ done = get_current_xferred_count(tdc, sg_req, wcount);
+ if (done != 0) {
+ result = residual - done;
+ break;
+ }
+
+ ndelay(100);
+ } while (--retries > 0);
+
+ if (retries <= 0) {
+ dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
+ result = residual;
+ }
+
+done:
+ dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
+ sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
+
+ return result;
+}
+
static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
dma_cookie_t cookie, struct dma_tx_state *txstate)
{
struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
struct tegra_dma_desc *dma_desc;
- struct tegra_dma_sg_req *sg_req;
+ struct tegra_dma_sg_req *sg_req = NULL;
enum dma_status ret;
unsigned long flags;
unsigned int residual;
@@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
residual = dma_desc->bytes_requested -
(dma_desc->bytes_transferred %
dma_desc->bytes_requested);
+ residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);
dma_set_residue(txstate, residual);
}
@@ -1436,12 +1521,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
- /*
- * XXX The hardware appears to support
- * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
- * only used by this driver during tegra_dma_terminate_all()
- */
- tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+ tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
tdma->dma_dev.device_config = tegra_dma_slave_config;
tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
^ permalink raw reply related
* [5/6] dma: tegra: add tracepoints to driver
From: Ben Dooks @ 2018-10-31 16:03 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: ldewangan, dmaengine, linux-tegra, Ben Dooks, Ingo Molnar,
Steven Rostedt
Add some trace-points to the driver to allow for debuging via the
trace pipe.
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Fixes since v1:
- take copy of dmachan name instead of pointer to device
Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
Cc: Steven Rostedt <rostedt@goodmis.org> (maintainer:TRACING)
---
drivers/dma/tegra20-apb-dma.c | 8 ++++
include/trace/events/tegra_apb_dma.h | 61 ++++++++++++++++++++++++++++
2 files changed, 69 insertions(+)
create mode 100644 include/trace/events/tegra_apb_dma.h
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 3fa3a1ac4f57..22114c9a6e98 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -38,6 +38,9 @@
#include "dmaengine.h"
+#define CREATE_TRACE_POINTS
+#include <trace/events/tegra_apb_dma.h>
+
#define TEGRA_APBDMA_GENERAL 0x0
#define TEGRA_APBDMA_GENERAL_ENABLE BIT(31)
@@ -672,6 +675,8 @@ static void tegra_dma_tasklet(unsigned long data)
dmaengine_desc_get_callback(&dma_desc->txd, &cb);
cb_count = dma_desc->cb_count;
dma_desc->cb_count = 0;
+ trace_tegra_dma_complete_cb(&tdc->dma_chan, cb_count,
+ cb.callback);
spin_unlock_irqrestore(&tdc->lock, flags);
while (cb_count--)
dmaengine_desc_callback_invoke(&cb, NULL);
@@ -688,6 +693,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
spin_lock_irqsave(&tdc->lock, flags);
+ trace_tegra_dma_isr(&tdc->dma_chan, irq);
status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
@@ -931,6 +937,8 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
dma_set_residue(txstate, residual);
}
+ trace_tegra_dma_tx_status(&tdc->dma_chan, cookie,
+ txstate ? txstate->residue : -1);
spin_unlock_irqrestore(&tdc->lock, flags);
return ret;
}
diff --git a/include/trace/events/tegra_apb_dma.h b/include/trace/events/tegra_apb_dma.h
new file mode 100644
index 000000000000..1f55c2c6049d
--- /dev/null
+++ b/include/trace/events/tegra_apb_dma.h
@@ -0,0 +1,61 @@
+#if !defined(_TRACE_TEGRA_APB_DMA_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_TEGRA_APM_DMA_H
+
+#include <linux/tracepoint.h>
+#include <linux/dmaengine.h>
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM tegra_apb_dma
+
+TRACE_EVENT(tegra_dma_tx_status,
+ TP_PROTO(struct dma_chan *dc, s32 cookie, u32 residue),
+ TP_ARGS(dc, cookie, residue),
+ TP_STRUCT__entry(
+ __string(chan, 16)
+ __field(__s32, cookie)
+ __field(__u32, residue)
+ ),
+ TP_fast_assign(
+ __assign_str(chan, dev_name(&dc->dev->device));
+ __entry->cookie = cookie;
+ __entry->residue = residue;
+ ),
+ TP_printk("channel %s: dma cookie %d, residue %u",
+ __get_str(chan), __entry->cookie, __entry->residue)
+);
+
+TRACE_EVENT(tegra_dma_complete_cb,
+ TP_PROTO(struct dma_chan *dc, int count, void *ptr),
+ TP_ARGS(dc, count, ptr),
+ TP_STRUCT__entry(
+ __string(chan, 16)
+ __field(int, count)
+ __field(void *, ptr)
+ ),
+ TP_fast_assign(
+ __assign_str(chan, dev_name(&dc->dev->device));
+ __entry->count = count;
+ __entry->ptr = ptr;
+ ),
+ TP_printk("channel %s: done %d, ptr %p",
+ __get_str(chan), __entry->count, __entry->ptr)
+);
+
+TRACE_EVENT(tegra_dma_isr,
+ TP_PROTO(struct dma_chan *dc, int irq),
+ TP_ARGS(dc, irq),
+ TP_STRUCT__entry(
+ __string(chan, 16)
+ __field(int, irq)
+ ),
+ TP_fast_assign(
+ __assign_str(chan, dev_name(&dc->dev->device));
+ __entry->irq = irq;
+ ),
+ TP_printk("%s: irq %d\n", __get_str(chan), __entry->irq)
+);
+
+#endif /* _TRACE_TEGRADMA_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
^ permalink raw reply related
* [6/6] dma: tegra: add tracepoint for residual update
From: Ben Dooks @ 2018-10-31 16:03 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: ldewangan, dmaengine, linux-tegra, Ben Dooks, Ingo Molnar,
Steven Rostedt
Add a tracepoint in the residual update instead of using dev_dbg()
to allow debugging via the trace pipe.
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Fixes since v1:
- copy the devname instead of referencing the dmachan device
Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
Cc: Steven Rostedt <rostedt@goodmis.org> (maintainer:TRACING)
---
drivers/dma/tegra20-apb-dma.c | 5 ++---
include/trace/events/tegra_apb_dma.h | 27 +++++++++++++++++++++++++++
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 22114c9a6e98..211efea67db6 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -886,9 +886,8 @@ static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
}
done:
- dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
- sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
-
+ trace_tegra_dma_tx_state(&tdc->dma_chan, ahbptr, status, result,
+ tmp, residual);
return result;
}
diff --git a/include/trace/events/tegra_apb_dma.h b/include/trace/events/tegra_apb_dma.h
index 1f55c2c6049d..8fa0f0974828 100644
--- a/include/trace/events/tegra_apb_dma.h
+++ b/include/trace/events/tegra_apb_dma.h
@@ -41,6 +41,33 @@ TRACE_EVENT(tegra_dma_complete_cb,
__get_str(chan), __entry->count, __entry->ptr)
);
+TRACE_EVENT(tegra_dma_tx_state,
+ TP_PROTO(struct dma_chan *dc, unsigned long ahb,
+ unsigned long wc, unsigned int done,
+ unsigned long byte_count, unsigned int residual),
+ TP_ARGS(dc, ahb, wc, done, byte_count, residual),
+ TP_STRUCT__entry(
+ __string(chan, 16)
+ __field(unsigned long, ahb)
+ __field(unsigned long, wc)
+ __field(unsigned long, done)
+ __field(unsigned int, residual)
+ __field(unsigned long, byte_count)
+ ),
+ TP_fast_assign(
+ __assign_str(chan, dev_name(&dc->dev->device));
+ __entry->ahb = ahb;
+ __entry->wc = wc;
+ __entry->done = done;
+ __entry->residual = residual;
+ __entry->byte_count = byte_count;
+ ),
+ TP_printk("%s: txresidual: ahb %08lx wc %08lx => done %lu bc %lu residual %u",
+ __get_str(chan),
+ __entry->ahb, __entry->wc, __entry->done,
+ __entry->byte_count, __entry->residual)
+);
+
TRACE_EVENT(tegra_dma_isr,
TP_PROTO(struct dma_chan *dc, int irq),
TP_ARGS(dc, irq),
^ permalink raw reply related
* [5/6] dma: tegra: add tracepoints to driver
From: Steven Rostedt @ 2018-11-01 13:54 UTC (permalink / raw)
To: Ben Dooks
Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra,
Ingo Molnar
On Wed, 31 Oct 2018 16:03:08 +0000
Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> diff --git a/include/trace/events/tegra_apb_dma.h b/include/trace/events/tegra_apb_dma.h
> new file mode 100644
> index 000000000000..1f55c2c6049d
> --- /dev/null
> +++ b/include/trace/events/tegra_apb_dma.h
> @@ -0,0 +1,61 @@
> +#if !defined(_TRACE_TEGRA_APB_DMA_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_TEGRA_APM_DMA_H
> +
> +#include <linux/tracepoint.h>
> +#include <linux/dmaengine.h>
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM tegra_apb_dma
> +
> +TRACE_EVENT(tegra_dma_tx_status,
> + TP_PROTO(struct dma_chan *dc, s32 cookie, u32 residue),
> + TP_ARGS(dc, cookie, residue),
> + TP_STRUCT__entry(
> + __string(chan, 16)
Hi Ben,
Was this tested? Because I think __string(chan, 16) would fault, as the
second parameter is meant to be a pointer to the source parameter.
Otherwise, you need to add dev_name(&dc->dev->device) as the second
parameter.
If you are just using fixed string lengths then it's best not to use the
__string() type, as that's for dynamic strings (strings changing in
size for each event) and uses 8 more bytes to store the offset and
length of the string in the event.
If you take a look at the sched_waking or sched_switch trace events in
include/trace/events/sched.h you'll see that it saves the comm string
directly as a character array:
__array( char, comm, TASK_COMM_LEN)
For you, you can use:
__array(char, chan, 16)
> + __field(__s32, cookie)
> + __field(__u32, residue)
> + ),
> + TP_fast_assign(
> + __assign_str(chan, dev_name(&dc->dev->device));
Then to store the array:
memcpy(__entry->chan, dev_name(&dc->dev->device), 16);
> + __entry->cookie = cookie;
> + __entry->residue = residue;
> + ),
> + TP_printk("channel %s: dma cookie %d, residue %u",
__entry->chan,
is all that's needed.
Same for the other references to chan.
Unless you meant to store each with a different size, then you need to
replace that "16" with the dev_name(&dc->dev->device).
-- Steve
> + __get_str(chan), __entry->cookie, __entry->residue)
> +);
> +
> +TRACE_EVENT(tegra_dma_complete_cb,
> + TP_PROTO(struct dma_chan *dc, int count, void *ptr),
> + TP_ARGS(dc, count, ptr),
> + TP_STRUCT__entry(
> + __string(chan, 16)
> + __field(int, count)
> + __field(void *, ptr)
> + ),
> + TP_fast_assign(
> + __assign_str(chan, dev_name(&dc->dev->device));
> + __entry->count = count;
> + __entry->ptr = ptr;
> + ),
> + TP_printk("channel %s: done %d, ptr %p",
> + __get_str(chan), __entry->count, __entry->ptr)
> +);
> +
> +TRACE_EVENT(tegra_dma_isr,
> + TP_PROTO(struct dma_chan *dc, int irq),
> + TP_ARGS(dc, irq),
> + TP_STRUCT__entry(
> + __string(chan, 16)
> + __field(int, irq)
> + ),
> + TP_fast_assign(
> + __assign_str(chan, dev_name(&dc->dev->device));
> + __entry->irq = irq;
> + ),
> + TP_printk("%s: irq %d\n", __get_str(chan), __entry->irq)
> +);
> +
> +#endif /* _TRACE_TEGRADMA_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
^ permalink raw reply
* dmaengine: fix some typo
From: Yangtao Li @ 2018-11-01 15:35 UTC (permalink / raw)
To: dan.j.williams, vkoul; +Cc: dmaengine, linux-kernel, Yangtao Li
Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
---
drivers/dma/ep93xx_dma.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index a15592383d4e..7997e9bb7e10 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -993,7 +993,7 @@ ep93xx_dma_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest,
for (offset = 0; offset < len; offset += bytes) {
desc = ep93xx_dma_desc_get(edmac);
if (!desc) {
- dev_warn(chan2dev(edmac), "couln't get descriptor\n");
+ dev_warn(chan2dev(edmac), "couldn't get descriptor\n");
goto fail;
}
@@ -1063,7 +1063,7 @@ ep93xx_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
desc = ep93xx_dma_desc_get(edmac);
if (!desc) {
- dev_warn(chan2dev(edmac), "couln't get descriptor\n");
+ dev_warn(chan2dev(edmac), "couldn't get descriptor\n");
goto fail;
}
@@ -1141,7 +1141,7 @@ ep93xx_dma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t dma_addr,
for (offset = 0; offset < buf_len; offset += period_len) {
desc = ep93xx_dma_desc_get(edmac);
if (!desc) {
- dev_warn(chan2dev(edmac), "couln't get descriptor\n");
+ dev_warn(chan2dev(edmac), "couldn't get descriptor\n");
goto fail;
}
^ permalink raw reply related
* dmaengine: dmatest: move size checks earlier in function
From: Alexandru Ardelean @ 2018-11-01 16:07 UTC (permalink / raw)
To: dmaengine, vkoul; +Cc: Alexandru Ardelean
There's no need to allocate all that memory if these sizes are invalid
anyway.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/dma/dmatest.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index aa1712beb0cc..fa991e095ef2 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -507,6 +507,19 @@ static int dmatest_func(void *data)
} else
goto err_thread_type;
+ /* Check if buffer count fits into map count variable (u8) */
+ if ((src_cnt + dst_cnt) >= 255) {
+ pr_err("too many buffers (%d of 255 supported)\n",
+ src_cnt + dst_cnt);
+ goto err_thread_type;
+ }
+
+ if (1 << align > params->buf_size) {
+ pr_err("%u-byte buffer too small for %d-byte alignment\n",
+ params->buf_size, 1 << align);
+ goto err_thread_type;
+ }
+
thread->srcs = kcalloc(src_cnt + 1, sizeof(u8 *), GFP_KERNEL);
if (!thread->srcs)
goto err_srcs;
@@ -576,19 +589,6 @@ static int dmatest_func(void *data)
total_tests++;
- /* Check if buffer count fits into map count variable (u8) */
- if ((src_cnt + dst_cnt) >= 255) {
- pr_err("too many buffers (%d of 255 supported)\n",
- src_cnt + dst_cnt);
- break;
- }
-
- if (1 << align > params->buf_size) {
- pr_err("%u-byte buffer too small for %d-byte alignment\n",
- params->buf_size, 1 << align);
- break;
- }
-
if (params->norandom)
len = params->buf_size;
else
^ permalink raw reply related
* [4/6] dma: tegra: add accurate reporting of dma state
From: Dmitry Osipenko @ 2018-11-03 12:24 UTC (permalink / raw)
To: Ben Dooks, dan.j.williams, vkoul; +Cc: ldewangan, dmaengine, linux-tegra
On 31.10.2018 19:03, Ben Dooks wrote:
> The tx_status callback does not report the state of the transfer
> beyond complete segments. This causes problems with users such as
> ALSA when applications want to know accurately how much data has
> been moved.
>
> This patch addes a function tegra_dma_update_residual() to query
> the hardware and modify the residual information accordinly. It
> takes into account any hardware issues when trying to read the
> state, such as delays between finishing a buffer and signalling
> the interrupt.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Hello Ben,
Thank you very much for the patch! It fixes "farting sound" for chromium-browser and applications that use chromium-engine (tested on Tegra20) because apparently it tries to use low latency for everything and audio buffer is constantly underflowing without more detailed DMA-progress reporting. See couple more comments below.
> ---
> drivers/dma/tegra20-apb-dma.c | 94 ++++++++++++++++++++++++++++++++---
> 1 file changed, 87 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 4f7d1e576d03..3fa3a1ac4f57 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -802,12 +802,96 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
> return 0;
> }
>
> +static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
> + struct tegra_dma_sg_req *sg_req,
> + struct tegra_dma_desc *dma_desc,
> + unsigned int residual)
> +{
> + unsigned long status = 0x0;
There is no need to initialize "status" variable.
> + unsigned long wcount;
> + unsigned long ahbptr;
> + unsigned long tmp = 0x0;
> + unsigned int result;
> + int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
> + int done;
> +
> + /* if we're not the current request, then don't alter the residual */
> + if (sg_req != list_first_entry(&tdc->pending_sg_req,
> + struct tegra_dma_sg_req, node)) {
> + result = residual;
> + ahbptr = 0xffffffff;
> + goto done;
> + }
> +
> + /* loop until we have a reliable result for residual */
> + do {
> + ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
> + status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
> + tmp = tdc_read(tdc, 0x08); /* total count for debug */
Register 0x08 (DMA_BYTE_STA) doesn't present on Tegra20 and "tmp" isn't used anywhere in the code. Please remove it entirely.
> +
> + /* check status, if channel isn't busy then skip */
> + if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
> + result = residual;
> + break;
> + }
If "BUSY" is unset, doesn't this mean that transaction could be completed already? I don't quite understand why you want to skip here.
> +
> + /* if we've got an interrupt pending on the channel, don't
> + * try and deal with the residue as the hardware has likely
> + * moved on to the next buffer. return all data moved.
> + */
> + if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
> + result = residual - sg_req->req_len;
> + break;
> + }
> +
> + if (tdc->tdma->chip_data->support_separate_wcount_reg)
> + wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
> + else
> + wcount = status;
> +
> + /* If the request is at the full point, then there is a
> + * chance that we have read the status register in the
> + * middle of the hardware reloading the next buffer.
> + *
> + * The sequence seems to be at the end of the buffer, to
> + * load the new word count before raising the EOC flag (or
> + * changing the ping-pong flag which could have also been
> + * used to determine a new buffer). This means there is a
two whitespaces here-----|
> + * small window where we cannot determine zero-done for the
> + * current buffer, or moved to next buffer.
> + *
> + * If done shows 0, then retry the load, as it may hit the
> + * above hardware race. We will either get a new value which
> + * is from the first buffer, or we get an EOC (new buffer)
> + * or both a new value and an EOC...
I think we just need to wait 20usec after reading out "words count" and then re-check interrupt status, so transfer is done if interrupt is set and otherwise "words count" value is actual and reliable.
> + */
> + done = get_current_xferred_count(tdc, sg_req, wcount);
> + if (done != 0) {
> + result = residual - done;
> + break;
> + }
> +
> + ndelay(100);
There is no ndelay() on ARM, hence your 20usec timeout is 200usec. Please use udelay().
> + } while (--retries > 0);
> +
> + if (retries <= 0) {
> + dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
> + result = residual;
> + }
> +
> +done:
Please rename goto label as it duplicates local variable name.
> + dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
> + sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
Whitespace just after tab not needed.
> +
> + return result;
> +}
> +
> static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
> dma_cookie_t cookie, struct dma_tx_state *txstate)
> {
> struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> struct tegra_dma_desc *dma_desc;
> - struct tegra_dma_sg_req *sg_req;
> + struct tegra_dma_sg_req *sg_req = NULL;
> enum dma_status ret;
> unsigned long flags;
> unsigned int residual;
> @@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
> residual = dma_desc->bytes_requested -
> (dma_desc->bytes_transferred %
> dma_desc->bytes_requested);
> + residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);
> dma_set_residue(txstate, residual);
> }
>
> @@ -1436,12 +1521,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
> BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> - /*
> - * XXX The hardware appears to support
> - * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
> - * only used by this driver during tegra_dma_terminate_all()
> - */
> - tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> + tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
> tdma->dma_dev.device_config = tegra_dma_slave_config;
> tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
> tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
>
Summarizing all of the comments above, patch may look like this:
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 9a558e30c461..956e8130c059 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -799,12 +799,73 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
return 0;
}
+static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *sg_req,
+ struct tegra_dma_desc *dma_desc,
+ unsigned int residual)
+{
+ unsigned long status;
+ unsigned int result;
+ int done;
+
+ /* if we're not the current request, then don't alter the residual */
+ if (sg_req != list_first_entry(&tdc->pending_sg_req,
+ struct tegra_dma_sg_req, node))
+ return residual;
+
+ status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+
+ /* if we've got an interrupt pending on the channel, don't
+ * try and deal with the residue as the hardware has likely
+ * moved on to the next buffer. return all data moved.
+ */
+ if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
+ result = residual - sg_req->req_len;
+ goto out;
+ }
+
+ if (tdc->tdma->chip_data->support_separate_wcount_reg)
+ status = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
+
+ /*
+ * If the request is at the full point, then there is a
+ * chance that we have read the status register in the
+ * middle of the hardware reloading the next buffer.
+ *
+ * The sequence seems to be at the end of the buffer, to
+ * load the new word count before raising the EOC flag (or
+ * changing the ping-pong flag which could have also been
+ * used to determine a new buffer). This means there is a
+ * small window where we cannot determine zero-done for the
+ * current buffer, or moved to next buffer.
+ */
+ done = get_current_xferred_count(tdc, sg_req, status);
+
+ udelay(TEGRA_APBDMA_BURST_COMPLETE_TIME);
+
+ status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+
+ if (status & TEGRA_APBDMA_STATUS_ISE_EOC)
+ result = residual - sg_req->req_len;
+ else
+ result = residual - done;
+out:
+#ifdef DEBUG
+ ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
+
+ dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
+ sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
+#endif
+
+ return result;
+}
+
static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
dma_cookie_t cookie, struct dma_tx_state *txstate)
{
struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
struct tegra_dma_desc *dma_desc;
- struct tegra_dma_sg_req *sg_req;
+ struct tegra_dma_sg_req *sg_req = NULL;
enum dma_status ret;
unsigned long flags;
unsigned int residual;
@@ -840,6 +901,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
residual = dma_desc->bytes_requested -
(dma_desc->bytes_transferred %
dma_desc->bytes_requested);
+ residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);
dma_set_residue(txstate, residual);
}
@@ -1433,12 +1495,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
- /*
- * XXX The hardware appears to support
- * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
- * only used by this driver during tegra_dma_terminate_all()
- */
- tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+ tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
tdma->dma_dev.device_config = tegra_dma_slave_config;
tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
^ permalink raw reply related
* [v1,1/2] dt-bindings: dmaengine: dw-dmac: add protection control property
From: Christian Lamparter @ 2018-11-04 17:01 UTC (permalink / raw)
To: dmaengine, devicetree
Cc: Dan Williams, Vinod Koul, Andy Shevchenko, Viresh Kumar,
Rob Herring, Mark Rutland
This patch adds the per-channel dma protection control
prop-encoded-array and dt-binding definitions (including
definitions for the existing channel allocation and priority
order values based on include/linux/platform_data/dma-dw.h)
for the DesignWare AHB Central Direct Memory Access Controller.
Note: The protection control signals are one-to-one mapped to
the AHB HPROT[1:3] signals. The HPROT0 (Data Access) is
always hardwired to 1 for this controller.
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
.../devicetree/bindings/dma/snps-dma.txt | 4 ++++
MAINTAINERS | 4 +++-
include/dt-bindings/dma/dw-dmac.h | 20 +++++++++++++++++++
3 files changed, 27 insertions(+), 1 deletion(-)
create mode 100644 include/dt-bindings/dma/dw-dmac.h
diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
index 39e2b26be344..72b4984a4c18 100644
--- a/Documentation/devicetree/bindings/dma/snps-dma.txt
+++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
@@ -27,6 +27,10 @@ Optional properties:
general purpose DMA channel allocator. False if not passed.
- multi-block: Multi block transfers supported by hardware. Array property with
one cell per channel. 0: not supported, 1 (default): supported.
+- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
+ Array property with one cell per channel. The default value for a channel
+ is 0 (for non-cacheable, strongly ordered, unprivileged data access).
+ Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
Example:
diff --git a/MAINTAINERS b/MAINTAINERS
index dacba23b80b4..c35998e20e9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14107,9 +14107,11 @@ SYNOPSYS DESIGNWARE DMAC DRIVER
M: Viresh Kumar <vireshk@kernel.org>
R: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
S: Maintained
+F: Documentation/devicetree/bindings/dma/snps-dma.txt
+F: drivers/dma/dw/
+F: include/dt-bindings/dma/dw-dmac.h
F: include/linux/dma/dw.h
F: include/linux/platform_data/dma-dw.h
-F: drivers/dma/dw/
SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
M: Jose Abreu <Jose.Abreu@synopsys.com>
diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
new file mode 100644
index 000000000000..9152a6e2406c
--- /dev/null
+++ b/include/dt-bindings/dma/dw-dmac.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+
+#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
+#define __DT_BINDINGS_DMA_DW_DMAC_H__
+
+#define DW_DMAC_CHAN_ALLOCATION_ASCENDING 0 /* zero to seven */
+#define DW_DMAC_CHAN_ALLOCATION_DESCENDING 1 /* seven to zero */
+#define DW_DMAC_CHAN_PRIORITY_ASCENDING 0 /* chan0 highest */
+#define DW_DMAC_CHAN_PRIORITY_DESCENDING 1 /* chan7 highest */
+
+/*
+ * Protection Control bits provide protection against illegal transactions.
+ * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
+ * The AHB HPROT[0] bit is hardwired to 1: Data Access.
+ */
+#define DW_DMAC_HPROT1_PRIVILEGED_MODE (1 << 0) /* Privileged Mode */
+#define DW_DMAC_HPROT2_BUFFERABLE (1 << 1) /* DMA is bufferable */
+#define DW_DMAC_HPROT3_CACHEABLE (1 << 2) /* DMA is cacheable */
+
+#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
^ permalink raw reply related
* [v1,2/2] dmaengine: dw: implement per-channel protection control setting
From: Christian Lamparter @ 2018-11-04 17:01 UTC (permalink / raw)
To: dmaengine, devicetree
Cc: Dan Williams, Vinod Koul, Andy Shevchenko, Viresh Kumar,
Rob Herring, Mark Rutland
This patch adds a new device-tree property that allows to
specify the protection control bits for each DMA channel
individually.
Setting the "correct" bits can have a huge impact on the
PPC460EX and APM82181 that use this DMA engine in combination
with a DesignWare' SATA-II core (sata_dwc_460ex driver).
In the OpenWrt Forum, the user takimata reported that:
|<https://forum.lede-project.org/t/wd-mybook-live-duo-two-disks/16195/55>
|It seems your patch unleashed the full power of the SATA port.
|Where I was previously hitting a really hard limit at around
|82 MB/s for reading and 27 MB/s for writing, I am now getting this:
|
|root@OpenWrt:/mnt# time dd if=/dev/zero of=tempfile bs=1M count=1024
|1024+0 records in
|1024+0 records out
|real 0m 13.65s
|user 0m 0.01s
|sys 0m 11.89s
|
|root@OpenWrt:/mnt# time dd if=tempfile of=/dev/null bs=1M count=1024
|1024+0 records in
|1024+0 records out
|real 0m 8.41s
|user 0m 0.01s
|sys 0m 4.70s
|
|This means: 121 MB/s reading and 75 MB/s writing!
|
|The drive is a WD Green WD10EARX taken from an older MBL Single.
|I repeated the test a few times with even larger files to rule out
|any caching, I'm still seeing the same great performance. OpenWrt is
|now completely on par with the original MBL firmware's performance.
Another user And.short reported in the same thread:
|<https://forum.openwrt.org/t/solved-wd-mybook-live-duo-two-disks/16195/50>
|I can report that your fix worked! Boots up fine with two
|drives even with more partitions, and no more reboot on
|concurrent disk access!
A closer look into the sata_dwc_460ex code revealed that
the driver did initally set the correct protection control
bits. However, this feature was lost when the sata_dwc_460ex
driver was converted to the generic DMA driver framework with:
8b3444852a2 ("sata_dwc_460ex: move to generic DMA driver").
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
drivers/dma/dw/core.c | 3 +++
drivers/dma/dw/platform.c | 12 +++++++++---
drivers/dma/dw/regs.h | 4 ++++
include/linux/platform_data/dma-dw.h | 6 ++++++
4 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index f43e6dafe446..2db15e9b33a8 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -160,12 +160,15 @@ static void dwc_initialize_chan_idma32(struct dw_dma_chan *dwc)
static void dwc_initialize_chan_dw(struct dw_dma_chan *dwc)
{
+ struct dw_dma *dw = to_dw_dma(dwc->chan.device);
+ size_t chanidx = (size_t)(dwc - dw->chan);
u32 cfghi = DWC_CFGH_FIFO_MODE;
u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
bool hs_polarity = dwc->dws.hs_polarity;
cfghi |= DWC_CFGH_DST_PER(dwc->dws.dst_id);
cfghi |= DWC_CFGH_SRC_PER(dwc->dws.src_id);
+ cfghi |= DWC_CFGH_PROTCTL(dw->pdata->protctl[chanidx]);
/* Set polarity of handshake interface */
cfglo |= hs_polarity ? DWC_CFGL_HS_DST_POL | DWC_CFGL_HS_SRC_POL : 0;
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index f62dd0944908..078cca6576c3 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -102,7 +102,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct dw_dma_platform_data *pdata;
- u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], mb[DW_DMA_MAX_NR_CHANNELS];
+ u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], val[DW_DMA_MAX_NR_CHANNELS];
u32 nr_masters;
u32 nr_channels;
@@ -154,14 +154,20 @@ dw_dma_parse_dt(struct platform_device *pdev)
pdata->data_width[tmp] = BIT(arr[tmp] & 0x07);
}
- if (!of_property_read_u32_array(np, "multi-block", mb, nr_channels)) {
+ if (!of_property_read_u32_array(np, "multi-block", val, nr_channels)) {
for (tmp = 0; tmp < nr_channels; tmp++)
- pdata->multi_block[tmp] = mb[tmp];
+ pdata->multi_block[tmp] = val[tmp];
} else {
for (tmp = 0; tmp < nr_channels; tmp++)
pdata->multi_block[tmp] = 1;
}
+ if (!of_property_read_u32_array(np, "snps,dma-protection-control",
+ val, nr_channels)) {
+ for (tmp = 0; tmp < nr_channels; tmp++)
+ pdata->protctl[tmp] = val[tmp];
+ }
+
return pdata;
}
#else
diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
index 09e7dfdbb790..646c9c960c07 100644
--- a/drivers/dma/dw/regs.h
+++ b/drivers/dma/dw/regs.h
@@ -200,6 +200,10 @@ enum dw_dma_msize {
#define DWC_CFGH_FCMODE (1 << 0)
#define DWC_CFGH_FIFO_MODE (1 << 1)
#define DWC_CFGH_PROTCTL(x) ((x) << 2)
+#define DWC_CFGH_PROTCTL_DATA (0 << 2) /* data access - always set */
+#define DWC_CFGH_PROTCTL_PRIV (1 << 2) /* privileged -> AHB HPROT[1] */
+#define DWC_CFGH_PROTCTL_BUFFER (2 << 2) /* bufferable -> AHB HPROT[2] */
+#define DWC_CFGH_PROTCTL_CACHE (4 << 2) /* cacheable -> AHB HPROT[3] */
#define DWC_CFGH_DS_UPD_EN (1 << 5)
#define DWC_CFGH_SS_UPD_EN (1 << 6)
#define DWC_CFGH_SRC_PER(x) ((x) << 7)
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 896cb71a382c..df65e3311a56 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -49,6 +49,7 @@ struct dw_dma_slave {
* @data_width: Maximum data width supported by hardware per AHB master
* (in bytes, power of 2)
* @multi_block: Multi block transfers supported by hardware per channel.
+ * @protctl: Protection control signals setting per channel.
*/
struct dw_dma_platform_data {
unsigned int nr_channels;
@@ -65,6 +66,11 @@ struct dw_dma_platform_data {
unsigned char nr_masters;
unsigned char data_width[DW_DMA_MAX_NR_MASTERS];
unsigned char multi_block[DW_DMA_MAX_NR_CHANNELS];
+#define CHAN_PROTCTL_PRIVILEGED BIT(0)
+#define CHAN_PROTCTL_BUFFERABLE BIT(1)
+#define CHAN_PROTCTL_CACHEABLE BIT(2)
+#define CHAN_PROTCTL_MASK 0x7
+ unsigned char protctl[DW_DMA_MAX_NR_CHANNELS];
};
#endif /* _PLATFORM_DATA_DMA_DW_H */
^ permalink raw reply related
* [4/6] dma: tegra: add accurate reporting of dma state
From: Ben Dooks @ 2018-11-05 9:03 UTC (permalink / raw)
To: Dmitry Osipenko; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra
On 2018-11-03 12:24, Dmitry Osipenko wrote:
> On 31.10.2018 19:03, Ben Dooks wrote:
>> The tx_status callback does not report the state of the transfer
>> beyond complete segments. This causes problems with users such as
>> ALSA when applications want to know accurately how much data has
>> been moved.
>>
>> This patch addes a function tegra_dma_update_residual() to query
>> the hardware and modify the residual information accordinly. It
>> takes into account any hardware issues when trying to read the
>> state, such as delays between finishing a buffer and signalling
>> the interrupt.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>
> Hello Ben,
>
> Thank you very much for the patch! It fixes "farting sound" for
> chromium-browser and applications that use chromium-engine (tested on
> Tegra20) because apparently it tries to use low latency for everything
> and audio buffer is constantly underflowing without more detailed
> DMA-progress reporting. See couple more comments below.
>
>> ---
>> drivers/dma/tegra20-apb-dma.c | 94
>> ++++++++++++++++++++++++++++++++---
>> 1 file changed, 87 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c
>> b/drivers/dma/tegra20-apb-dma.c
>> index 4f7d1e576d03..3fa3a1ac4f57 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -802,12 +802,96 @@ static int tegra_dma_terminate_all(struct
>> dma_chan *dc)
>> return 0;
>> }
>>
>> +static unsigned int tegra_dma_update_residual(struct
>> tegra_dma_channel *tdc,
>> + struct tegra_dma_sg_req *sg_req,
>> + struct tegra_dma_desc *dma_desc,
>> + unsigned int residual)
>> +{
>> + unsigned long status = 0x0;
>
> There is no need to initialize "status" variable.
ok, will check this.
>> + unsigned long wcount;
>> + unsigned long ahbptr;
>> + unsigned long tmp = 0x0;
>> + unsigned int result;
>> + int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
>> + int done;
>> +
>> + /* if we're not the current request, then don't alter the residual
>> */
>> + if (sg_req != list_first_entry(&tdc->pending_sg_req,
>> + struct tegra_dma_sg_req, node)) {
>> + result = residual;
>> + ahbptr = 0xffffffff;
>> + goto done;
>> + }
>> +
>> + /* loop until we have a reliable result for residual */
>> + do {
>> + ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
>> + status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
>
>> + tmp = tdc_read(tdc, 0x08); /* total count for debug */
>
> Register 0x08 (DMA_BYTE_STA) doesn't present on Tegra20 and "tmp"
> isn't used anywhere in the code. Please remove it entirely.
ok, fixed
>> +
>> + /* check status, if channel isn't busy then skip */
>> + if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
>> + result = residual;
>> + break;
>> + }
>
> If "BUSY" is unset, doesn't this mean that transaction could be
> completed already? I don't quite understand why you want to skip here.
I can't remember what the reasoning behind this was, this work was
originally done over a year ago and I am not sure if I can find any
of the notes from this.
>> +
>> + /* if we've got an interrupt pending on the channel, don't
>> + * try and deal with the residue as the hardware has likely
>> + * moved on to the next buffer. return all data moved.
>> + */
>> + if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
>> + result = residual - sg_req->req_len;
>> + break;
>> + }
>> +
>> + if (tdc->tdma->chip_data->support_separate_wcount_reg)
>> + wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
>> + else
>> + wcount = status;
>> +
>> + /* If the request is at the full point, then there is a
>> + * chance that we have read the status register in the
>> + * middle of the hardware reloading the next buffer.
>> + *
>> + * The sequence seems to be at the end of the buffer, to
>> + * load the new word count before raising the EOC flag (or
>> + * changing the ping-pong flag which could have also been
>> + * used to determine a new buffer). This means there is a
> two whitespaces here-----|
>
>> + * small window where we cannot determine zero-done for the
>> + * current buffer, or moved to next buffer.
>> + *
>
>> + * If done shows 0, then retry the load, as it may hit the
>> + * above hardware race. We will either get a new value which
>> + * is from the first buffer, or we get an EOC (new buffer)
>> + * or both a new value and an EOC...
>
> I think we just need to wait 20usec after reading out "words count"
> and then re-check interrupt status, so transfer is done if interrupt
> is set and otherwise "words count" value is actual and reliable.
At the moment I have no way of going back and re-testing this code
against the problem it originally fixed, so would rather not change
the algorithm in this.
>
>> + */
>> + done = get_current_xferred_count(tdc, sg_req, wcount);
>> + if (done != 0) {
>> + result = residual - done;
>> + break;
>> + }
>> +
>> + ndelay(100);
>
> There is no ndelay() on ARM, hence your 20usec timeout is 200usec.
> Please use udelay().
I thought there was one based on the lpj calculations, I will check
later.
>> + } while (--retries > 0);
>> +
>> + if (retries <= 0) {
>> + dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
>> + result = residual;
>> + }
>> +
>> +done:
>
> Please rename goto label as it duplicates local variable name.
ok, fixed.
>> + dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx,
>> done %d\n",
>> + sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
>
> Whitespace just after tab not needed.
>
>> +
>> + return result;
>> +}
>> +
>> static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>> dma_cookie_t cookie, struct dma_tx_state *txstate)
>> {
>> struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>> struct tegra_dma_desc *dma_desc;
>> - struct tegra_dma_sg_req *sg_req;
>> + struct tegra_dma_sg_req *sg_req = NULL;
>> enum dma_status ret;
>> unsigned long flags;
>> unsigned int residual;
>> @@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct
>> dma_chan *dc,
>> residual = dma_desc->bytes_requested -
>> (dma_desc->bytes_transferred %
>> dma_desc->bytes_requested);
>> + residual = tegra_dma_update_residual(tdc, sg_req, dma_desc,
>> residual);
>> dma_set_residue(txstate, residual);
>> }
>>
>> @@ -1436,12 +1521,7 @@ static int tegra_dma_probe(struct
>> platform_device *pdev)
>> BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>> BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
>> tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) |
>> BIT(DMA_MEM_TO_DEV);
>> - /*
>> - * XXX The hardware appears to support
>> - * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
>> - * only used by this driver during tegra_dma_terminate_all()
>> - */
>> - tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
>> + tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
>> tdma->dma_dev.device_config = tegra_dma_slave_config;
>> tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
>> tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
>>
>
> Summarizing all of the comments above, patch may look like this:
>
> diff --git a/drivers/dma/tegra20-apb-dma.c
> b/drivers/dma/tegra20-apb-dma.c
> index 9a558e30c461..956e8130c059 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -799,12 +799,73 @@ static int tegra_dma_terminate_all(struct
> dma_chan *dc)
> return 0;
> }
>
> +static unsigned int tegra_dma_update_residual(struct tegra_dma_channel
> *tdc,
> + struct tegra_dma_sg_req *sg_req,
> + struct tegra_dma_desc *dma_desc,
> + unsigned int residual)
> +{
> + unsigned long status;
> + unsigned int result;
> + int done;
> +
> + /* if we're not the current request, then don't alter the residual */
> + if (sg_req != list_first_entry(&tdc->pending_sg_req,
> + struct tegra_dma_sg_req, node))
> + return residual;
> +
> + status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
> +
> + /* if we've got an interrupt pending on the channel, don't
> + * try and deal with the residue as the hardware has likely
> + * moved on to the next buffer. return all data moved.
> + */
> + if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
> + result = residual - sg_req->req_len;
> + goto out;
> + }
> +
> + if (tdc->tdma->chip_data->support_separate_wcount_reg)
> + status = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
> +
> + /*
> + * If the request is at the full point, then there is a
> + * chance that we have read the status register in the
> + * middle of the hardware reloading the next buffer.
> + *
> + * The sequence seems to be at the end of the buffer, to
> + * load the new word count before raising the EOC flag (or
> + * changing the ping-pong flag which could have also been
> + * used to determine a new buffer). This means there is a
> + * small window where we cannot determine zero-done for the
> + * current buffer, or moved to next buffer.
> + */
> + done = get_current_xferred_count(tdc, sg_req, status);
> +
> + udelay(TEGRA_APBDMA_BURST_COMPLETE_TIME);
> +
> + status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
> +
> + if (status & TEGRA_APBDMA_STATUS_ISE_EOC)
> + result = residual - sg_req->req_len;
> + else
> + result = residual - done;
> +out:
> +#ifdef DEBUG
> + ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
> +
> + dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx,
> done %d\n",
> + sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
> +#endif
> +
> + return result;
> +}
> +
> static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
> dma_cookie_t cookie, struct dma_tx_state *txstate)
> {
> struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> struct tegra_dma_desc *dma_desc;
> - struct tegra_dma_sg_req *sg_req;
> + struct tegra_dma_sg_req *sg_req = NULL;
> enum dma_status ret;
> unsigned long flags;
> unsigned int residual;
> @@ -840,6 +901,7 @@ static enum dma_status tegra_dma_tx_status(struct
> dma_chan *dc,
> residual = dma_desc->bytes_requested -
> (dma_desc->bytes_transferred %
> dma_desc->bytes_requested);
> + residual = tegra_dma_update_residual(tdc, sg_req, dma_desc,
> residual);
> dma_set_residue(txstate, residual);
> }
>
> @@ -1433,12 +1495,7 @@ static int tegra_dma_probe(struct
> platform_device *pdev)
> BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> - /*
> - * XXX The hardware appears to support
> - * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
> - * only used by this driver during tegra_dma_terminate_all()
> - */
> - tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> + tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
> tdma->dma_dev.device_config = tegra_dma_slave_config;
> tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
> tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
^ permalink raw reply
* [4/6] dma: tegra: add accurate reporting of dma state
From: Jon Hunter @ 2018-11-05 11:32 UTC (permalink / raw)
To: Ben Dooks, Dmitry Osipenko
Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra
Hi Ben,
On 05/11/2018 09:03, Ben Dooks wrote:
> On 2018-11-03 12:24, Dmitry Osipenko wrote:
>> On 31.10.2018 19:03, Ben Dooks wrote:
>>> The tx_status callback does not report the state of the transfer
>>> beyond complete segments. This causes problems with users such as
>>> ALSA when applications want to know accurately how much data has
>>> been moved.
>>>
>>> This patch addes a function tegra_dma_update_residual() to query
>>> the hardware and modify the residual information accordinly. It
>>> takes into account any hardware issues when trying to read the
>>> state, such as delays between finishing a buffer and signalling
>>> the interrupt.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Can you CC linux-tegra on these patches? I am always interested in
updates to the DMA drivers.
Cheers!
Jon
^ permalink raw reply
* [4/6] dma: tegra: add accurate reporting of dma state
From: Ben Dooks @ 2018-11-05 11:34 UTC (permalink / raw)
To: Jon Hunter, Dmitry Osipenko
Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra
On 05/11/18 11:32, Jon Hunter wrote:
> Hi Ben,
>
> On 05/11/2018 09:03, Ben Dooks wrote:
>> On 2018-11-03 12:24, Dmitry Osipenko wrote:
>>> On 31.10.2018 19:03, Ben Dooks wrote:
>>>> The tx_status callback does not report the state of the transfer
>>>> beyond complete segments. This causes problems with users such as
>>>> ALSA when applications want to know accurately how much data has
>>>> been moved.
>>>>
>>>> This patch addes a function tegra_dma_update_residual() to query
>>>> the hardware and modify the residual information accordinly. It
>>>> takes into account any hardware issues when trying to read the
>>>> state, such as delays between finishing a buffer and signalling
>>>> the interrupt.
>>>>
>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Can you CC linux-tegra on these patches? I am always interested in
> updates to the DMA drivers.
Is linux-tegra@vger.kernel.org not the right address for the list then?
^ permalink raw reply
* [4/6] dma: tegra: add accurate reporting of dma state
From: Jon Hunter @ 2018-11-05 11:38 UTC (permalink / raw)
To: Ben Dooks, Dmitry Osipenko
Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra
On 05/11/2018 11:34, Ben Dooks wrote:
> On 05/11/18 11:32, Jon Hunter wrote:
>> Hi Ben,
>>
>> On 05/11/2018 09:03, Ben Dooks wrote:
>>> On 2018-11-03 12:24, Dmitry Osipenko wrote:
>>>> On 31.10.2018 19:03, Ben Dooks wrote:
>>>>> The tx_status callback does not report the state of the transfer
>>>>> beyond complete segments. This causes problems with users such as
>>>>> ALSA when applications want to know accurately how much data has
>>>>> been moved.
>>>>>
>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>> the hardware and modify the residual information accordinly. It
>>>>> takes into account any hardware issues when trying to read the
>>>>> state, such as delays between finishing a buffer and signalling
>>>>> the interrupt.
>>>>>
>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> Can you CC linux-tegra on these patches? I am always interested in
>> updates to the DMA drivers.
>
> Is linux-tegra@vger.kernel.org not the right address for the list then?
That's the right one. However, for some reason I did not receive the
complete series only a few responses to some patches.
Jon
^ permalink raw reply
* [4/6] dma: tegra: add accurate reporting of dma state
From: Dmitry Osipenko @ 2018-11-05 14:05 UTC (permalink / raw)
To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra
On 05.11.2018 12:03, Ben Dooks wrote:
>
>
> On 2018-11-03 12:24, Dmitry Osipenko wrote:
>> On 31.10.2018 19:03, Ben Dooks wrote:
>>> The tx_status callback does not report the state of the transfer
>>> beyond complete segments. This causes problems with users such as
>>> ALSA when applications want to know accurately how much data has
>>> been moved.
>>>
>>> This patch addes a function tegra_dma_update_residual() to query
>>> the hardware and modify the residual information accordinly. It
>>> takes into account any hardware issues when trying to read the
>>> state, such as delays between finishing a buffer and signalling
>>> the interrupt.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>
>> Hello Ben,
>>
>> Thank you very much for the patch! It fixes "farting sound" for
>> chromium-browser and applications that use chromium-engine (tested on
>> Tegra20) because apparently it tries to use low latency for everything
>> and audio buffer is constantly underflowing without more detailed
>> DMA-progress reporting. See couple more comments below.
>>
>>> ---
>>> drivers/dma/tegra20-apb-dma.c | 94 ++++++++++++++++++++++++++++++++---
>>> 1 file changed, 87 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>> index 4f7d1e576d03..3fa3a1ac4f57 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>> @@ -802,12 +802,96 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
>>> return 0;
>>> }
>>>
>>> +static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
>>> + struct tegra_dma_sg_req *sg_req,
>>> + struct tegra_dma_desc *dma_desc,
>>> + unsigned int residual)
>>> +{
>>> + unsigned long status = 0x0;
>>
>> There is no need to initialize "status" variable.
>
> ok, will check this.
>
>>> + unsigned long wcount;
>>> + unsigned long ahbptr;
>>> + unsigned long tmp = 0x0;
>>> + unsigned int result;
>>> + int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
>>> + int done;
>>> +
>>> + /* if we're not the current request, then don't alter the residual */
>>> + if (sg_req != list_first_entry(&tdc->pending_sg_req,
>>> + struct tegra_dma_sg_req, node)) {
>>> + result = residual;
>>> + ahbptr = 0xffffffff;
>>> + goto done;
>>> + }
>>> +
>>> + /* loop until we have a reliable result for residual */
>>> + do {
>>> + ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
>>> + status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
>>
>>> + tmp = tdc_read(tdc, 0x08); /* total count for debug */
>>
>> Register 0x08 (DMA_BYTE_STA) doesn't present on Tegra20 and "tmp"
>> isn't used anywhere in the code. Please remove it entirely.
>
> ok, fixed
>
>>> +
>>> + /* check status, if channel isn't busy then skip */
>>> + if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
>>> + result = residual;
>>> + break;
>>> + }
>>
>> If "BUSY" is unset, doesn't this mean that transaction could be
>> completed already? I don't quite understand why you want to skip here.
>
> I can't remember what the reasoning behind this was, this work was
> originally done over a year ago and I am not sure if I can find any
> of the notes from this.
>
Looks like it should be safe to remove this hunk. Please consider the removal or provide reasoning in the comment to the code if you'll recall it.
>>> +
>>> + /* if we've got an interrupt pending on the channel, don't
>>> + * try and deal with the residue as the hardware has likely
>>> + * moved on to the next buffer. return all data moved.
>>> + */
>>> + if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
>>> + result = residual - sg_req->req_len;
>>> + break;
>>> + }
>>> +
>>> + if (tdc->tdma->chip_data->support_separate_wcount_reg)
>>> + wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
>>> + else
>>> + wcount = status;
>>> +
>>> + /* If the request is at the full point, then there is a
>>> + * chance that we have read the status register in the
>>> + * middle of the hardware reloading the next buffer.
>>> + *
>>> + * The sequence seems to be at the end of the buffer, to
>>> + * load the new word count before raising the EOC flag (or
>>> + * changing the ping-pong flag which could have also been
>>> + * used to determine a new buffer). This means there is a
>> two whitespaces here-----|
>>
>>> + * small window where we cannot determine zero-done for the
>>> + * current buffer, or moved to next buffer.
>>> + *
>>
>>> + * If done shows 0, then retry the load, as it may hit the
>>> + * above hardware race. We will either get a new value which
>>> + * is from the first buffer, or we get an EOC (new buffer)
>>> + * or both a new value and an EOC...
>>
>> I think we just need to wait 20usec after reading out "words count"
>> and then re-check interrupt status, so transfer is done if interrupt
>> is set and otherwise "words count" value is actual and reliable.
>
> At the moment I have no way of going back and re-testing this code
> against the problem it originally fixed, so would rather not change
> the algorithm in this.
Please tell what was the problem.
>>
>>> + */
>>> + done = get_current_xferred_count(tdc, sg_req, wcount);
>>> + if (done != 0) {
>>> + result = residual - done;
>>> + break;
>>> + }
>>> +
>>> + ndelay(100);
>>
>> There is no ndelay() on ARM, hence your 20usec timeout is 200usec.
>> Please use udelay().
>
> I thought there was one based on the lpj calculations, I will check later.
>
>>> + } while (--retries > 0);
>>> +
>>> + if (retries <= 0) {
>>> + dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
>>> + result = residual;
>>> + }
>>> +
>>> +done:
>>
>> Please rename goto label as it duplicates local variable name.
>
> ok, fixed.
>
>>> + dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
>>> + sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
>>
>> Whitespace just after tab not needed.
>>
>>> +
>>> + return result;
>>> +}
>>> +
>>> static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>>> dma_cookie_t cookie, struct dma_tx_state *txstate)
>>> {
>>> struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> struct tegra_dma_desc *dma_desc;
>>> - struct tegra_dma_sg_req *sg_req;
>>> + struct tegra_dma_sg_req *sg_req = NULL;
>>> enum dma_status ret;
>>> unsigned long flags;
>>> unsigned int residual;
>>> @@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>>> residual = dma_desc->bytes_requested -
>>> (dma_desc->bytes_transferred %
>>> dma_desc->bytes_requested);
>>> + residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);
Line over 80 characters. Please make sure that "scripts/checkpatch.pl" doesn't have valid complains about the patches.
[snip]
^ permalink raw reply
* [v1,2/2] dmaengine: dw: implement per-channel protection control setting
From: Andy Shevchenko @ 2018-11-05 14:22 UTC (permalink / raw)
To: Christian Lamparter
Cc: dmaengine, devicetree, Dan Williams, Vinod Koul, Viresh Kumar,
Rob Herring, Mark Rutland
Thanks for the patch, my comments below.
On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> This patch adds a new device-tree property that allows to
> specify the protection control bits for each DMA channel
> individually.
>
> Setting the "correct" bits can have a huge impact on the
> PPC460EX and APM82181 that use this DMA engine in combination
> with a DesignWare' SATA-II core (sata_dwc_460ex driver).
>
> In the OpenWrt Forum, the user takimata reported that:
> |<https://forum.lede-project.org/t/wd-mybook-live-duo-two-disks/16195/55>
You may use BugLink: tag at the end of commit message.
> |It seems your patch unleashed the full power of the SATA port.
> |Where I was previously hitting a really hard limit at around
> |82 MB/s for reading and 27 MB/s for writing, I am now getting this:
> |
> |root@OpenWrt:/mnt# time dd if=/dev/zero of=tempfile bs=1M count=1024
> |1024+0 records in
> |1024+0 records out
> |real 0m 13.65s
> |user 0m 0.01s
> |sys 0m 11.89s
> |
> |root@OpenWrt:/mnt# time dd if=tempfile of=/dev/null bs=1M count=1024
> |1024+0 records in
> |1024+0 records out
> |real 0m 8.41s
> |user 0m 0.01s
> |sys 0m 4.70s
> |
> |This means: 121 MB/s reading and 75 MB/s writing!
> |
> |The drive is a WD Green WD10EARX taken from an older MBL Single.
> |I repeated the test a few times with even larger files to rule out
> |any caching, I'm still seeing the same great performance. OpenWrt is
> |now completely on par with the original MBL firmware's performance.
>
> Another user And.short reported in the same thread:
> |<https://forum.openwrt.org/t/solved-wd-mybook-live-duo-two-disks/16195/50>
Another BugLink: tag entry :-)
> |I can report that your fix worked! Boots up fine with two
> |drives even with more partitions, and no more reboot on
> |concurrent disk access!
>
> A closer look into the sata_dwc_460ex code revealed that
> the driver did initally set the correct protection control
> bits.
> However, this feature was lost when the sata_dwc_460ex
> driver was converted to the generic DMA driver framework with:
> 8b3444852a2 ("sata_dwc_460ex: move to generic DMA driver").
Fixes: tag.
>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> drivers/dma/dw/core.c | 3 +++
> drivers/dma/dw/platform.c | 12 +++++++++---
> drivers/dma/dw/regs.h | 4 ++++
> include/linux/platform_data/dma-dw.h | 6 ++++++
> 4 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index f43e6dafe446..2db15e9b33a8 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -160,12 +160,15 @@ static void dwc_initialize_chan_idma32(struct dw_dma_chan *dwc)
>
> static void dwc_initialize_chan_dw(struct dw_dma_chan *dwc)
> {
> + struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> + size_t chanidx = (size_t)(dwc - dw->chan);
We have mask field, so, index is a first set bit out of mask, __ffs(mask).
unsigned int protctl = dw->pdata->protctl[__ffs(mask)];
> u32 cfghi = DWC_CFGH_FIFO_MODE;
> u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
> bool hs_polarity = dwc->dws.hs_polarity;
>
> cfghi |= DWC_CFGH_DST_PER(dwc->dws.dst_id);
> cfghi |= DWC_CFGH_SRC_PER(dwc->dws.src_id);
> + cfghi |= DWC_CFGH_PROTCTL(dw->pdata->protctl[chanidx]);
>
> /* Set polarity of handshake interface */
> cfglo |= hs_polarity ? DWC_CFGL_HS_DST_POL | DWC_CFGL_HS_SRC_POL : 0;
> diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> index f62dd0944908..078cca6576c3 100644
> --- a/drivers/dma/dw/platform.c
> +++ b/drivers/dma/dw/platform.c
> @@ -102,7 +102,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct dw_dma_platform_data *pdata;
> - u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], mb[DW_DMA_MAX_NR_CHANNELS];
> + u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], val[DW_DMA_MAX_NR_CHANNELS];
> u32 nr_masters;
> u32 nr_channels;
>
> @@ -154,14 +154,20 @@ dw_dma_parse_dt(struct platform_device *pdev)
> pdata->data_width[tmp] = BIT(arr[tmp] & 0x07);
> }
>
> - if (!of_property_read_u32_array(np, "multi-block", mb, nr_channels)) {
> + if (!of_property_read_u32_array(np, "multi-block", val, nr_channels)) {
> for (tmp = 0; tmp < nr_channels; tmp++)
> - pdata->multi_block[tmp] = mb[tmp];
> + pdata->multi_block[tmp] = val[tmp];
> } else {
> for (tmp = 0; tmp < nr_channels; tmp++)
> pdata->multi_block[tmp] = 1;
> }
>
> + if (!of_property_read_u32_array(np, "snps,dma-protection-control",
> + val, nr_channels)) {
> + for (tmp = 0; tmp < nr_channels; tmp++)
> + pdata->protctl[tmp] = val[tmp];
> + }
> +
> return pdata;
> }
> #else
> diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
> index 09e7dfdbb790..646c9c960c07 100644
> --- a/drivers/dma/dw/regs.h
> +++ b/drivers/dma/dw/regs.h
> @@ -200,6 +200,10 @@ enum dw_dma_msize {
> #define DWC_CFGH_FCMODE (1 << 0)
> #define DWC_CFGH_FIFO_MODE (1 << 1)
> #define DWC_CFGH_PROTCTL(x) ((x) << 2)
> +#define DWC_CFGH_PROTCTL_DATA (0 << 2) /* data access - always set */
> +#define DWC_CFGH_PROTCTL_PRIV (1 << 2) /* privileged -> AHB HPROT[1] */
> +#define DWC_CFGH_PROTCTL_BUFFER (2 << 2) /* bufferable -> AHB HPROT[2] */
> +#define DWC_CFGH_PROTCTL_CACHE (4 << 2) /* cacheable -> AHB HPROT[3] */
> #define DWC_CFGH_DS_UPD_EN (1 << 5)
> #define DWC_CFGH_SS_UPD_EN (1 << 6)
> #define DWC_CFGH_SRC_PER(x) ((x) << 7)
> diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
> index 896cb71a382c..df65e3311a56 100644
> --- a/include/linux/platform_data/dma-dw.h
> +++ b/include/linux/platform_data/dma-dw.h
> @@ -49,6 +49,7 @@ struct dw_dma_slave {
> * @data_width: Maximum data width supported by hardware per AHB master
> * (in bytes, power of 2)
> * @multi_block: Multi block transfers supported by hardware per channel.
> + * @protctl: Protection control signals setting per channel.
> */
> struct dw_dma_platform_data {
> unsigned int nr_channels;
> @@ -65,6 +66,11 @@ struct dw_dma_platform_data {
> unsigned char nr_masters;
> unsigned char data_width[DW_DMA_MAX_NR_MASTERS];
> unsigned char multi_block[DW_DMA_MAX_NR_CHANNELS];
> +#define CHAN_PROTCTL_PRIVILEGED BIT(0)
> +#define CHAN_PROTCTL_BUFFERABLE BIT(1)
> +#define CHAN_PROTCTL_CACHEABLE BIT(2)
> +#define CHAN_PROTCTL_MASK 0x7
GENMASK()
> + unsigned char protctl[DW_DMA_MAX_NR_CHANNELS];
> };
>
> #endif /* _PLATFORM_DATA_DMA_DW_H */
> --
> 2.19.1
>
^ permalink raw reply
* [v1,1/2] dt-bindings: dmaengine: dw-dmac: add protection control property
From: Andy Shevchenko @ 2018-11-05 14:23 UTC (permalink / raw)
To: Christian Lamparter
Cc: dmaengine, devicetree, Dan Williams, Vinod Koul, Viresh Kumar,
Rob Herring, Mark Rutland
On Sun, Nov 04, 2018 at 06:01:38PM +0100, Christian Lamparter wrote:
> This patch adds the per-channel dma protection control
> prop-encoded-array and dt-binding definitions (including
> definitions for the existing channel allocation and priority
> order values based on include/linux/platform_data/dma-dw.h)
> for the DesignWare AHB Central Direct Memory Access Controller.
>
> Note: The protection control signals are one-to-one mapped to
> the AHB HPROT[1:3] signals. The HPROT0 (Data Access) is
> always hardwired to 1 for this controller.
>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> .../devicetree/bindings/dma/snps-dma.txt | 4 ++++
> MAINTAINERS | 4 +++-
> include/dt-bindings/dma/dw-dmac.h | 20 +++++++++++++++++++
> 3 files changed, 27 insertions(+), 1 deletion(-)
> create mode 100644 include/dt-bindings/dma/dw-dmac.h
>
> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> index 39e2b26be344..72b4984a4c18 100644
> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> @@ -27,6 +27,10 @@ Optional properties:
> general purpose DMA channel allocator. False if not passed.
> - multi-block: Multi block transfers supported by hardware. Array property with
> one cell per channel. 0: not supported, 1 (default): supported.
> +- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
> + Array property with one cell per channel. The default value for a channel
> + is 0 (for non-cacheable, strongly ordered, unprivileged data access).
> + Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
>
> Example:
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dacba23b80b4..c35998e20e9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14107,9 +14107,11 @@ SYNOPSYS DESIGNWARE DMAC DRIVER
> M: Viresh Kumar <vireshk@kernel.org>
> R: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> S: Maintained
> +F: Documentation/devicetree/bindings/dma/snps-dma.txt
> +F: drivers/dma/dw/
> +F: include/dt-bindings/dma/dw-dmac.h
> F: include/linux/dma/dw.h
> F: include/linux/platform_data/dma-dw.h
> -F: drivers/dma/dw/
>
> SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
> M: Jose Abreu <Jose.Abreu@synopsys.com>
> diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
> new file mode 100644
> index 000000000000..9152a6e2406c
> --- /dev/null
> +++ b/include/dt-bindings/dma/dw-dmac.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +
> +#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
> +#define __DT_BINDINGS_DMA_DW_DMAC_H__
> +
> +#define DW_DMAC_CHAN_ALLOCATION_ASCENDING 0 /* zero to seven */
> +#define DW_DMAC_CHAN_ALLOCATION_DESCENDING 1 /* seven to zero */
> +#define DW_DMAC_CHAN_PRIORITY_ASCENDING 0 /* chan0 highest */
> +#define DW_DMAC_CHAN_PRIORITY_DESCENDING 1 /* chan7 highest */
> +
> +/*
> + * Protection Control bits provide protection against illegal transactions.
> + * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
> + * The AHB HPROT[0] bit is hardwired to 1: Data Access.
> + */
> +#define DW_DMAC_HPROT1_PRIVILEGED_MODE (1 << 0) /* Privileged Mode */
> +#define DW_DMAC_HPROT2_BUFFERABLE (1 << 1) /* DMA is bufferable */
> +#define DW_DMAC_HPROT3_CACHEABLE (1 << 2) /* DMA is cacheable */
> +
> +#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
> --
> 2.19.1
>
^ permalink raw reply
* [v1,2/2] dmaengine: dw: implement per-channel protection control setting
From: Andy Shevchenko @ 2018-11-05 14:27 UTC (permalink / raw)
To: Christian Lamparter
Cc: dmaengine, devicetree, Dan Williams, Vinod Koul, Viresh Kumar,
Rob Herring, Mark Rutland
On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> > + struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > + size_t chanidx = (size_t)(dwc - dw->chan);
>
> We have mask field, so, index is a first set bit out of mask, __ffs(mask).
>
> unsigned int protctl = dw->pdata->protctl[__ffs(mask)];
dwc->mask, of course.
Also, it's possible to use (though better to check) dwc->chan.chan_id, though I
dunno if it's reliable.
^ permalink raw reply
* [v1,2/2] dmaengine: dw: implement per-channel protection control setting
From: Christian Lamparter @ 2018-11-05 16:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: dmaengine, devicetree, Dan Williams, Vinod Koul, Viresh Kumar,
Rob Herring, Mark Rutland
On Monday, November 5, 2018 3:27:41 PM CET Andy Shevchenko wrote:
> On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
>
> > > + struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > > + size_t chanidx = (size_t)(dwc - dw->chan);
> >
> > We have mask field, so, index is a first set bit out of mask, __ffs(mask).
> >
> > unsigned int protctl = dw->pdata->protctl[__ffs(mask)];
>
> dwc->mask, of course.
Ok, will do. I'll sent a v2 later this week.
> Also, it's possible to use (though better to check) dwc->chan.chan_id,
> though I dunno if it's reliable.
dwc->chan.chan_id is subjected to the chan_allocation setting.
So, if it's set to CHAN_ALLOCATION_DESCENDING the dt prop array values
for the protctl would need to be reversed as well in order to match the
other per-channel settings (for example multiblock).
So, let's not do that since this gets very confusing.
^ permalink raw reply
* [v1,2/2] dmaengine: dw: implement per-channel protection control setting
From: Andy Shevchenko @ 2018-11-05 16:23 UTC (permalink / raw)
To: Christian Lamparter
Cc: dmaengine, devicetree, Dan Williams, Vinod Koul, Viresh Kumar,
Rob Herring, Mark Rutland
On Mon, Nov 05, 2018 at 05:06:20PM +0100, Christian Lamparter wrote:
> On Monday, November 5, 2018 3:27:41 PM CET Andy Shevchenko wrote:
> > On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> > > On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> >
> > > > + struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > > > + size_t chanidx = (size_t)(dwc - dw->chan);
> > >
> > > We have mask field, so, index is a first set bit out of mask, __ffs(mask).
> > >
> > > unsigned int protctl = dw->pdata->protctl[__ffs(mask)];
> >
> > dwc->mask, of course.
> Ok, will do. I'll sent a v2 later this week.
>
> > Also, it's possible to use (though better to check) dwc->chan.chan_id,
> > though I dunno if it's reliable.
> dwc->chan.chan_id is subjected to the chan_allocation setting.
> So, if it's set to CHAN_ALLOCATION_DESCENDING the dt prop array values
> for the protctl would need to be reversed as well in order to match the
> other per-channel settings (for example multiblock).
> So, let's not do that since this gets very confusing.
Yes, that's what I suspected. So, __ffs(dwc->mask) seems feasible approach.
^ permalink raw reply
* [1/2] firmware: add nowarn variant of request_firmware_nowait()
From: Lucas Stach @ 2018-11-05 17:02 UTC (permalink / raw)
To: Luis R . Rodriguez, Vinod Koul
Cc: Greg Kroah-Hartman, dmaengine, linux-kernel, patchwork-lst,
kernel
Device drivers with optional firmware may still want to use the
asynchronous firmware loading interface. To avoid printing a
warning into the kernel log when the optional firmware is
absent, add a nowarn variant of this interface.
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
drivers/base/firmware_loader/main.c | 85 ++++++++++++++++++++---------
include/linux/firmware.h | 12 ++++
2 files changed, 70 insertions(+), 27 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 8e9213b36e31..81f4a5f6aa5b 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -790,34 +790,12 @@ static void request_firmware_work_func(struct work_struct *work)
kfree(fw_work);
}
-/**
- * request_firmware_nowait() - asynchronous version of request_firmware
- * @module: module requesting the firmware
- * @uevent: sends uevent to copy the firmware image if this flag
- * is non-zero else the firmware copy must be done manually.
- * @name: name of firmware file
- * @device: device for which firmware is being loaded
- * @gfp: allocation flags
- * @context: will be passed over to @cont, and
- * @fw may be %NULL if firmware request fails.
- * @cont: function will be called asynchronously when the firmware
- * request is over.
- *
- * Caller must hold the reference count of @device.
- *
- * Asynchronous variant of request_firmware() for user contexts:
- * - sleep for as small periods as possible since it may
- * increase kernel boot time of built-in device drivers
- * requesting firmware in their ->probe() methods, if
- * @gfp is GFP_KERNEL.
- *
- * - can't sleep at all if @gfp is GFP_ATOMIC.
- **/
-int
-request_firmware_nowait(
+
+static int
+_request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
- void (*cont)(const struct firmware *fw, void *context))
+ void (*cont)(const struct firmware *fw, void *context), bool nowarn)
{
struct firmware_work *fw_work;
@@ -835,7 +813,8 @@ request_firmware_nowait(
fw_work->context = context;
fw_work->cont = cont;
fw_work->opt_flags = FW_OPT_NOWAIT |
- (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+ (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER) |
+ (nowarn ? FW_OPT_NO_WARN : 0);
if (!uevent && fw_cache_is_setup(device, name)) {
kfree_const(fw_work->name);
@@ -854,8 +833,60 @@ request_firmware_nowait(
schedule_work(&fw_work->work);
return 0;
}
+
+/**
+ * request_firmware_nowait() - asynchronous version of request_firmware
+ * @module: module requesting the firmware
+ * @uevent: sends uevent to copy the firmware image if this flag
+ * is non-zero else the firmware copy must be done manually.
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ * @gfp: allocation flags
+ * @context: will be passed over to @cont, and
+ * @fw may be %NULL if firmware request fails.
+ * @cont: function will be called asynchronously when the firmware
+ * request is over.
+ *
+ * Caller must hold the reference count of @device.
+ *
+ * Asynchronous variant of request_firmware() for user contexts:
+ * - sleep for as small periods as possible since it may
+ * increase kernel boot time of built-in device drivers
+ * requesting firmware in their ->probe() methods, if
+ * @gfp is GFP_KERNEL.
+ *
+ * - can't sleep at all if @gfp is GFP_ATOMIC.
+ **/
+int
+request_firmware_nowait(
+ struct module *module, bool uevent,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context))
+{
+ return _request_firmware_nowait(module, uevent, name, device, gfp,
+ context, cont, false);
+
+}
EXPORT_SYMBOL(request_firmware_nowait);
+/**
+ * request_firmware_nowait_nowarn() - async version of request_firmware_nowarn
+ *
+ * Similar in fucntion to request_firmware_nowait(), but doesn't print a warning
+ * when the firmware file could not be found.
+ */
+int
+request_firmware_nowait_nowarn(
+ struct module *module, bool uevent,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context))
+{
+ return _request_firmware_nowait(module, uevent, name, device, gfp,
+ context, cont, true);
+
+}
+EXPORT_SYMBOL(request_firmware_nowait_nowarn);
+
#ifdef CONFIG_PM_SLEEP
static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 2dd566c91d44..a6d0bc8273a4 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -48,6 +48,10 @@ int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
void (*cont)(const struct firmware *fw, void *context));
+int request_firmware_nowait_nowarn(
+ struct module *module, bool uevent,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context));
int request_firmware_direct(const struct firmware **fw, const char *name,
struct device *device);
int request_firmware_into_buf(const struct firmware **firmware_p,
@@ -77,6 +81,14 @@ static inline int request_firmware_nowait(
return -EINVAL;
}
+static inline int request_firmware_nowait_nowarn(
+ struct module *module, bool uevent,
+ const char *name, struct device *device, gfp_t gfp, void *context,
+ void (*cont)(const struct firmware *fw, void *context))
+{
+ return -EINVAL;
+}
+
static inline void release_firmware(const struct firmware *fw)
{
}
^ permalink raw reply related
* [2/2] dmaengine: imx-sdma: don't print warning when firmware is absent
From: Lucas Stach @ 2018-11-05 17:02 UTC (permalink / raw)
To: Luis R . Rodriguez, Vinod Koul
Cc: Greg Kroah-Hartman, dmaengine, linux-kernel, patchwork-lst,
kernel
The SDMA firmware is optional and a usable fallback to the internal
ROM firmware is present in the driver. There is already a message
printed informing the user that the internal firmware is used, so
there is no need to print a scary warning for what is normal behavior
on most systems.
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
drivers/dma/imx-sdma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index b4ec2d20e661..c2e7819cecb8 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1784,7 +1784,7 @@ static int sdma_get_firmware(struct sdma_engine *sdma,
{
int ret;
- ret = request_firmware_nowait(THIS_MODULE,
+ ret = request_firmware_nowait_nowarn(THIS_MODULE,
FW_ACTION_HOTPLUG, fw_name, sdma->dev,
GFP_KERNEL, sdma, sdma_load_firmware);
^ permalink raw reply related
* [1/2] firmware: add nowarn variant of request_firmware_nowait()
From: Randy Dunlap @ 2018-11-05 18:35 UTC (permalink / raw)
To: Lucas Stach, Luis R . Rodriguez, Vinod Koul
Cc: Greg Kroah-Hartman, dmaengine, linux-kernel, patchwork-lst,
kernel
Hi,
typo below:
On 11/5/18 9:02 AM, Lucas Stach wrote:
> Device drivers with optional firmware may still want to use the
> asynchronous firmware loading interface. To avoid printing a
> warning into the kernel log when the optional firmware is
> absent, add a nowarn variant of this interface.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> drivers/base/firmware_loader/main.c | 85 ++++++++++++++++++++---------
> include/linux/firmware.h | 12 ++++
> 2 files changed, 70 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 8e9213b36e31..81f4a5f6aa5b 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> +/**
> + * request_firmware_nowait_nowarn() - async version of request_firmware_nowarn
> + *
> + * Similar in fucntion to request_firmware_nowait(), but doesn't print a warning
function
> + * when the firmware file could not be found.
> + */
> +int
> +request_firmware_nowait_nowarn(
> + struct module *module, bool uevent,
> + const char *name, struct device *device, gfp_t gfp, void *context,
> + void (*cont)(const struct firmware *fw, void *context))
> +{
> + return _request_firmware_nowait(module, uevent, name, device, gfp,
> + context, cont, true);
> +
> +}
> +EXPORT_SYMBOL(request_firmware_nowait_nowarn);
> +
> #ifdef CONFIG_PM_SLEEP
> static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
>
^ permalink raw reply
* [v1,1/2] dt-bindings: dmaengine: dw-dmac: add protection control property
From: Rob Herring @ 2018-11-05 23:06 UTC (permalink / raw)
To: Christian Lamparter
Cc: dmaengine, devicetree, Dan Williams, Vinod Koul, Andy Shevchenko,
Viresh Kumar, Mark Rutland
On Sun, Nov 04, 2018 at 06:01:38PM +0100, Christian Lamparter wrote:
> This patch adds the per-channel dma protection control
> prop-encoded-array and dt-binding definitions (including
> definitions for the existing channel allocation and priority
> order values based on include/linux/platform_data/dma-dw.h)
> for the DesignWare AHB Central Direct Memory Access Controller.
>
> Note: The protection control signals are one-to-one mapped to
> the AHB HPROT[1:3] signals. The HPROT0 (Data Access) is
> always hardwired to 1 for this controller.
>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> .../devicetree/bindings/dma/snps-dma.txt | 4 ++++
> MAINTAINERS | 4 +++-
> include/dt-bindings/dma/dw-dmac.h | 20 +++++++++++++++++++
> 3 files changed, 27 insertions(+), 1 deletion(-)
> create mode 100644 include/dt-bindings/dma/dw-dmac.h
>
> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> index 39e2b26be344..72b4984a4c18 100644
> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> @@ -27,6 +27,10 @@ Optional properties:
> general purpose DMA channel allocator. False if not passed.
> - multi-block: Multi block transfers supported by hardware. Array property with
> one cell per channel. 0: not supported, 1 (default): supported.
> +- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
> + Array property with one cell per channel. The default value for a channel
> + is 0 (for non-cacheable, strongly ordered, unprivileged data access).
IIRC, 'strongly ordered' is outside the scope of AHB (and AXI)
definitions. There is a mapping of SO page table entries to bus control
signals, but that's part of the cpu and outside the scope of this doc.
> + Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
Do you really need this to be per channel rather than just per platform?
If anything, the optimal setting is probably based on the memory address
(device, on-chip RAM, or DRAM), not the channel.
I'd expect for most platforms, bufferable works without any s/w issue
(and should be more correct in that coherent allocations are bufferable
(at least for ARM). Cacheable probably has no effect as most systems
don't have coherent i/o (again, at least for ARM).
> Example:
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dacba23b80b4..c35998e20e9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14107,9 +14107,11 @@ SYNOPSYS DESIGNWARE DMAC DRIVER
> M: Viresh Kumar <vireshk@kernel.org>
> R: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> S: Maintained
> +F: Documentation/devicetree/bindings/dma/snps-dma.txt
> +F: drivers/dma/dw/
> +F: include/dt-bindings/dma/dw-dmac.h
> F: include/linux/dma/dw.h
> F: include/linux/platform_data/dma-dw.h
> -F: drivers/dma/dw/
>
> SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
> M: Jose Abreu <Jose.Abreu@synopsys.com>
> diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
> new file mode 100644
> index 000000000000..9152a6e2406c
> --- /dev/null
> +++ b/include/dt-bindings/dma/dw-dmac.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +
> +#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
> +#define __DT_BINDINGS_DMA_DW_DMAC_H__
> +
> +#define DW_DMAC_CHAN_ALLOCATION_ASCENDING 0 /* zero to seven */
> +#define DW_DMAC_CHAN_ALLOCATION_DESCENDING 1 /* seven to zero */
> +#define DW_DMAC_CHAN_PRIORITY_ASCENDING 0 /* chan0 highest */
> +#define DW_DMAC_CHAN_PRIORITY_DESCENDING 1 /* chan7 highest */
These seem unrelated?
> +
> +/*
> + * Protection Control bits provide protection against illegal transactions.
> + * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
> + * The AHB HPROT[0] bit is hardwired to 1: Data Access.
> + */
> +#define DW_DMAC_HPROT1_PRIVILEGED_MODE (1 << 0) /* Privileged Mode */
> +#define DW_DMAC_HPROT2_BUFFERABLE (1 << 1) /* DMA is bufferable */
> +#define DW_DMAC_HPROT3_CACHEABLE (1 << 2) /* DMA is cacheable */
> +
> +#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
> --
> 2.19.1
>
^ permalink raw reply
* [1/2] dt-bindings: dmaengine: usb-dmac: Add binding for r8a77470
From: Rob Herring @ 2018-11-05 23:29 UTC (permalink / raw)
To: Biju Das
Cc: Vinod Koul, Mark Rutland, dmaengine, devicetree, Simon Horman,
Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
linux-renesas-soc
On Thu, 25 Oct 2018 15:53:37 +0100, Biju Das wrote:
> This patch adds usb high-speed dmac binding for r8a77470 (RZ/G1C) SoC.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
> This patch tested against linux-next.
> ---
> Documentation/devicetree/bindings/dma/renesas,usb-dmac.txt | 1 +
> 1 file changed, 1 insertion(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* dma: coh901318: Fix a double-lock bug
From: Jia-Ju Bai @ 2018-11-06 3:33 UTC (permalink / raw)
To: linus.walleij, vkoul, dan.j.williams
Cc: linux-arm-kernel, dmaengine, linux-kernel, Jia-Ju Bai
The function coh901318_alloc_chan_resources() calls spin_lock_irqsave()
before calling coh901318_config().
But coh901318_config() calls spin_lock_irqsave() again in its
definition, which may cause a double-lock bug.
Because coh901318_config() is only called by
coh901318_alloc_chan_resources(), the bug fix is to remove the
calls to spin-lock and -unlock functions in coh901318_config().
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/dma/coh901318.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c
index eebaba3d9e78..fd862a478738 100644
--- a/drivers/dma/coh901318.c
+++ b/drivers/dma/coh901318.c
@@ -1807,8 +1807,6 @@ static int coh901318_config(struct coh901318_chan *cohc,
int channel = cohc->id;
void __iomem *virtbase = cohc->base->virtbase;
- spin_lock_irqsave(&cohc->lock, flags);
-
if (param)
p = param;
else
@@ -1828,8 +1826,6 @@ static int coh901318_config(struct coh901318_chan *cohc,
coh901318_set_conf(cohc, p->config);
coh901318_set_ctrl(cohc, p->ctrl_lli_last);
- spin_unlock_irqrestore(&cohc->lock, flags);
-
return 0;
}
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox