* Re: [RFC v6 1/6] dmaengine: Add Synopsys eDMA IP core driver
From: Vinod Koul @ 2019-05-06 11:20 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci, dmaengine, Dan Williams, Andy Shevchenko, Russell King,
Joao Pinto
In-Reply-To: <0e877ac0115d37e466ac234f47c51cb1cae7f292.1556043127.git.gustavo.pimentel@synopsys.com>
On 23-04-19, 20:30, Gustavo Pimentel wrote:
> Add Synopsys PCIe Endpoint eDMA IP core driver to kernel.
Still an RFC ?
> +static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc)
> +{
> + struct dw_edma_chan *chan = desc->chan;
> + struct dw_edma *dw = chan->chip->dw;
> + struct dw_edma_chunk *chunk;
> +
> + chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);
Looking at the code this should be called from one of the
device_prep_xxx calls so this should not sleep, so GFP_NOWAIT please
(pls audit rest of the mem allocations in the code)
> + if (unlikely(!chunk))
> + return NULL;
> +
> + INIT_LIST_HEAD(&chunk->list);
> + chunk->chan = chan;
> + chunk->cb = !(desc->chunks_alloc % 2);
? why %2?
> +static enum dma_status
> +dw_edma_device_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> + struct dw_edma_desc *desc;
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> + enum dma_status ret;
> + u32 residue = 0;
> +
> + ret = dma_cookie_status(dchan, cookie, txstate);
> + if (ret == DMA_COMPLETE)
> + return ret;
> +
> + if (ret == DMA_IN_PROGRESS && chan->status == EDMA_ST_PAUSE)
> + ret = DMA_PAUSED;
Don't you want to set residue on paused channel, how else will user know
the position of pause?
> +static struct dma_async_tx_descriptor *
> +dw_edma_device_transfer(struct dw_edma_transfer *xfer)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(xfer->dchan);
> + enum dma_transfer_direction direction = xfer->direction;
> + phys_addr_t src_addr, dst_addr;
> + struct scatterlist *sg = NULL;
> + struct dw_edma_chunk *chunk;
> + struct dw_edma_burst *burst;
> + struct dw_edma_desc *desc;
> + u32 cnt;
> + int i;
> +
> + if ((direction == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_WRITE) ||
> + (direction == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ))
> + return NULL;
> +
> + if (xfer->cyclic) {
> + if (!xfer->xfer.cyclic.len || !xfer->xfer.cyclic.cnt)
> + return NULL;
> + } else {
> + if (xfer->xfer.sg.len < 1)
> + return NULL;
> + }
> +
> + if (!chan->configured)
> + return NULL;
> +
> + desc = dw_edma_alloc_desc(chan);
> + if (unlikely(!desc))
> + goto err_alloc;
> +
> + chunk = dw_edma_alloc_chunk(desc);
> + if (unlikely(!chunk))
> + goto err_alloc;
> +
> + src_addr = chan->config.src_addr;
> + dst_addr = chan->config.dst_addr;
> +
> + if (xfer->cyclic) {
> + cnt = xfer->xfer.cyclic.cnt;
> + } else {
> + cnt = xfer->xfer.sg.len;
> + sg = xfer->xfer.sg.sgl;
> + }
> +
> + for (i = 0; i < cnt; i++) {
> + if (!xfer->cyclic && !sg)
> + break;
> +
> + if (chunk->bursts_alloc == chan->ll_max) {
> + chunk = dw_edma_alloc_chunk(desc);
> + if (unlikely(!chunk))
> + goto err_alloc;
> + }
> +
> + burst = dw_edma_alloc_burst(chunk);
> + if (unlikely(!burst))
> + goto err_alloc;
> +
> + if (xfer->cyclic)
> + burst->sz = xfer->xfer.cyclic.len;
> + else
> + burst->sz = sg_dma_len(sg);
> +
> + chunk->ll_region.sz += burst->sz;
> + desc->alloc_sz += burst->sz;
> +
> + if (direction == DMA_DEV_TO_MEM) {
> + burst->sar = src_addr;
We are device to mem, so src is peripheral.. okay
> + if (xfer->cyclic) {
> + burst->dar = xfer->xfer.cyclic.paddr;
> + } else {
> + burst->dar = sg_dma_address(sg);
> + src_addr += sg_dma_len(sg);
and we increment the src, doesn't make sense to me!
> + }
> + } else {
> + burst->dar = dst_addr;
> + if (xfer->cyclic) {
> + burst->sar = xfer->xfer.cyclic.paddr;
> + } else {
> + burst->sar = sg_dma_address(sg);
> + dst_addr += sg_dma_len(sg);
same here as well
> +static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> +{
> + struct dw_edma_desc *desc;
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> +
> + dw_edma_v0_core_clear_done_int(chan);
> +
> + spin_lock_irqsave(&chan->vc.lock, flags);
> + vd = vchan_next_desc(&chan->vc);
> + if (vd) {
> + switch (chan->request) {
> + case EDMA_REQ_NONE:
> + desc = vd2dw_edma_desc(vd);
> + if (desc->chunks_alloc) {
> + chan->status = EDMA_ST_BUSY;
> + dw_edma_start_transfer(chan);
> + } else {
> + list_del(&vd->node);
> + vchan_cookie_complete(vd);
> + chan->status = EDMA_ST_IDLE;
> + }
> + break;
Empty line after each break please
> + case EDMA_REQ_STOP:
> + list_del(&vd->node);
> + vchan_cookie_complete(vd);
> + chan->request = EDMA_REQ_NONE;
> + chan->status = EDMA_ST_IDLE;
Why do we need to track request as well as status?
> +static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> + if (chan->status != EDMA_ST_IDLE)
> + return -EBUSY;
> +
> + dma_cookie_init(dchan);
not using vchan_init() you need to do this and init the lists..?
> +struct dw_edma_transfer {
> + struct dma_chan *dchan;
> + union Xfer {
no camel case please
It would help to run checkpatch with --strict option to find any style
issues and fix them as well
--
~Vinod
^ permalink raw reply
* [PATCH 2/4] arm64: dts: fsl: ls1028a: Add eDMA node
From: Peng Ma @ 2019-05-06 9:03 UTC (permalink / raw)
To: vkoul, robh+dt, shawnguo, mark.rutland, leoyang.li
Cc: dan.j.williams, dmaengine, devicetree, linux-kernel,
linux-arm-kernel, Peng Ma
In-Reply-To: <20190506090344.37784-1-peng.ma@nxp.com>
Add the eDMA device tree nodes for LS1028A devices
Signed-off-by: Peng Ma <peng.ma@nxp.com>
---
arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
index 8116fb3..71b87cb 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -235,6 +235,21 @@
status = "disabled";
};
+ edma0: edma@22c0000 {
+ #dma-cells = <2>;
+ compatible = "fsl,vf610-edma";
+ reg = <0x0 0x22c0000 0x0 0x10000>,
+ <0x0 0x22d0000 0x0 0x10000>,
+ <0x0 0x22e0000 0x0 0x10000>;
+ interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "edma-tx", "edma-err";
+ dma-channels = <32>;
+ clock-names = "dmamux0", "dmamux1";
+ clocks = <&clockgen 4 1>,
+ <&clockgen 4 1>;
+ };
+
gpio1: gpio@2300000 {
compatible = "fsl,qoriq-gpio";
reg = <0x0 0x2300000 0x0 0x10000>;
--
1.7.1
^ permalink raw reply related
* [PATCH 3/4] dmaengine: fsl-edma: support little endian for edma driver
From: Peng Ma @ 2019-05-06 9:03 UTC (permalink / raw)
To: vkoul, robh+dt, shawnguo, mark.rutland, leoyang.li
Cc: dan.j.williams, dmaengine, devicetree, linux-kernel,
linux-arm-kernel, Peng Ma
In-Reply-To: <20190506090344.37784-1-peng.ma@nxp.com>
improve edma driver to support little endian.
Signed-off-by: Peng Ma <peng.ma@nxp.com>
---
drivers/dma/fsl-edma-common.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
index 680b2a0..6bf238e 100644
--- a/drivers/dma/fsl-edma-common.c
+++ b/drivers/dma/fsl-edma-common.c
@@ -83,9 +83,14 @@ void fsl_edma_chan_mux(struct fsl_edma_chan *fsl_chan,
u32 ch = fsl_chan->vchan.chan.chan_id;
void __iomem *muxaddr;
unsigned int chans_per_mux, ch_off;
+ int endian_diff[4] = {3, 1, -1, -3};
chans_per_mux = fsl_chan->edma->n_chans / DMAMUX_NR;
ch_off = fsl_chan->vchan.chan.chan_id % chans_per_mux;
+
+ if (!fsl_chan->edma->big_endian)
+ ch_off += endian_diff[ch_off % 4];
+
muxaddr = fsl_chan->edma->muxbase[ch / chans_per_mux];
slot = EDMAMUX_CHCFG_SOURCE(slot);
--
1.7.1
^ permalink raw reply related
* [PATCH 4/4] dt-bindings: fsl-qdma: Add LS1028A qDMA bindings
From: Peng Ma @ 2019-05-06 9:03 UTC (permalink / raw)
To: vkoul, robh+dt, shawnguo, mark.rutland, leoyang.li
Cc: dan.j.williams, dmaengine, devicetree, linux-kernel,
linux-arm-kernel, Peng Ma
In-Reply-To: <20190506090344.37784-1-peng.ma@nxp.com>
Add LS1028A qDMA controller bindings to fsl-qdma bindings.
Signed-off-by: Peng Ma <peng.ma@nxp.com>
---
Documentation/devicetree/bindings/dma/fsl-qdma.txt | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/dma/fsl-qdma.txt b/Documentation/devicetree/bindings/dma/fsl-qdma.txt
index 6a0ff90..da371c4 100644
--- a/Documentation/devicetree/bindings/dma/fsl-qdma.txt
+++ b/Documentation/devicetree/bindings/dma/fsl-qdma.txt
@@ -7,6 +7,7 @@ Required properties:
- compatible: Must be one of
"fsl,ls1021a-qdma": for LS1021A Board
+ "fsl,ls1028a-qdma": for LS1028A Board
"fsl,ls1043a-qdma": for ls1043A Board
"fsl,ls1046a-qdma": for ls1046A Board
- reg: Should contain the register's base address and length.
--
1.7.1
^ permalink raw reply related
* [PATCH 1/4] arm64: dts: fsl: ls1028a: Add qDMA node
From: Peng Ma @ 2019-05-06 9:03 UTC (permalink / raw)
To: vkoul, robh+dt, shawnguo, mark.rutland, leoyang.li
Cc: dan.j.williams, dmaengine, devicetree, linux-kernel,
linux-arm-kernel, Peng Ma
Add the qDMA device tree nodes for LS1028A devices
Signed-off-by: Peng Ma <peng.ma@nxp.com>
---
arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 20 ++++++++++++++++++++
1 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
index 2896bbc..8116fb3 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -336,6 +336,26 @@
<GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>;
};
+ qdma: dma-controller@8380000 {
+ compatible = "fsl,ls1028a-qdma", "fsl,ls1021a-qdma";
+ reg = <0x0 0x8380000 0x0 0x1000>, /* Controller regs */
+ <0x0 0x8390000 0x0 0x10000>, /* Status regs */
+ <0x0 0x83a0000 0x0 0x40000>; /* Block regs */
+ interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 254 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "qdma-error", "qdma-queue0",
+ "qdma-queue1", "qdma-queue2", "qdma-queue3";
+ dma-channels = <8>;
+ block-number = <1>;
+ block-offset = <0x10000>;
+ fsl,dma-queues = <2>;
+ status-sizes = <64>;
+ queue-sizes = <64 64>;
+ };
+
pcie@1f0000000 { /* Integrated Endpoint Root Complex */
compatible = "pci-host-ecam-generic";
reg = <0x01 0xf0000000 0x0 0x100000>;
--
1.7.1
^ permalink raw reply related
* RE: [RFC v6 0/6] dmaengine: Add Synopsys eDMA IP driver (version 0)
From: Gustavo Pimentel @ 2019-05-06 8:12 UTC (permalink / raw)
To: Gustavo Pimentel, linux-pci@vger.kernel.org,
dmaengine@vger.kernel.org
Cc: Vinod Koul, Andy Shevchenko, Russell King, Lorenzo Pieralisi,
Bjorn Helgaas, Kishon Vijay Abraham I, Joao Pinto
In-Reply-To: <cover.1556043127.git.gustavo.pimentel@synopsys.com>
Hi all,
An gentle reminder...
Thanks.
On Tue, Apr 23, 2019 at 19:30:7, Gustavo Pimentel
<gustavo.pimentel@synopsys.com> wrote:
> Add Synopsys eDMA IP driver (version 0 and for EP side only) to Linux
> kernel. This IP is generally distributed with Synopsys PCIe EndPoint IP
> (depends of the use and licensing agreement), which supports:
> - legacy and unroll modes
> - 16 independent and concurrent channels (8 write + 8 read)
> - supports linked list (scatter-gather) transfer
> - each linked list descriptor can transfer from 1 byte to 4 Gbytes
> - supports cyclic transfer
> - PCIe EndPoint glue-logic
>
> This patch series contains:
> - eDMA core + eDMA core v0 driver (implements the interface with
> DMAengine controller APIs and interfaces with eDMA HW block)
> - eDMA PCIe glue-logic reference driver (attaches to Synopsys EP and
> provides memory access to eDMA core driver)
>
> Gustavo Pimentel (6):
> dmaengine: Add Synopsys eDMA IP core driver
> dmaengine: Add Synopsys eDMA IP version 0 support
> dmaengine: Add Synopsys eDMA IP version 0 debugfs support
> PCI: Add Synopsys endpoint EDDA Device ID
> dmaengine: Add Synopsys eDMA IP PCIe glue-logic
> MAINTAINERS: Add Synopsys eDMA IP driver maintainer
>
> MAINTAINERS | 7 +
> drivers/dma/Kconfig | 2 +
> drivers/dma/Makefile | 1 +
> drivers/dma/dw-edma/Kconfig | 18 +
> drivers/dma/dw-edma/Makefile | 7 +
> drivers/dma/dw-edma/dw-edma-core.c | 920 +++++++++++++++++++++++++++++++
> drivers/dma/dw-edma/dw-edma-core.h | 165 ++++++
> drivers/dma/dw-edma/dw-edma-pcie.c | 229 ++++++++
> drivers/dma/dw-edma/dw-edma-v0-core.c | 348 ++++++++++++
> drivers/dma/dw-edma/dw-edma-v0-core.h | 28 +
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c | 361 ++++++++++++
> drivers/dma/dw-edma/dw-edma-v0-debugfs.h | 28 +
> drivers/dma/dw-edma/dw-edma-v0-regs.h | 158 ++++++
> drivers/misc/pci_endpoint_test.c | 2 +-
> include/linux/dma/edma.h | 47 ++
> include/linux/pci_ids.h | 1 +
> 16 files changed, 2321 insertions(+), 1 deletion(-)
> create mode 100644 drivers/dma/dw-edma/Kconfig
> create mode 100644 drivers/dma/dw-edma/Makefile
> create mode 100644 drivers/dma/dw-edma/dw-edma-core.c
> create mode 100644 drivers/dma/dw-edma/dw-edma-core.h
> create mode 100644 drivers/dma/dw-edma/dw-edma-pcie.c
> create mode 100644 drivers/dma/dw-edma/dw-edma-v0-core.c
> create mode 100644 drivers/dma/dw-edma/dw-edma-v0-core.h
> create mode 100644 drivers/dma/dw-edma/dw-edma-v0-debugfs.c
> create mode 100644 drivers/dma/dw-edma/dw-edma-v0-debugfs.h
> create mode 100644 drivers/dma/dw-edma/dw-edma-v0-regs.h
> create mode 100644 include/linux/dma/edma.h
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> --
> 2.7.4
^ permalink raw reply
* [PATCH v2 0/6] Fix some bugs and add new feature for Spreadtrum DMA engine
From: Baolin Wang @ 2019-05-06 7:28 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: eric.long, orsonzhai, zhang.lyra, vincent.guittot, baolin.wang,
dmaengine, linux-kernel
Hi,
This patch set fixes some DMA engine bugs and adds interrupt support
for 2-stage transfer.
Changes from v1:
- Improve the commit message for patch 1.
- Drop patch 4 from the v1 patch set, and I will create another patch
set to move the fix to the core.
Baolin Wang (3):
dmaengine: sprd: Fix the possible crash when getting descriptor
status
dmaengine: sprd: Add validation of current descriptor in irq handler
dmaengine: sprd: Add interrupt support for 2-stage transfer
Eric Long (3):
dmaengine: sprd: Fix the incorrect start for 2-stage destination
channels
dmaengine: sprd: Fix block length overflow
dmaengine: sprd: Fix the right place to configure 2-stage transfer
drivers/dma/sprd-dma.c | 49 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 38 insertions(+), 11 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [PATCH v2 1/6] dmaengine: sprd: Fix the possible crash when getting descriptor status
From: Baolin Wang @ 2019-05-06 7:28 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: eric.long, orsonzhai, zhang.lyra, vincent.guittot, baolin.wang,
dmaengine, linux-kernel
In-Reply-To: <cover.1557127239.git.baolin.wang@linaro.org>
We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
has been submitted, that will crash the kernel when getting the descriptor
status.
In this case, since the descriptor has been submitted to process, but it
is not completed now, which means the descriptor is listed into the
'vc->desc_submitted' list now. So we can not get current processing descriptor
by vchan_find_desc(), but the pointer 'schan->cur_desc' will point to the
current processing descriptor, then we can use 'schan->cur_desc' to get
current processing descriptor's status to avoid this issue.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/dma/sprd-dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 48431e2..e29342a 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
else
pos = 0;
} else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
- struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
+ struct sprd_dma_desc *sdesc = schan->cur_desc;
if (sdesc->dir == DMA_DEV_TO_MEM)
pos = sprd_dma_get_dst_addr(schan);
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 2/6] dmaengine: sprd: Add validation of current descriptor in irq handler
From: Baolin Wang @ 2019-05-06 7:28 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: eric.long, orsonzhai, zhang.lyra, vincent.guittot, baolin.wang,
dmaengine, linux-kernel
In-Reply-To: <cover.1557127239.git.baolin.wang@linaro.org>
When user terminates one DMA channel to free all its descriptors, but
at the same time one transaction interrupt was triggered possibly, now
we should not handle this interrupt by validating if the 'schan->cur_desc'
was set as NULL to avoid crashing the kernel.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/dma/sprd-dma.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index e29342a..431e289 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -552,12 +552,17 @@ static irqreturn_t dma_irq_handle(int irq, void *dev_id)
schan = &sdev->channels[i];
spin_lock(&schan->vc.lock);
+
+ sdesc = schan->cur_desc;
+ if (!sdesc) {
+ spin_unlock(&schan->vc.lock);
+ return IRQ_HANDLED;
+ }
+
int_type = sprd_dma_get_int_type(schan);
req_type = sprd_dma_get_req_type(schan);
sprd_dma_clear_int(schan);
- sdesc = schan->cur_desc;
-
/* cyclic mode schedule callback */
cyclic = schan->linklist.phy_addr ? true : false;
if (cyclic == true) {
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 3/6] dmaengine: sprd: Fix the incorrect start for 2-stage destination channels
From: Baolin Wang @ 2019-05-06 7:28 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: eric.long, orsonzhai, zhang.lyra, vincent.guittot, baolin.wang,
dmaengine, linux-kernel
In-Reply-To: <cover.1557127239.git.baolin.wang@linaro.org>
From: Eric Long <eric.long@unisoc.com>
The 2-stage destination channel will be triggered by source channel
automatically, which means we should not trigger it by software request.
Signed-off-by: Eric Long <eric.long@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/dma/sprd-dma.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 431e289..0f92e60 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -510,7 +510,9 @@ static void sprd_dma_start(struct sprd_dma_chn *schan)
sprd_dma_set_uid(schan);
sprd_dma_enable_chn(schan);
- if (schan->dev_id == SPRD_DMA_SOFTWARE_UID)
+ if (schan->dev_id == SPRD_DMA_SOFTWARE_UID &&
+ schan->chn_mode != SPRD_DMA_DST_CHN0 &&
+ schan->chn_mode != SPRD_DMA_DST_CHN1)
sprd_dma_soft_request(schan);
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 4/6] dmaengine: sprd: Fix block length overflow
From: Baolin Wang @ 2019-05-06 7:28 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: eric.long, orsonzhai, zhang.lyra, vincent.guittot, baolin.wang,
dmaengine, linux-kernel
In-Reply-To: <cover.1557127239.git.baolin.wang@linaro.org>
From: Eric Long <eric.long@unisoc.com>
The maximum value of block length is 0xffff, so if the configured transfer length
is more than 0xffff, that will cause block length overflow to lead a configuration
error.
Thus we can set block length as the maximum burst length to avoid this issue, since
the maximum burst length will not be a big value which is more than 0xffff.
Signed-off-by: Eric Long <eric.long@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/dma/sprd-dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 0f92e60..a01c232 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -778,7 +778,7 @@ static int sprd_dma_fill_desc(struct dma_chan *chan,
temp |= slave_cfg->src_maxburst & SPRD_DMA_FRG_LEN_MASK;
hw->frg_len = temp;
- hw->blk_len = len & SPRD_DMA_BLK_LEN_MASK;
+ hw->blk_len = slave_cfg->src_maxburst & SPRD_DMA_BLK_LEN_MASK;
hw->trsc_len = len & SPRD_DMA_TRSC_LEN_MASK;
temp = (dst_step & SPRD_DMA_TRSF_STEP_MASK) << SPRD_DMA_DEST_TRSF_STEP_OFFSET;
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 5/6] dmaengine: sprd: Fix the right place to configure 2-stage transfer
From: Baolin Wang @ 2019-05-06 7:28 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: eric.long, orsonzhai, zhang.lyra, vincent.guittot, baolin.wang,
dmaengine, linux-kernel
In-Reply-To: <cover.1557127239.git.baolin.wang@linaro.org>
From: Eric Long <eric.long@unisoc.com>
Move the 2-stage configuration before configuring the link-list mode,
since we will use some 2-stage configuration to fill the link-list
configuration.
Signed-off-by: Eric Long <eric.long@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/dma/sprd-dma.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index a01c232..01abed5 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -911,6 +911,12 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
schan->linklist.virt_addr = 0;
}
+ /* Set channel mode and trigger mode for 2-stage transfer */
+ schan->chn_mode =
+ (flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
+ schan->trg_mode =
+ (flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
+
sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
if (!sdesc)
return NULL;
@@ -944,12 +950,6 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
}
}
- /* Set channel mode and trigger mode for 2-stage transfer */
- schan->chn_mode =
- (flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
- schan->trg_mode =
- (flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
-
ret = sprd_dma_fill_desc(chan, &sdesc->chn_hw, 0, 0, src, dst, len,
dir, flags, slave_cfg);
if (ret) {
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 6/6] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Baolin Wang @ 2019-05-06 7:28 UTC (permalink / raw)
To: dan.j.williams, vkoul
Cc: eric.long, orsonzhai, zhang.lyra, vincent.guittot, baolin.wang,
dmaengine, linux-kernel
In-Reply-To: <cover.1557127239.git.baolin.wang@linaro.org>
For 2-stage transfer, some users like Audio still need transaction interrupt
to notify when the 2-stage transfer is completed. Thus we should enable
2-stage transfer interrupt to support this feature.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/dma/sprd-dma.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 01abed5..baac476 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -62,6 +62,8 @@
/* SPRD_DMA_GLB_2STAGE_GRP register definition */
#define SPRD_DMA_GLB_2STAGE_EN BIT(24)
#define SPRD_DMA_GLB_CHN_INT_MASK GENMASK(23, 20)
+#define SPRD_DMA_GLB_DEST_INT BIT(22)
+#define SPRD_DMA_GLB_SRC_INT BIT(20)
#define SPRD_DMA_GLB_LIST_DONE_TRG BIT(19)
#define SPRD_DMA_GLB_TRANS_DONE_TRG BIT(18)
#define SPRD_DMA_GLB_BLOCK_DONE_TRG BIT(17)
@@ -135,6 +137,7 @@
/* define DMA channel mode & trigger mode mask */
#define SPRD_DMA_CHN_MODE_MASK GENMASK(7, 0)
#define SPRD_DMA_TRG_MODE_MASK GENMASK(7, 0)
+#define SPRD_DMA_INT_TYPE_MASK GENMASK(7, 0)
/* define the DMA transfer step type */
#define SPRD_DMA_NONE_STEP 0
@@ -190,6 +193,7 @@ struct sprd_dma_chn {
u32 dev_id;
enum sprd_dma_chn_mode chn_mode;
enum sprd_dma_trg_mode trg_mode;
+ enum sprd_dma_int_type int_type;
struct sprd_dma_desc *cur_desc;
};
@@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
val |= SPRD_DMA_GLB_2STAGE_EN;
+ if (schan->int_type != SPRD_DMA_NO_INT)
+ val |= SPRD_DMA_GLB_SRC_INT;
+
sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
break;
@@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
val |= SPRD_DMA_GLB_2STAGE_EN;
+ if (schan->int_type != SPRD_DMA_NO_INT)
+ val |= SPRD_DMA_GLB_SRC_INT;
+
sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
break;
@@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
SPRD_DMA_GLB_DEST_CHN_MASK;
val |= SPRD_DMA_GLB_2STAGE_EN;
+ if (schan->int_type != SPRD_DMA_NO_INT)
+ val |= SPRD_DMA_GLB_DEST_INT;
+
sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
break;
@@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
SPRD_DMA_GLB_DEST_CHN_MASK;
val |= SPRD_DMA_GLB_2STAGE_EN;
+ if (schan->int_type != SPRD_DMA_NO_INT)
+ val |= SPRD_DMA_GLB_DEST_INT;
+
sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
break;
@@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
schan->linklist.virt_addr = 0;
}
- /* Set channel mode and trigger mode for 2-stage transfer */
+ /*
+ * Set channel mode, interrupt mode and trigger mode for 2-stage
+ * transfer.
+ */
schan->chn_mode =
(flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
schan->trg_mode =
(flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
+ schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
if (!sdesc)
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Baolin Wang @ 2019-05-06 4:48 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <20190502060148.GH3845@vkoul-mobl.Dlink>
Hi Vinod,
On Thu, 2 May 2019 at 14:01, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 30-04-19, 16:53, Baolin Wang wrote:
> > Hi Vinod,
> >
> > On Tue, 30 Apr 2019 at 16:34, Baolin Wang <baolin.wang@linaro.org> wrote:
> > >
> > > On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > On 30-04-19, 13:30, Baolin Wang wrote:
> > > > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> > > > > >
> > > > > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > > > > From: Eric Long <eric.long@unisoc.com>
> > > > > > > > >
> > > > > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > > > > device validation in filter function to check if the correct controller
> > > > > > > > > to be requested.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > > > > > ---
> > > > > > > > > drivers/dma/sprd-dma.c | 5 +++++
> > > > > > > > > 1 file changed, 5 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > > > > index 0f92e60..9f99d4b 100644
> > > > > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > > > > > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > > > > > {
> > > > > > > > > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > > > > + struct of_phandle_args *dma_spec =
> > > > > > > > > + container_of(param, struct of_phandle_args, args[0]);
> > > > > > > > > u32 slave_id = *(u32 *)param;
> > > > > > > > >
> > > > > > > > > + if (chan->device->dev->of_node != dma_spec->np)
> > > > > > > >
> > > > > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > > > > be useless!
> > > > > > >
> > > > > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > > > > complicated than current solution. Since we need introduce one
> > > > > > > structure to save the node to validate in the filter function like
> > > > > > > below, which seems make things complicated. But if you still like to
> > > > > > > use of_dma_find_controller(), I can change to use it in next version.
> > > > > >
> > > > > > Sorry I should have clarified more..
> > > > > >
> > > > > > of_dma_find_controller() is called by xlate, so you already run this
> > > > > > check, so why use this :)
> > > > >
> > > > > The of_dma_find_controller() can save the requested device node into
> > > > > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > > > > dma_request_channel() to request one channel, but it did not validate
> > > > > the device node to find the corresponding dma device in
> > > > > dma_request_channel(). So we should in our filter function to validate
> > > > > the device node with the device node specified by the dma_spec. Hope I
> > > > > make things clear.
> > > >
> > > > But dma_request_channel() calls of_dma_request_slave_channel() which
> > > > invokes of_dma_find_controller() why is it broken for you if that is the
> > > > case..
> > >
> > > No,the calling process should be:
> > > dma_request_slave_channel()
> > > --->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
> > > ----> dma_request_channel().
>
> The thing is that this is a generic issue, so fix should be in the core
> and not in the driver. Agree in you case of_dma_find_controller() is not
> invoked, so we should fix that in core
>
> >
> > You can check other drivers, they also will save the device node to
> > validate in their filter function.
> > For example the imx-sdma driver:
> > https://elixir.bootlin.com/linux/v5.1-rc6/source/drivers/dma/imx-sdma.c#L1931
>
> Exactly, more the reason this should be in core :)
Sorry for late reply due to my holiday.
OK, I can move the fix into the core. So I think I will drop this
patch from my patchset, and I will create another patch set to fix the
device node validation issue with converting other drivers which did
the similar things. Thanks.
--
Baolin Wang
Best Regards
^ permalink raw reply
* [V2 2/2] dmaengine: fsl-qdma: Add improvement
From: Peng Ma @ 2019-05-06 2:21 UTC (permalink / raw)
To: vkoul, dan.j.williams, leoyang.li; +Cc: dmaengine, linux-kernel, Peng Ma
In-Reply-To: <20190506022111.31751-1-peng.ma@nxp.com>
When an error occurs we should clean the error register then to return
Signed-off-by: Peng Ma <peng.ma@nxp.com>
---
changed for V2:
- Separate one patch to two patchs
drivers/dma/fsl-qdma.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c
index 2e8b46b..542765a 100644
--- a/drivers/dma/fsl-qdma.c
+++ b/drivers/dma/fsl-qdma.c
@@ -710,10 +710,8 @@ static irqreturn_t fsl_qdma_error_handler(int irq, void *dev_id)
intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
- if (intr) {
+ if (intr)
dev_err(fsl_qdma->dma_dev.dev, "DMA transaction error!\n");
- return IRQ_NONE;
- }
qdma_writel(fsl_qdma, FSL_QDMA_DEDR_CLEAR, status + FSL_QDMA_DEDR);
return IRQ_HANDLED;
--
1.7.1
^ permalink raw reply related
* [V2 1/2] dmaengine: fsl-qdma: fixed the source/destination descriptor format
From: Peng Ma @ 2019-05-06 2:21 UTC (permalink / raw)
To: vkoul, dan.j.williams, leoyang.li; +Cc: dmaengine, linux-kernel, Peng Ma
CMD of Source/Destination descriptor format should be lower of
struct fsl_qdma_engine number data address.
Signed-off-by: Peng Ma <peng.ma@nxp.com>
---
changed for V2:
- Fix descriptor spelling
drivers/dma/fsl-qdma.c | 25 +++++++++++++++++--------
1 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c
index aa1d0ae..2e8b46b 100644
--- a/drivers/dma/fsl-qdma.c
+++ b/drivers/dma/fsl-qdma.c
@@ -113,6 +113,7 @@
/* Field definition for Descriptor offset */
#define QDMA_CCDF_STATUS 20
#define QDMA_CCDF_OFFSET 20
+#define QDMA_SDDF_CMD(x) (((u64)(x)) << 32)
/* Field definition for safe loop count*/
#define FSL_QDMA_HALT_COUNT 1500
@@ -214,6 +215,12 @@ struct fsl_qdma_engine {
};
+static inline void
+qdma_sddf_set_cmd(struct fsl_qdma_format *sddf, u32 val)
+{
+ sddf->data = QDMA_SDDF_CMD(val);
+}
+
static inline u64
qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf)
{
@@ -341,6 +348,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan *chan)
static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp *fsl_comp,
dma_addr_t dst, dma_addr_t src, u32 len)
{
+ u32 cmd;
struct fsl_qdma_format *sdf, *ddf;
struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src, *csgf_dest;
@@ -353,6 +361,7 @@ static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp *fsl_comp,
memset(fsl_comp->virt_addr, 0, FSL_QDMA_COMMAND_BUFFER_SIZE);
memset(fsl_comp->desc_virt_addr, 0, FSL_QDMA_DESCRIPTOR_BUFFER_SIZE);
+
/* Head Command Descriptor(Frame Descriptor) */
qdma_desc_addr_set64(ccdf, fsl_comp->bus_addr + 16);
qdma_ccdf_set_format(ccdf, qdma_ccdf_get_offset(ccdf));
@@ -369,14 +378,14 @@ static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp *fsl_comp,
/* This entry is the last entry. */
qdma_csgf_set_f(csgf_dest, len);
/* Descriptor Buffer */
- sdf->data =
- cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
- FSL_QDMA_CMD_RWTTYPE_OFFSET);
- ddf->data =
- cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
- FSL_QDMA_CMD_RWTTYPE_OFFSET);
- ddf->data |=
- cpu_to_le64(FSL_QDMA_CMD_LWC << FSL_QDMA_CMD_LWC_OFFSET);
+ cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
+ FSL_QDMA_CMD_RWTTYPE_OFFSET);
+ qdma_sddf_set_cmd(sdf, cmd);
+
+ cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
+ FSL_QDMA_CMD_RWTTYPE_OFFSET);
+ cmd |= cpu_to_le32(FSL_QDMA_CMD_LWC << FSL_QDMA_CMD_LWC_OFFSET);
+ qdma_sddf_set_cmd(ddf, cmd);
}
/*
--
1.7.1
^ permalink raw reply related
* [PATCH v1] dmaengine: tegra-apb: Handle DMA_PREP_INTERRUPT flag properly
From: Dmitry Osipenko @ 2019-05-05 18:12 UTC (permalink / raw)
To: Laxman Dewangan, Vinod Koul, Thierry Reding, Jonathan Hunter,
Ben Dooks
Cc: dmaengine, linux-tegra, linux-kernel
The DMA_PREP_INTERRUPT flag means that descriptor's callback should be
invoked upon transfer completion and that's it. For some reason driver
completely disables the hardware interrupt handling, leaving channel in
unusable state if transfer is issued with the flag being unset. Note
that there are no occurrences in the relevant drivers that do not set
the flag, hence this patch doesn't fix any actual bug and merely fixes
potential problem.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/dma/tegra20-apb-dma.c | 41 ++++++++++++++++++++++++-----------
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index cf462b1abc0b..29d972b7546f 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -561,6 +561,9 @@ static void tegra_dma_abort_all(struct tegra_dma_channel *tdc)
dma_desc->dma_status = DMA_ERROR;
list_add_tail(&dma_desc->node, &tdc->free_dma_desc);
+ if (dma_desc->cb_count < 0)
+ continue;
+
/* Add in cb list if it is not there. */
if (!dma_desc->cb_count)
list_add_tail(&dma_desc->cb_node,
@@ -616,9 +619,13 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
if (sgreq->last_sg) {
dma_desc->dma_status = DMA_COMPLETE;
dma_cookie_complete(&dma_desc->txd);
- if (!dma_desc->cb_count)
- list_add_tail(&dma_desc->cb_node, &tdc->cb_desc);
- dma_desc->cb_count++;
+ if (dma_desc->cb_count >= 0) {
+ if (!dma_desc->cb_count)
+ list_add_tail(&dma_desc->cb_node,
+ &tdc->cb_desc);
+
+ dma_desc->cb_count++;
+ }
list_add_tail(&dma_desc->node, &tdc->free_dma_desc);
}
list_add_tail(&sgreq->node, &tdc->free_sg_req);
@@ -645,9 +652,11 @@ static void handle_cont_sngl_cycle_dma_done(struct tegra_dma_channel *tdc,
dma_desc->bytes_requested;
/* Callback need to be call */
- if (!dma_desc->cb_count)
- list_add_tail(&dma_desc->cb_node, &tdc->cb_desc);
- dma_desc->cb_count++;
+ if (dma_desc->cb_count >= 0) {
+ if (!dma_desc->cb_count)
+ list_add_tail(&dma_desc->cb_node, &tdc->cb_desc);
+ dma_desc->cb_count++;
+ }
/* If not last req then put at end of pending list */
if (!list_is_last(&sgreq->node, &tdc->pending_sg_req)) {
@@ -802,7 +811,7 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
dma_desc = list_first_entry(&tdc->cb_desc,
typeof(*dma_desc), cb_node);
list_del(&dma_desc->cb_node);
- dma_desc->cb_count = 0;
+ dma_desc->cb_count = -1;
}
spin_unlock_irqrestore(&tdc->lock, flags);
return 0;
@@ -988,8 +997,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
csr |= tdc->slave_id << TEGRA_APBDMA_CSR_REQ_SEL_SHIFT;
}
- if (flags & DMA_PREP_INTERRUPT)
- csr |= TEGRA_APBDMA_CSR_IE_EOC;
+ csr |= TEGRA_APBDMA_CSR_IE_EOC;
apb_seq |= TEGRA_APBDMA_APBSEQ_WRAP_WORD_1;
@@ -1000,11 +1008,15 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
}
INIT_LIST_HEAD(&dma_desc->tx_list);
INIT_LIST_HEAD(&dma_desc->cb_node);
- dma_desc->cb_count = 0;
dma_desc->bytes_requested = 0;
dma_desc->bytes_transferred = 0;
dma_desc->dma_status = DMA_IN_PROGRESS;
+ if (flags & DMA_PREP_INTERRUPT)
+ dma_desc->cb_count = 0;
+ else
+ dma_desc->cb_count = -1;
+
/* Make transfer requests */
for_each_sg(sgl, sg, sg_len, i) {
u32 len, mem;
@@ -1131,8 +1143,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
csr |= tdc->slave_id << TEGRA_APBDMA_CSR_REQ_SEL_SHIFT;
}
- if (flags & DMA_PREP_INTERRUPT)
- csr |= TEGRA_APBDMA_CSR_IE_EOC;
+ csr |= TEGRA_APBDMA_CSR_IE_EOC;
apb_seq |= TEGRA_APBDMA_APBSEQ_WRAP_WORD_1;
@@ -1144,7 +1155,11 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
INIT_LIST_HEAD(&dma_desc->tx_list);
INIT_LIST_HEAD(&dma_desc->cb_node);
- dma_desc->cb_count = 0;
+
+ if (flags & DMA_PREP_INTERRUPT)
+ dma_desc->cb_count = 0;
+ else
+ dma_desc->cb_count = -1;
dma_desc->bytes_transferred = 0;
dma_desc->bytes_requested = buf_len;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] dma: tegra: add accurate reporting of dma state
From: Dmitry Osipenko @ 2019-05-05 13:39 UTC (permalink / raw)
To: Ben Dooks, linux-kernel
Cc: Laxman Dewangan, Jon Hunter, Vinod Koul, Dan Williams,
Thierry Reding, dmaengine, linux-tegra, linux-kernel
In-Reply-To: <6631ad02-0e3c-d768-8c23-fd1f091402df@gmail.com>
04.05.2019 19:06, Dmitry Osipenko пишет:
> 01.05.2019 11:58, Ben Dooks пишет:
>> On 24/04/2019 19:17, Dmitry Osipenko wrote:
>>> 24.04.2019 19:23, 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>
>>>
>>> Hello Ben,
>>>
>>> Thank you very much for keeping it up. I have couple comments, please
>>> see them below.
>>>
>>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>>> Cc: Laxman Dewangan <ldewangan@nvidia.com> (supporter:TEGRA DMA DRIVERS)
>>>> Cc: Jon Hunter <jonathanh@nvidia.com> (supporter:TEGRA DMA DRIVERS)
>>>> Cc: Vinod Koul <vkoul@kernel.org> (maintainer:DMA GENERIC OFFLOAD
>>>> ENGINE SUBSYSTEM)
>>>> Cc: Dan Williams <dan.j.williams@intel.com> (reviewer:ASYNCHRONOUS
>>>> TRANSFERS/TRANSFORMS (IOAT) API)
>>>> Cc: Thierry Reding <thierry.reding@gmail.com> (supporter:TEGRA
>>>> ARCHITECTURE SUPPORT)
>>>> Cc: dmaengine@vger.kernel.org (open list:DMA GENERIC OFFLOAD ENGINE
>>>> SUBSYSTEM)
>>>> Cc: linux-tegra@vger.kernel.org (open list:TEGRA ARCHITECTURE SUPPORT)
>>>> Cc: linux-kernel@vger.kernel.org (open list)
>>>> ---
>>>> drivers/dma/tegra20-apb-dma.c | 92 ++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 86 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/tegra20-apb-dma.c
>>>> b/drivers/dma/tegra20-apb-dma.c
>>>> index cf462b1abc0b..544e7273e741 100644
>>>> --- a/drivers/dma/tegra20-apb-dma.c
>>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>>> @@ -808,6 +808,90 @@ 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;
>>>
>>> You could pre-assign ahbptr=0xffffffff and result=residual here, then
>>> you could remove all the duplicated assigns below.
>>
>> ok, ta.
>>
>>>> + 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 */
>>>
>>> The "tmp" variable isn't used anywhere in the code, please remove it.
>>
>> must have been left over.
>>
>>>> +
>>>> + /* check status, if channel isn't busy then skip */
>>>> + if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
>>>> + result = residual;
>>>> + break;
>>>> + }
>>>
>>> This doesn't look correct because TRM says "Busy bit gets set as soon
>>> as a channel is enabled and gets cleared after transfer completes",
>>> hence a cleared BUSY bit means that all transfers are completed and
>>> result=residual is incorrect here. Given that there is a check for EOC
>>> bit being set below, this hunk should be removed.
>>
>> I'll check notes, but see below.
>>
>>>> +
>>>> + /* 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);
>>>
>>> Please use udelay(1) because there is no ndelay on arm32 and
>>> ndelay(100) is getting rounded up to 1usec. AFAIK, arm64 doesn't have
>>> reliable ndelay on Tegra either because timer rate changes with the
>>> CPU frequency scaling.
>>
>> I'll check, but last time it was implemented. This seems a backwards step.
>>
>>> Secondly done=0 isn't a error case, technically this could be the case
>>> when tegra_dma_update_residual() is invoked just after starting the
>>> transfer. Hence I think this do-while loop and timeout checking aren't
>>> needed at all since done=0 is a perfectly valid case.
>>
>> this is not checking for an error, it's checking for a possible
>> inaccurate reading.
>
> If you'll change reading order of the status / words registers like I
> suggested, then there won't be a case for the inaccuracy.
>
> The EOC bit should be set atomically once transfer is finished, you
> can't get wrapped around words count and EOC bit not being set.
>
> For oneshot transfer that runs with interrupt being disabled, the words
> counter will stop at 0 and the unset BUSY bit will indicate that the
> transfer is completed.
>
>>>
>>> Altogether seems the tegra_dma_update_residual() could be reduced to:
>>>
>>> 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, wcount;
>>>
>>> if (list_is_first(&sg_req->node, &tdc->pending_sg_req))
>>> return residual;
>>>
>>> if (tdc->tdma->chip_data->support_separate_wcount_reg)
>>> wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
>>>
>>> status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
>>>
>>> if (!tdc->tdma->chip_data->support_separate_wcount_reg)
>>> wcount = status;
>>>
>>> if (status & TEGRA_APBDMA_STATUS_ISE_EOC)
>>> return residual - sg_req->req_len;
>>>
>>> return residual - get_current_xferred_count(tdc, sg_req, wcount);
>>> }
>>
>> I'm not sure if that will work all the time. It took days of testing to
>> get reliable error data for the cases we're looking for here.
>
> Could you please tell exactly what those cases are. I don't see when the
> simplified variant could fail, but maybe I already forgot some extra
> details about how APB DMA works.
>
> I tested the variant I'm suggesting (with the fixed typos and added
> check for the BUSY bit) and it works absolutely fine, audio stuttering
> issue is fixed, everything else works too. Please consider to use it for
> the next version of the patch if there are no objections.
>
Actually the BUSY bit checking shouldn't be needed. I think it's a bug
in the driver that it may not enable EOC interrupt and will send a patch
to fix it.
^ permalink raw reply
* RE: [EXT] Re: [PATCH v2 08/15] dt-bindings: spi: imx: add i.mx6ul to state errata fixed
From: Robin Gong @ 2019-05-05 8:55 UTC (permalink / raw)
To: Rob Herring
Cc: broonie@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
festevam@gmail.com, mark.rutland@arm.com,
u.kleine-koenig@pengutronix.de, plyatov@gmail.com,
dan.j.williams@intel.com, catalin.marinas@arm.com,
will.deacon@arm.com, dl-linux-imx, linux-spi@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org,
devicetree@vger.kernel.org, kernel@pengutronix.de
In-Reply-To: <20190501200711.GA31231@bogus>
> On Fri, Apr 26, 2019 at 08:05:51AM +0000, Robin Gong wrote:
> > ERR009165 fixed from i.mx6ul, add it to show the errata fixed.
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > ---
> > Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> > b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> > index 2d32641..32c4263d 100644
> > --- a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> > +++ b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> > @@ -10,6 +10,8 @@ Required properties:
> > - "fsl,imx35-cspi" for SPI compatible with the one integrated on i.MX35
> > - "fsl,imx51-ecspi" for SPI compatible with the one integrated on i.MX51
> > - "fsl,imx53-ecspi" for SPI compatible with the one integrated on
> > i.MX53 and later Soc
> > + - "fsl,imx6ul-ecspi" ERR009165 fixed on i.MX6UL and later Soc
> > +
> > + (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> > +
> ww.nxp.com%2Fdocs%2Fen%2Ferrata%2FIMX6DQCE.pdf&data=02%7C01
> %7Cyi
> > +
> bin.gong%40nxp.com%7C1eb9b302759b4af6fbe408d6ce709b8b%7C686ea1d
> 3bc2b
> > +
> 4c6fa92cd99c5c301635%7C0%7C1%7C636923380371230101&sdata=%
> 2BxM9fN
> > + 6aEFkNlY5KU9qNiqqFMuDEfqGNrzADDiPO9gQ%3D&reserved=0)
>
> What about other i.MX6 chips?
I only state in the cover letter of this patch set, for i.mx6q/dl/sl/sx still need this
errata which is fixed i.mx chips after i.mx6ul including i.mx6ull,i.mx8m,i.mx8mm.
I'll double confirm again and describe it clearly in commit log in v3.
>
> Seems like this is missing some fallbacks. The binding doc should make it clear
> what are all valid combinations of compatible strings.
In another Uwe's comment, I'm thinking move such errata information into spi
driver level which makes binding doc clear. What do you think?
>
> > - "fsl,imx8mq-ecspi" for SPI compatible with the one integrated on
> > i.MX8M
> > - reg : Offset and length of the register set for the device
> > - interrupts : Should contain CSPI/eCSPI interrupt
> > --
> > 2.7.4
> >
^ permalink raw reply
* RE: [EXT] Re: [PATCH v2 08/15] dt-bindings: spi: imx: add i.mx6ul to state errata fixed
From: Robin Gong @ 2019-05-05 8:47 UTC (permalink / raw)
To: Uwe Kleine-König, robh+dt@kernel.org
Cc: broonie@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
festevam@gmail.com, mark.rutland@arm.com, plyatov@gmail.com,
dan.j.williams@intel.com, catalin.marinas@arm.com,
will.deacon@arm.com, dl-linux-imx, linux-spi@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org,
devicetree@vger.kernel.org, kernel@pengutronix.de
In-Reply-To: <20190502065939.nyejomrsowhy6xox@pengutronix.de>
> On Fri, Apr 26, 2019 at 08:05:51AM +0000, Robin Gong wrote:
> > ERR009165 fixed from i.mx6ul, add it to show the errata fixed.
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > ---
> > Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> > b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> > index 2d32641..32c4263d 100644
> > --- a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> > +++ b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> > @@ -10,6 +10,8 @@ Required properties:
> > - "fsl,imx35-cspi" for SPI compatible with the one integrated on i.MX35
> > - "fsl,imx51-ecspi" for SPI compatible with the one integrated on i.MX51
> > - "fsl,imx53-ecspi" for SPI compatible with the one integrated on
> > i.MX53 and later Soc
> > + - "fsl,imx6ul-ecspi" ERR009165 fixed on i.MX6UL and later Soc
> > +
> > + (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> > +
> ww.nxp.com%2Fdocs%2Fen%2Ferrata%2FIMX6DQCE.pdf&data=02%7C01
> %7Cyi
> > +
> bin.gong%40nxp.com%7Cc36708a8219843ffe1d308d6cecbc512%7C686ea1d
> 3bc2b
> > +
> 4c6fa92cd99c5c301635%7C0%7C1%7C636923771907346428&sdata=9e
> 8c8GK2
> > + hSE90rzyArm8elog9dwNqn4Jeoeff79XwI4%3D&reserved=0)
>
> I wouldn't mention this errata in the binding documentation. Just state that
> fsl,imx6ul-ecspi is designed to be used on i.MX6UL. And that it might also be
> used on later SoCs as a "fallback compatible" is a detail that is so common that
Eh...Maybe add one flag member in 'struct spi_imx_devtype_data' to state errata fixed
is better?
> I would expect it not being worth mentioning. So for me it would also be OK to
> drop "fsl,imx53-ecspi" from the list as this is only used like:
>
> compatible = "fsl,imx53-ecspi", "fsl,imx51-ecspi";
>
> (But note that I have no authority here.)
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König
> |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> ngutronix.de%2F&data=02%7C01%7Cyibin.gong%40nxp.com%7Cc36708
> a8219843ffe1d308d6cecbc512%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C1%7C636923771907346428&sdata=McF9ueExCPpCq5Nxa6iJ9h53
> 9d9fO4DIW2IaS90ZdpQ%3D&reserved=0 |
^ permalink raw reply
* [PATCH] dmaengine: jz4780: Fix transfers being ACKed too soon
From: Paul Cercueil @ 2019-05-04 21:37 UTC (permalink / raw)
To: Dan Williams, Vinod Koul; +Cc: od, dmaengine, linux-kernel, Paul Cercueil
When a multi-descriptor DMA transfer is in progress, the "IRQ pending"
flag will apparently be set for that channel as soon as the last
descriptor loads, way before the IRQ actually happens. This behaviour
has been observed on the JZ4725B, but maybe other SoCs are affected.
In the case where another DMA transfer is running into completion on a
separate channel, the IRQ handler would then run the completion handler
for our previous channel even if the transfer didn't actually finish.
Fix this by checking in the completion handler that we're indeed done;
if not the interrupted DMA transfer will simply be resumed.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/dma/dma-jz4780.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 02075417c69f..5c34d23bdea4 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -662,10 +662,11 @@ static enum dma_status jz4780_dma_tx_status(struct dma_chan *chan,
return status;
}
-static void jz4780_dma_chan_irq(struct jz4780_dma_dev *jzdma,
- struct jz4780_dma_chan *jzchan)
+static bool jz4780_dma_chan_irq(struct jz4780_dma_dev *jzdma,
+ struct jz4780_dma_chan *jzchan)
{
uint32_t dcs;
+ bool ack = true;
spin_lock(&jzchan->vchan.lock);
@@ -688,12 +689,20 @@ static void jz4780_dma_chan_irq(struct jz4780_dma_dev *jzdma,
if ((dcs & (JZ_DMA_DCS_AR | JZ_DMA_DCS_HLT)) == 0) {
if (jzchan->desc->type == DMA_CYCLIC) {
vchan_cyclic_callback(&jzchan->desc->vdesc);
- } else {
+
+ jz4780_dma_begin(jzchan);
+ } else if (dcs & JZ_DMA_DCS_TT) {
vchan_cookie_complete(&jzchan->desc->vdesc);
jzchan->desc = NULL;
- }
- jz4780_dma_begin(jzchan);
+ jz4780_dma_begin(jzchan);
+ } else {
+ /* False positive - continue the transfer */
+ ack = false;
+ jz4780_dma_chn_writel(jzdma, jzchan->id,
+ JZ_DMA_REG_DCS,
+ JZ_DMA_DCS_CTE);
+ }
}
} else {
dev_err(&jzchan->vchan.chan.dev->device,
@@ -701,21 +710,22 @@ static void jz4780_dma_chan_irq(struct jz4780_dma_dev *jzdma,
}
spin_unlock(&jzchan->vchan.lock);
+
+ return ack;
}
static irqreturn_t jz4780_dma_irq_handler(int irq, void *data)
{
struct jz4780_dma_dev *jzdma = data;
+ unsigned int nb_channels = jzdma->soc_data->nb_channels;
uint32_t pending, dmac;
int i;
pending = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DIRQP);
- for (i = 0; i < jzdma->soc_data->nb_channels; i++) {
- if (!(pending & (1<<i)))
- continue;
-
- jz4780_dma_chan_irq(jzdma, &jzdma->chan[i]);
+ for_each_set_bit(i, (unsigned long *)&pending, nb_channels) {
+ if (jz4780_dma_chan_irq(jzdma, &jzdma->chan[i]))
+ pending &= ~BIT(i);
}
/* Clear halt and address error status of all channels. */
@@ -724,7 +734,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data)
jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
/* Clear interrupt pending status. */
- jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
+ jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, pending);
return IRQ_HANDLED;
}
--
2.21.0.593.g511ec345e18
^ permalink raw reply related
* [PATCH] dmaengine: jz4780: Use SPDX license notifier
From: Paul Cercueil @ 2019-05-04 21:34 UTC (permalink / raw)
To: Zubair Lutfullah Kakakhel, Vinod Koul, Dan Williams
Cc: od, dmaengine, linux-kernel, Paul Cercueil
Use SPDX license notifier instead of plain text in the header.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/dma/dma-jz4780.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 9ce0a386225b..02075417c69f 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -1,13 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0+
/*
* Ingenic JZ4780 DMA controller
*
* Copyright (c) 2015 Imagination Technologies
* Author: Alex Smith <alex@alex-smith.me.uk>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
*/
#include <linux/clk.h>
--
2.21.0.593.g511ec345e18
^ permalink raw reply related
* Re: [PATCH] dma: tegra: add accurate reporting of dma state
From: Dmitry Osipenko @ 2019-05-04 16:06 UTC (permalink / raw)
To: Ben Dooks, linux-kernel
Cc: Laxman Dewangan, Jon Hunter, Vinod Koul, Dan Williams,
Thierry Reding, dmaengine, linux-tegra, linux-kernel
In-Reply-To: <b10126c5-227b-af9d-8774-7c9cf8f43908@codethink.co.uk>
01.05.2019 11:58, Ben Dooks пишет:
> On 24/04/2019 19:17, Dmitry Osipenko wrote:
>> 24.04.2019 19:23, 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>
>>
>> Hello Ben,
>>
>> Thank you very much for keeping it up. I have couple comments, please
>> see them below.
>>
>>> Cc: Dmitry Osipenko <digetx@gmail.com>
>>> Cc: Laxman Dewangan <ldewangan@nvidia.com> (supporter:TEGRA DMA DRIVERS)
>>> Cc: Jon Hunter <jonathanh@nvidia.com> (supporter:TEGRA DMA DRIVERS)
>>> Cc: Vinod Koul <vkoul@kernel.org> (maintainer:DMA GENERIC OFFLOAD
>>> ENGINE SUBSYSTEM)
>>> Cc: Dan Williams <dan.j.williams@intel.com> (reviewer:ASYNCHRONOUS
>>> TRANSFERS/TRANSFORMS (IOAT) API)
>>> Cc: Thierry Reding <thierry.reding@gmail.com> (supporter:TEGRA
>>> ARCHITECTURE SUPPORT)
>>> Cc: dmaengine@vger.kernel.org (open list:DMA GENERIC OFFLOAD ENGINE
>>> SUBSYSTEM)
>>> Cc: linux-tegra@vger.kernel.org (open list:TEGRA ARCHITECTURE SUPPORT)
>>> Cc: linux-kernel@vger.kernel.org (open list)
>>> ---
>>> drivers/dma/tegra20-apb-dma.c | 92 ++++++++++++++++++++++++++++++++---
>>> 1 file changed, 86 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c
>>> b/drivers/dma/tegra20-apb-dma.c
>>> index cf462b1abc0b..544e7273e741 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>> @@ -808,6 +808,90 @@ 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;
>>
>> You could pre-assign ahbptr=0xffffffff and result=residual here, then
>> you could remove all the duplicated assigns below.
>
> ok, ta.
>
>>> + 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 */
>>
>> The "tmp" variable isn't used anywhere in the code, please remove it.
>
> must have been left over.
>
>>> +
>>> + /* check status, if channel isn't busy then skip */
>>> + if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
>>> + result = residual;
>>> + break;
>>> + }
>>
>> This doesn't look correct because TRM says "Busy bit gets set as soon
>> as a channel is enabled and gets cleared after transfer completes",
>> hence a cleared BUSY bit means that all transfers are completed and
>> result=residual is incorrect here. Given that there is a check for EOC
>> bit being set below, this hunk should be removed.
>
> I'll check notes, but see below.
>
>>> +
>>> + /* 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);
>>
>> Please use udelay(1) because there is no ndelay on arm32 and
>> ndelay(100) is getting rounded up to 1usec. AFAIK, arm64 doesn't have
>> reliable ndelay on Tegra either because timer rate changes with the
>> CPU frequency scaling.
>
> I'll check, but last time it was implemented. This seems a backwards step.
>
>> Secondly done=0 isn't a error case, technically this could be the case
>> when tegra_dma_update_residual() is invoked just after starting the
>> transfer. Hence I think this do-while loop and timeout checking aren't
>> needed at all since done=0 is a perfectly valid case.
>
> this is not checking for an error, it's checking for a possible
> inaccurate reading.
If you'll change reading order of the status / words registers like I
suggested, then there won't be a case for the inaccuracy.
The EOC bit should be set atomically once transfer is finished, you
can't get wrapped around words count and EOC bit not being set.
For oneshot transfer that runs with interrupt being disabled, the words
counter will stop at 0 and the unset BUSY bit will indicate that the
transfer is completed.
>>
>> Altogether seems the tegra_dma_update_residual() could be reduced to:
>>
>> 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, wcount;
>>
>> if (list_is_first(&sg_req->node, &tdc->pending_sg_req))
>> return residual;
>>
>> if (tdc->tdma->chip_data->support_separate_wcount_reg)
>> wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
>>
>> status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
>>
>> if (!tdc->tdma->chip_data->support_separate_wcount_reg)
>> wcount = status;
>>
>> if (status & TEGRA_APBDMA_STATUS_ISE_EOC)
>> return residual - sg_req->req_len;
>>
>> return residual - get_current_xferred_count(tdc, sg_req, wcount);
>> }
>
> I'm not sure if that will work all the time. It took days of testing to
> get reliable error data for the cases we're looking for here.
Could you please tell exactly what those cases are. I don't see when the
simplified variant could fail, but maybe I already forgot some extra
details about how APB DMA works.
I tested the variant I'm suggesting (with the fixed typos and added
check for the BUSY bit) and it works absolutely fine, audio stuttering
issue is fixed, everything else works too. Please consider to use it for
the next version of the patch if there are no objections.
^ permalink raw reply
* Re: [PATCH 0/6] Add support for Tegra186/Tegra194 and generic fixes
From: Vinod Koul @ 2019-05-04 10:43 UTC (permalink / raw)
To: Sameer Pujar
Cc: dan.j.williams, robh+dt, mark.rutland, thierry.reding, jonathanh,
ldewangan, dmaengine, devicetree, linux-tegra, linux-kernel
In-Reply-To: <1556801717-31507-1-git-send-email-spujar@nvidia.com>
On 02-05-19, 18:25, Sameer Pujar wrote:
> Audio DMA(ADMA) interface is a gateway in the AHUB for facilitating DMA
> transfers between memory and all of its clients. Currently the driver
> supports Tegra210 based platforms. This series adds support for Tegra186
> and Tegra194 based platforms and fixes few functional issues.
Applied all, thanks
--
~Vinod
^ permalink raw reply
* Re: [PATCH] dmaengine: at_xdmac: remove a stray bottom half unlock
From: Vinod Koul @ 2019-05-04 10:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ludovic Desroches, Barry Song, Dan Williams, dmaengine,
kernel-janitors
In-Reply-To: <20190503131507.GA1236@mwanda>
On 03-05-19, 16:15, Dan Carpenter wrote:
> We switched this code from spin_lock_bh() to vanilla spin_lock() but
> there was one stray spin_unlock_bh() that was overlooked. This
> patch converts it to spin_unlock() as well.
Applied, thanks
--
~Vinod
^ permalink raw reply
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