[PATCH V3 0/2] Add support for Tegra210 ADMA

[PATCH V3 0/2] Add support for Tegra210 ADMA

[Jon Hunter]

Add support for the Tegra210 Audio DMA (ADMA) controller. This was
originally distributed as an RFC [0] based upon the existing tegra
APB-DMA driver. Since then the driver has been significantly
re-worked to remove a lot of the unused/unnecessary functionality
that was carried over from the APB-DMA. This version is no longer
derived from the APB-DMA driver and has been updated to use the
virt-dma helpers.

V3 changes:
- Updated DT binding per feedback from Mark and Stephen
- Fixed up items mentioned by Vinod

V2 changes:
- Re-worked device-tree binding

[0] https://lkml.org/lkml/2015/8/18/237

Jon Hunter (2):
  Documentation: DT: Add binding documentation for NVIDIA ADMA
  dmaengine: tegra-adma: Add support for Tegra210 ADMA

 .../devicetree/bindings/dma/tegra210-adma.txt      |  50 ++
 drivers/dma/Kconfig                                |  13 +
 drivers/dma/Makefile                               |   1 +
 drivers/dma/tegra210-adma.c                        | 909 +++++++++++++++++++++
 4 files changed, 973 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
 create mode 100644 drivers/dma/tegra210-adma.c

-- 
2.1.4

[PATCH V3 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

[Jon Hunter]

Add device-tree binding documentation for the Tegra210 Audio DMA
controller.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 .../devicetree/bindings/dma/tegra210-adma.txt      | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt

diff --git a/Documentation/devicetree/bindings/dma/tegra210-adma.txt b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
new file mode 100644
index 000000000000..5e4eda1d1918
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
@@ -0,0 +1,50 @@
+* NVIDIA Tegra Audio DMA (ADMA) controller
+
+Required properties:
+- compatible: Must be "nvidia,tegra210-adma".
+- reg: Should contain DMA registers location and length. This should be
+  a single entry that includes all of the per-channel registers in one
+  contiguous bank.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts in
+  ascending order with respect to the DMA channel index.
+- clocks: Must contain one entry for the ADMA module clock, "adma_ape".
+- clock-names: Must contain the entry "adma_ape".
+- #dma-cells : Must be 1. The first cell denotes the receive/transmit
+  request number and should be between 1 and the maximum number of
+  requests supported. This value corresponds to the RX/TX_REQUEST_SELECT
+  fields in the ADMA_CHn_CTRL register.
+
+
+Example:
+
+adma: adma@702e2000 {
+	compatible = "nvidia,tegra210-adma";
+	reg = <0x0 0x702e2000 0x0 0x2000>;
+	interrupt-parent = <&tegra_agic>;
+	interrupts = <GIC_SPI INT_ADMA_EOT0 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT1 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT2 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT3 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT4 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT5 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT6 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT7 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT8 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT9 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT10 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT11 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT12 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT13 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT14 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT15 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT16 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT17 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT18 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT19 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT20 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT21 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&tegra_car TEGRA210_CLK_ADMA_APE>;
+	clock-names = "adma_ape";
+	#dma-cells = <1>;
+};
-- 
2.1.4

[PATCH V3 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Jon Hunter]

Add support for the Tegra210 Audio DMA controller that is used for
transferring data between system memory and the Audio sub-system.
The driver only supports cyclic transfers because this is being solely
used for audio.

This driver is based upon the work by Dara Ramesh <dramesh@nvidia.com>.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/dma/Kconfig         |  13 +
 drivers/dma/Makefile        |   1 +
 drivers/dma/tegra210-adma.c | 909 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 923 insertions(+)
 create mode 100644 drivers/dma/tegra210-adma.c

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 5c931d45fdca..1f83877f3328 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -463,6 +463,19 @@ config TEGRA20_APB_DMA
 	  This DMA controller transfers data from memory to peripheral fifo
 	  or vice versa. It does not support memory to memory data transfer.
 
+config TEGRA210_ADMA
+	bool "NVIDIA Tegra210 ADMA support"
+	depends on ARCH_TEGRA
+	select DMA_ENGINE
+	select DMA_VIRTUAL_CHANNELS
+	help
+	  Support for the NVIDIA Tegra210 ADMA controller driver. The
+	  DMA controller has multiple DMA channels and is used to service
+	  various audio clients in the Tegra210 audio processing engine
+	  (APE). This DMA controller transfers data from memory to
+	  peripheral and vice versa. It does not support memory to
+	  memory data transfer.
+
 config TIMB_DMA
 	tristate "Timberdale FPGA DMA support"
 	depends on MFD_TIMBERDALE
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index ef9c099bd2b6..0b81fb20207b 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
 obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
 obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
+obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o
 obj-$(CONFIG_TIMB_DMA) += timb_dma.o
 obj-$(CONFIG_TI_CPPI41) += cppi41.o
 obj-$(CONFIG_TI_DMA_CROSSBAR) += ti-dma-crossbar.o
diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
new file mode 100644
index 000000000000..929574650421
--- /dev/null
+++ b/drivers/dma/tegra210-adma.c
@@ -0,0 +1,909 @@
+/*
+ * ADMA driver for Nvidia's Tegra210 ADMA controller.
+ *
+ * Copyright (c) 2015, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_dma.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/clk/tegra.h>
+
+#include "dmaengine.h"
+#include "virt-dma.h"
+
+#define ADMA_CH_CMD					0x00
+#define ADMA_CH_STATUS					0x0c
+#define ADMA_CH_STATUS_XFER_EN				BIT(0)
+
+#define ADMA_CH_INT_STATUS				0x10
+#define ADMA_CH_INT_STATUS_XFER_DONE			BIT(0)
+
+#define ADMA_CH_INT_CLEAR				0x1c
+#define ADMA_CH_CTRL					0x24
+#define ADMA_CH_CTRL_TX_REQ(val)			(((val) & 0xf) << 28)
+#define ADMA_CH_CTRL_TX_REQ_MAX				10
+#define ADMA_CH_CTRL_RX_REQ(val)			(((val) & 0xf) << 24)
+#define ADMA_CH_CTRL_RX_REQ_MAX				10
+#define ADMA_CH_CTRL_DIR(val)				(((val) & 0xf) << 12)
+#define ADMA_CH_CTRL_DIR_AHUB2MEM			2
+#define ADMA_CH_CTRL_DIR_MEM2AHUB			4
+#define ADMA_CH_CTRL_MODE_CONTINUOUS			(2 << 8)
+#define ADMA_CH_CTRL_FLOWCTRL_EN			BIT(1)
+
+#define ADMA_CH_CONFIG					0x28
+#define ADMA_CH_CONFIG_SRC_BUF(val)			(((val) & 0x7) << 28)
+#define ADMA_CH_CONFIG_TRG_BUF(val)			(((val) & 0x7) << 24)
+#define ADMA_CH_CONFIG_BURST_SIZE(val)			(((val) & 0x7) << 20)
+#define ADMA_CH_CONFIG_BURST_16				5
+#define ADMA_CH_CONFIG_WEIGHT_FOR_WRR(val)		((val) & 0xf)
+#define ADMA_CH_CONFIG_MAX_BUFS				8
+
+#define ADMA_CH_FIFO_CTRL				0x2c
+#define ADMA_CH_FIFO_CTRL_OVRFW_THRES(val)		(((val) & 0xf) << 24)
+#define ADMA_CH_FIFO_CTRL_STARV_THRES(val)		(((val) & 0xf) << 16)
+#define ADMA_CH_FIFO_CTRL_TX_SIZE(val)			(((val) & 0xf) << 8)
+#define ADMA_CH_FIFO_CTRL_RX_SIZE(val)			((val) & 0xf)
+
+#define ADMA_CH_TC_STATUS				0x30
+#define ADMA_CH_LOWER_SRC_ADDR				0x34
+#define ADMA_CH_LOWER_TRG_ADDR				0x3c
+#define ADMA_CH_TC					0x44
+#define ADMA_CH_TC_COUNT_MASK				0x3ffffffc
+
+#define ADMA_CH_XFER_STATUS				0x54
+#define ADMA_CH_XFER_STATUS_COUNT_MASK			0xffff
+
+#define ADMA_GLOBAL_CMD					0xc00
+#define ADMA_GLOBAL_SOFT_RESET				0xc04
+#define ADMA_GLOBAL_INT_CLEAR				0xc20
+#define ADMA_GLOBAL_CTRL				0xc24
+
+#define ADMA_CH_REG_OFFSET(a)				(a * 0x80)
+
+#define ADMA_CH_FIFO_CTRL_DEFAULT	(ADMA_CH_FIFO_CTRL_OVRFW_THRES(1) | \
+					 ADMA_CH_FIFO_CTRL_STARV_THRES(1) | \
+					 ADMA_CH_FIFO_CTRL_TX_SIZE(3)     | \
+					 ADMA_CH_FIFO_CTRL_RX_SIZE(3))
+struct tegra_adma;
+
+/*
+ * struct tegra_adma_chip_data - Tegra chip specific data
+ * @nr_channels: Number of DMA channels available.
+ */
+struct tegra_adma_chip_data {
+	int nr_channels;
+};
+
+/*
+ * struct tegra_adma_chan_regs - Tegra ADMA channel registers
+ */
+struct tegra_adma_chan_regs {
+	unsigned int ctrl;
+	unsigned int config;
+	unsigned int src_addr;
+	unsigned int trg_addr;
+	unsigned int fifo_ctrl;
+	unsigned int tc;
+};
+
+/*
+ * struct tegra_adma_desc - Tegra ADMA descriptor to manage transfer requests.
+ */
+struct tegra_adma_desc {
+	struct virt_dma_desc		vd;
+	struct tegra_adma_chan_regs	ch_regs;
+	unsigned long			bytes_requested;
+	unsigned long			bytes_transferred;
+};
+
+/*
+ * struct tegra_adma_chan - Tegra ADMA channel information
+ */
+struct tegra_adma_chan {
+	struct virt_dma_chan		vc;
+	struct tegra_adma_desc		*desc;
+	struct tegra_adma		*tdma;
+	char				name[30];
+	int				irq;
+	void __iomem			*chan_addr;
+	spinlock_t			lock;
+
+	/* Slave channel configuration info */
+	struct dma_slave_config		sconfig;
+	bool				sconfig_valid;
+	unsigned int			sreq_dir;
+	unsigned int			sreq_index;
+	bool				sreq_reserved;
+
+	/* Transfer count and position info */
+	unsigned int			tx_buf_count;
+	unsigned int			tx_buf_pos;
+};
+
+/*
+ * struct tegra_adma - Tegra ADMA controller information
+ */
+struct tegra_adma {
+	struct dma_device			dma_dev;
+	struct device				*dev;
+	struct clk				*adma_clk;
+	void __iomem				*base_addr;
+	unsigned int				nr_channels;
+	unsigned long				rx_requests_reserved;
+	unsigned long				tx_requests_reserved;
+
+	/* Used to store global command register state when suspending */
+	unsigned int				global_cmd;
+
+	/* Last member of the structure */
+	struct tegra_adma_chan			channels[0];
+};
+
+static inline void tdma_write(struct tegra_adma *tdma, u32 reg, u32 val)
+{
+	writel(val, tdma->base_addr + reg);
+}
+
+static inline u32 tdma_read(struct tegra_adma *tdma, u32 reg)
+{
+	return readl(tdma->base_addr + reg);
+}
+
+static inline void tdma_ch_write(struct tegra_adma_chan *tdc,
+		u32 reg, u32 val)
+{
+	writel(val, tdc->chan_addr + reg);
+}
+
+static inline u32 tdma_ch_read(struct tegra_adma_chan *tdc, u32 reg)
+{
+	return readl(tdc->chan_addr + reg);
+}
+
+static inline struct tegra_adma_chan *to_tegra_adma_chan(struct dma_chan *dc)
+{
+	return container_of(dc, struct tegra_adma_chan, vc.chan);
+}
+
+static inline struct tegra_adma_desc *to_tegra_adma_desc(
+		struct dma_async_tx_descriptor *td)
+{
+	return container_of(td, struct tegra_adma_desc, vd.tx);
+}
+
+static inline struct device *tdc2dev(struct tegra_adma_chan *tdc)
+{
+	return tdc->tdma->dev;
+}
+
+static void tegra_adma_desc_free(struct virt_dma_desc *vd)
+{
+	kfree(container_of(vd, struct tegra_adma_desc, vd));
+}
+
+static int tegra_adma_slave_config(struct dma_chan *dc,
+				   struct dma_slave_config *sconfig)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+
+	memcpy(&tdc->sconfig, sconfig, sizeof(*sconfig));
+	tdc->sconfig_valid = true;
+
+	return 0;
+}
+
+static int tegra_adma_init(struct tegra_adma *tdma)
+{
+	u32 status;
+	int ret;
+
+	/* Clear any interrupts */
+	tdma_write(tdma, ADMA_GLOBAL_INT_CLEAR, 0x1);
+
+	/* Assert soft reset */
+	tdma_write(tdma, ADMA_GLOBAL_SOFT_RESET, 0x1);
+
+	/* Wait for reset to clear */
+	ret = readx_poll_timeout(readl,
+				 tdma->base_addr + ADMA_GLOBAL_SOFT_RESET,
+				 status, status == 0, 20, 10000);
+	if (ret)
+		return ret;
+
+	/* Enable global ADMA registers */
+	tdma_write(tdma, ADMA_GLOBAL_CMD, 1);
+
+	return 0;
+}
+
+static int tegra_adma_request_alloc(struct tegra_adma_chan *tdc,
+				    unsigned int sreq_dir)
+{
+	struct tegra_adma *tdma = tdc->tdma;
+	unsigned int sreq_index = tdc->sreq_index;
+
+	if (tdc->sreq_reserved)
+		return tdc->sreq_dir == sreq_dir ? 0 : -EINVAL;
+
+	switch (sreq_dir) {
+	case ADMA_CH_CTRL_DIR_MEM2AHUB:
+		if (sreq_index > ADMA_CH_CTRL_TX_REQ_MAX) {
+			dev_err(tdma->dev, "invalid DMA request\n");
+			return -EINVAL;
+		}
+
+		if (test_and_set_bit(sreq_index, &tdma->tx_requests_reserved)) {
+			dev_err(tdma->dev, "DMA request reserved\n");
+			return -EINVAL;
+		}
+		break;
+
+	case ADMA_CH_CTRL_DIR_AHUB2MEM:
+		if (sreq_index > ADMA_CH_CTRL_RX_REQ_MAX) {
+			dev_err(tdma->dev, "invalid DMA request\n");
+			return -EINVAL;
+		}
+
+		if (test_and_set_bit(sreq_index, &tdma->rx_requests_reserved)) {
+			dev_err(tdma->dev, "DMA request reserved\n");
+			return -EINVAL;
+		}
+		break;
+
+	default:
+		dev_WARN(tdma->dev, "channel %s has invalid transfer type\n",
+			 tdc->name);
+		return -EINVAL;
+	}
+
+	tdc->sreq_dir = sreq_dir;
+	tdc->sreq_reserved = true;
+
+	return 0;
+}
+
+static void tegra_adma_request_free(struct tegra_adma_chan *tdc)
+{
+	struct tegra_adma *tdma = tdc->tdma;
+
+	if (!tdc->sreq_reserved)
+		return;
+
+	switch (tdc->sreq_dir) {
+	case ADMA_CH_CTRL_DIR_MEM2AHUB:
+		clear_bit(tdc->sreq_index, &tdma->tx_requests_reserved);
+		break;
+	case ADMA_CH_CTRL_DIR_AHUB2MEM:
+		clear_bit(tdc->sreq_index, &tdma->rx_requests_reserved);
+		break;
+	default:
+		dev_WARN(tdma->dev, "channel %s has invalid transfer type\n",
+			 tdc->name);
+		return;
+	}
+
+	tdc->sreq_reserved = false;
+}
+
+static u32 tegra_adma_irq_status(struct tegra_adma_chan *tdc)
+{
+	u32 status = tdma_ch_read(tdc, ADMA_CH_INT_STATUS);
+
+	return status & ADMA_CH_INT_STATUS_XFER_DONE;
+}
+
+static u32 tegra_adma_irq_clear(struct tegra_adma_chan *tdc)
+{
+	u32 status = tegra_adma_irq_status(tdc);
+
+	if (status)
+		tdma_ch_write(tdc, ADMA_CH_INT_CLEAR, status);
+
+	return status;
+}
+
+static void tegra_adma_stop(struct tegra_adma_chan *tdc)
+{
+	unsigned int status;
+
+	/* Disable ADMA */
+	tdma_ch_write(tdc, ADMA_CH_CMD, 0);
+
+	/* Clear interrupt status */
+	tegra_adma_irq_clear(tdc);
+
+	if (readx_poll_timeout_atomic(readl, tdc->chan_addr + ADMA_CH_STATUS,
+			status, !(status & ADMA_CH_STATUS_XFER_EN),
+			20, 10000)) {
+		dev_err(tdc2dev(tdc), "unable to stop DMA channel\n");
+		return;
+	}
+
+	tdc->desc = NULL;
+}
+
+static void tegra_adma_start(struct tegra_adma_chan *tdc)
+{
+	struct virt_dma_desc *vd = vchan_next_desc(&tdc->vc);
+	struct tegra_adma_chan_regs *ch_regs;
+	struct tegra_adma_desc *desc;
+
+	if (!vd)
+		return;
+
+	list_del(&vd->node);
+
+	desc = to_tegra_adma_desc(&vd->tx);
+
+	if (!desc) {
+		dev_warn(tdc2dev(tdc), "unable to start DMA, no descriptor\n");
+		return;
+	}
+
+	ch_regs = &desc->ch_regs;
+
+	tdc->tx_buf_pos = 0;
+	tdc->tx_buf_count = 0;
+	tdma_ch_write(tdc, ADMA_CH_TC, ch_regs->tc);
+	tdma_ch_write(tdc, ADMA_CH_CTRL, ch_regs->ctrl);
+	tdma_ch_write(tdc, ADMA_CH_LOWER_SRC_ADDR, ch_regs->src_addr);
+	tdma_ch_write(tdc, ADMA_CH_LOWER_TRG_ADDR, ch_regs->trg_addr);
+	tdma_ch_write(tdc, ADMA_CH_FIFO_CTRL, ch_regs->fifo_ctrl);
+	tdma_ch_write(tdc, ADMA_CH_CONFIG, ch_regs->config);
+
+	/* Start ADMA */
+	tdma_ch_write(tdc, ADMA_CH_CMD, 1);
+
+	tdc->desc = desc;
+}
+
+static void tegra_adma_update_position(struct tegra_adma_chan *tdc)
+{
+	struct tegra_adma_desc *desc = tdc->desc;
+	unsigned int max = ADMA_CH_XFER_STATUS_COUNT_MASK + 1;
+	unsigned int pos = tdma_ch_read(tdc, ADMA_CH_XFER_STATUS);
+
+	/*
+	 * Handle wrap around of buffer count register
+	 */
+	if (pos < tdc->tx_buf_pos)
+		tdc->tx_buf_count += pos + (max - tdc->tx_buf_pos);
+	else
+		tdc->tx_buf_count += pos - tdc->tx_buf_pos;
+
+	tdc->tx_buf_pos = pos;
+
+	desc->bytes_transferred = tdc->tx_buf_count * desc->ch_regs.tc;
+
+	/*
+	 * If we are not currently active, then it is safe to read the
+	 * remaining words from the TC_STATUS register and add the partial
+	 * buffer to the total transferred.
+	 */
+	if (!tdc->desc)
+		desc->bytes_transferred += desc->ch_regs.tc -
+					   tdma_ch_read(tdc, ADMA_CH_TC_STATUS);
+}
+
+static unsigned int tegra_adma_get_residue(struct tegra_adma_desc *desc)
+{
+	return desc->bytes_requested - (desc->bytes_transferred %
+					desc->bytes_requested);
+}
+
+static irqreturn_t tegra_adma_isr(int irq, void *dev_id)
+{
+	struct tegra_adma_chan *tdc = dev_id;
+	unsigned long status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tdc->lock, flags);
+
+	status = tegra_adma_irq_clear(tdc);
+	if (status == 0 || !tdc->desc) {
+		spin_unlock_irqrestore(&tdc->lock, flags);
+		return IRQ_NONE;
+	}
+
+	vchan_cyclic_callback(&tdc->desc->vd);
+
+	spin_unlock_irqrestore(&tdc->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
+static void tegra_adma_issue_pending(struct dma_chan *dc)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&tdc->lock, flags);
+
+	if (vchan_issue_pending(&tdc->vc)) {
+		if (tdc->desc)
+			dev_warn(tdc2dev(tdc), "DMA already running\n");
+		else
+			tegra_adma_start(tdc);
+	}
+
+	spin_unlock_irqrestore(&tdc->lock, flags);
+}
+
+static int tegra_adma_terminate_all(struct dma_chan *dc)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	unsigned long flags;
+	LIST_HEAD(head);
+
+	spin_lock_irqsave(&tdc->lock, flags);
+
+	if (tdc->desc)
+		tegra_adma_stop(tdc);
+
+	tegra_adma_request_free(tdc);
+	vchan_get_all_descriptors(&tdc->vc, &head);
+	spin_unlock_irqrestore(&tdc->lock, flags);
+	vchan_dma_desc_free_list(&tdc->vc, &head);
+
+	return 0;
+}
+
+static enum dma_status tegra_adma_tx_status(struct dma_chan *dc,
+					    dma_cookie_t cookie,
+					    struct dma_tx_state *txstate)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	struct tegra_adma_desc *desc;
+	struct virt_dma_desc *vd;
+	enum dma_status ret;
+	unsigned long flags;
+	unsigned int residual;
+
+	spin_lock_irqsave(&tdc->lock, flags);
+
+	ret = dma_cookie_status(dc, cookie, txstate);
+	if (ret == DMA_COMPLETE || !txstate) {
+		spin_unlock_irqrestore(&tdc->lock, flags);
+		return ret;
+	}
+
+	vd = vchan_find_desc(&tdc->vc, cookie);
+	if (vd) {
+		desc = to_tegra_adma_desc(&vd->tx);
+		residual = desc->ch_regs.tc;
+	} else if (tdc->desc && tdc->desc->vd.tx.cookie == cookie) {
+		tegra_adma_update_position(tdc);
+		residual = tegra_adma_get_residue(tdc->desc);
+	} else {
+		residual = 0;
+	}
+
+	dma_set_residue(txstate, residual);
+
+	spin_unlock_irqrestore(&tdc->lock, flags);
+
+	return ret;
+}
+
+static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc,
+				      struct tegra_adma_desc *desc,
+				      dma_addr_t buf_addr, size_t buf_len,
+				      size_t period_len,
+				      enum dma_transfer_direction direction)
+{
+	struct tegra_adma_chan_regs *ch_regs = &desc->ch_regs;
+	unsigned int burst_size, num_bufs, sreq_dir;
+
+	num_bufs = buf_len / period_len;
+
+	if (num_bufs > ADMA_CH_CONFIG_MAX_BUFS)
+		return -EINVAL;
+
+	switch (direction) {
+	case DMA_MEM_TO_DEV:
+		sreq_dir = ADMA_CH_CTRL_DIR_MEM2AHUB;
+		burst_size = fls(tdc->sconfig.dst_maxburst);
+		ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
+		ch_regs->ctrl = ADMA_CH_CTRL_TX_REQ(tdc->sreq_index);
+		ch_regs->src_addr = buf_addr;
+		break;
+
+	case DMA_DEV_TO_MEM:
+		sreq_dir = ADMA_CH_CTRL_DIR_AHUB2MEM;
+		burst_size = fls(tdc->sconfig.src_maxburst);
+		ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
+		ch_regs->ctrl = ADMA_CH_CTRL_RX_REQ(tdc->sreq_index);
+		ch_regs->trg_addr = buf_addr;
+		break;
+
+	default:
+		dev_err(tdc2dev(tdc), "DMA direction is not supported\n");
+		return -EINVAL;
+	}
+
+	if (!burst_size || burst_size > ADMA_CH_CONFIG_BURST_16)
+		burst_size = ADMA_CH_CONFIG_BURST_16;
+
+	ch_regs->ctrl |= ADMA_CH_CTRL_DIR(sreq_dir) |
+			 ADMA_CH_CTRL_MODE_CONTINUOUS |
+			 ADMA_CH_CTRL_FLOWCTRL_EN;
+	ch_regs->config |= ADMA_CH_CONFIG_BURST_SIZE(burst_size);
+	ch_regs->config |= ADMA_CH_CONFIG_WEIGHT_FOR_WRR(1);
+	ch_regs->fifo_ctrl = ADMA_CH_FIFO_CTRL_DEFAULT;
+	ch_regs->tc = period_len & ADMA_CH_TC_COUNT_MASK;
+
+	return tegra_adma_request_alloc(tdc, sreq_dir);
+}
+
+static struct dma_async_tx_descriptor *tegra_adma_prep_slave_sg(
+	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
+	enum dma_transfer_direction direction, unsigned long flags,
+	void *context)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+
+	dev_warn(tdc2dev(tdc), "scatter-gather transfers are not supported\n");
+
+	return NULL;
+}
+
+static struct dma_async_tx_descriptor *tegra_adma_prep_dma_cyclic(
+	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
+	size_t period_len, enum dma_transfer_direction direction,
+	unsigned long flags)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	struct tegra_adma_desc *desc = NULL;
+
+	if (!tdc->sconfig_valid) {
+		dev_err(tdc2dev(tdc), "ADMA slave configuration not set\n");
+		return NULL;
+	}
+
+	if (!buf_len || !period_len || period_len > ADMA_CH_TC_COUNT_MASK) {
+		dev_err(tdc2dev(tdc), "invalid buffer/period len\n");
+		return NULL;
+	}
+
+	if (buf_len % period_len) {
+		dev_err(tdc2dev(tdc), "buf_len not a multiple of period_len\n");
+		return NULL;
+	}
+
+	if (!IS_ALIGNED(buf_addr, 4)) {
+		dev_err(tdc2dev(tdc), "invalid buffer alignment\n");
+		return NULL;
+	}
+
+	desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	desc->bytes_transferred = 0;
+	desc->bytes_requested = buf_len;
+
+	if (tegra_adma_set_xfer_params(tdc, desc, buf_addr, buf_len, period_len,
+				       direction)) {
+		kfree(desc);
+		return NULL;
+	}
+
+	return vchan_tx_prep(&tdc->vc, &desc->vd, flags);
+}
+
+static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	int ret;
+
+	ret = pm_runtime_get_sync(tdc2dev(tdc));
+	if (ret)
+		return ret;
+
+	dma_cookie_init(&tdc->vc.chan);
+	tdc->sconfig_valid = false;
+
+	return 0;
+}
+
+static void tegra_adma_free_chan_resources(struct dma_chan *dc)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+
+	if (tdc->desc)
+		tegra_adma_terminate_all(dc);
+
+	tdc->sconfig_valid = false;
+	vchan_free_chan_resources(&tdc->vc);
+
+	pm_runtime_put(tdc2dev(tdc));
+
+	tegra_adma_request_free(tdc);
+
+	tdc->sreq_index = 0;
+	tdc->sreq_dir = 0;
+}
+
+static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
+					   struct of_dma *ofdma)
+{
+	struct tegra_adma *tdma = ofdma->of_dma_data;
+	struct tegra_adma_chan *tdc;
+	struct dma_chan *chan;
+	unsigned int sreq_index;
+
+	if (dma_spec->args_count != 1)
+		return NULL;
+
+	sreq_index = dma_spec->args[0];
+
+	if (sreq_index == 0) {
+		dev_err(tdma->dev, "DMA request must not be 0\n");
+		return NULL;
+	}
+
+	chan = dma_get_any_slave_channel(&tdma->dma_dev);
+	if (!chan)
+		return NULL;
+
+	tdc = to_tegra_adma_chan(chan);
+	tdc->sreq_index = sreq_index;
+
+	return chan;
+}
+
+static int tegra_adma_runtime_suspend(struct device *dev)
+{
+	struct tegra_adma *tdma = dev_get_drvdata(dev);
+
+	tdma->global_cmd = tdma_read(tdma, ADMA_GLOBAL_CMD);
+
+	clk_disable_unprepare(tdma->adma_clk);
+
+	return 0;
+}
+
+static int tegra_adma_runtime_resume(struct device *dev)
+{
+	struct tegra_adma *tdma = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(tdma->adma_clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable ADMA clock: %d\n", ret);
+		return ret;
+	}
+
+	tdma_write(tdma, ADMA_GLOBAL_CMD, tdma->global_cmd);
+
+	return 0;
+}
+
+static const struct tegra_adma_chip_data tegra210_chip_data = {
+	.nr_channels = 22,
+};
+
+static const struct of_device_id tegra_adma_of_match[] = {
+	{ .compatible = "nvidia,tegra210-adma", .data = &tegra210_chip_data },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_adma_of_match);
+
+static int tegra_adma_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	const struct tegra_adma_chip_data *cdata;
+	struct tegra_adma *tdma;
+	struct resource	*res;
+	int ret, i;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "no device tree node for ADMA\n");
+		return -ENODEV;
+	}
+
+	match = of_match_device(tegra_adma_of_match, &pdev->dev);
+	if (!match) {
+		dev_err(&pdev->dev, "no device match found\n");
+		return -ENODEV;
+	}
+	cdata = match->data;
+
+	tdma = devm_kzalloc(&pdev->dev, sizeof(*tdma) + cdata->nr_channels *
+			    sizeof(struct tegra_adma_chan), GFP_KERNEL);
+	if (!tdma)
+		return -ENOMEM;
+
+	tdma->dev = &pdev->dev;
+	tdma->nr_channels = cdata->nr_channels;
+	platform_set_drvdata(pdev, tdma);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tdma->base_addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tdma->base_addr))
+		return PTR_ERR(tdma->base_addr);
+
+	tdma->adma_clk = devm_clk_get(&pdev->dev, "adma_ape");
+	if (IS_ERR(tdma->adma_clk)) {
+		dev_err(&pdev->dev, "ADMA clock not found\n");
+		return PTR_ERR(tdma->adma_clk);
+	}
+
+	pm_runtime_enable(&pdev->dev);
+	if (pm_runtime_enabled(&pdev->dev))
+		ret = pm_runtime_get_sync(&pdev->dev);
+	else
+		ret = tegra_adma_runtime_resume(&pdev->dev);
+
+	if (ret) {
+		pm_runtime_disable(&pdev->dev);
+		return ret;
+	}
+
+	ret = tegra_adma_init(tdma);
+	if (ret)
+		goto err_pm_disable;
+
+	INIT_LIST_HEAD(&tdma->dma_dev.channels);
+	for (i = 0; i < tdma->nr_channels; i++) {
+		struct tegra_adma_chan *tdc = &tdma->channels[i];
+
+		tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i);
+
+		snprintf(tdc->name, sizeof(tdc->name), "adma.%d", i);
+
+		tdc->irq = platform_get_irq(pdev, i);
+		if (tdc->irq < 0) {
+			ret = -EPROBE_DEFER;
+			goto err_irq;
+		}
+
+		ret = devm_request_irq(&pdev->dev, tdc->irq, tegra_adma_isr, 0,
+				       tdc->name, tdc);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to get interrupt for channel %d\n", i);
+			goto err_irq;
+		}
+
+		spin_lock_init(&tdc->lock);
+		vchan_init(&tdc->vc, &tdma->dma_dev);
+		tdc->vc.desc_free = tegra_adma_desc_free;
+		tdc->tdma = tdma;
+	}
+
+	dma_cap_set(DMA_SLAVE, tdma->dma_dev.cap_mask);
+	dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
+	dma_cap_set(DMA_CYCLIC, tdma->dma_dev.cap_mask);
+
+	tdma->dma_dev.dev = &pdev->dev;
+	tdma->dma_dev.device_alloc_chan_resources =
+					tegra_adma_alloc_chan_resources;
+	tdma->dma_dev.device_free_chan_resources =
+					tegra_adma_free_chan_resources;
+	tdma->dma_dev.device_issue_pending = tegra_adma_issue_pending;
+	tdma->dma_dev.device_prep_slave_sg = tegra_adma_prep_slave_sg;
+	tdma->dma_dev.device_prep_dma_cyclic = tegra_adma_prep_dma_cyclic;
+	tdma->dma_dev.device_config = tegra_adma_slave_config;
+	tdma->dma_dev.device_tx_status = tegra_adma_tx_status;
+	tdma->dma_dev.device_terminate_all = tegra_adma_terminate_all;
+	tdma->dma_dev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	tdma->dma_dev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+
+	ret = dma_async_device_register(&tdma->dma_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "ADMA registration failed: %d\n", ret);
+		goto err_irq;
+	}
+
+	ret = of_dma_controller_register(pdev->dev.of_node,
+					 tegra_dma_of_xlate, tdma);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "ADMA OF registration failed %d\n", ret);
+		goto err_unregister_dma_dev;
+	}
+
+	pm_runtime_put(&pdev->dev);
+
+	dev_info(&pdev->dev, "Tegra210 ADMA driver registered %d channels\n",
+		 tdma->nr_channels);
+
+	return 0;
+
+err_unregister_dma_dev:
+	dma_async_device_unregister(&tdma->dma_dev);
+err_irq:
+	while (--i >= 0) {
+		struct tegra_adma_chan *tdc = &tdma->channels[i];
+
+		tasklet_kill(&tdc->vc.task);
+	}
+err_pm_disable:
+	pm_runtime_disable(&pdev->dev);
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		tegra_adma_runtime_suspend(&pdev->dev);
+
+	return ret;
+}
+
+static int tegra_adma_remove(struct platform_device *pdev)
+{
+	struct tegra_adma *tdma = platform_get_drvdata(pdev);
+	struct tegra_adma_chan *tdc;
+	int i;
+
+	dma_async_device_unregister(&tdma->dma_dev);
+
+	for (i = 0; i < tdma->nr_channels; ++i) {
+		tdc = &tdma->channels[i];
+		disable_irq(tdc->irq);
+		tasklet_kill(&tdc->vc.task);
+	}
+
+	pm_runtime_disable(&pdev->dev);
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		tegra_adma_runtime_suspend(&pdev->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int tegra_adma_pm_suspend(struct device *dev)
+{
+	return pm_runtime_suspended(dev);
+}
+#endif
+
+static const struct dev_pm_ops tegra_adma_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(tegra_adma_runtime_suspend,
+			   tegra_adma_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(tegra_adma_pm_suspend, NULL)
+};
+
+static struct platform_driver tegra_admac_driver = {
+	.driver = {
+		.name	= "tegra-adma",
+		.pm	= &tegra_adma_dev_pm_ops,
+		.of_match_table = tegra_adma_of_match,
+	},
+	.probe		= tegra_adma_probe,
+	.remove		= tegra_adma_remove,
+};
+
+module_platform_driver(tegra_admac_driver);
+
+MODULE_ALIAS("platform:tegra210-adma");
+MODULE_DESCRIPTION("NVIDIA Tegra ADMA driver");
+MODULE_AUTHOR("Dara Ramesh <dramesh@nvidia.com>");
+MODULE_AUTHOR("Jon Hunter <jonathanh@nvidia.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH V3 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

[Stephen Warren]

On 10/16/2015 01:35 AM, Jon Hunter wrote:
> Add device-tree binding documentation for the Tegra210 Audio DMA
> controller.

> diff --git a/Documentation/devicetree/bindings/dma/tegra210-adma.txt b/Documentation/devicetree/bindings/dma/tegra210-adma.txt

> +Required properties:

> +- interrupt-parent: Phandle to the interrupt parent controller.

Nit: Since that is more of a "system level"/standard property, it's 
typical not to document it. The property is not actually required if the 
inherited value is already correct. Still, it's obvious enough what this 
means, so I'd only suggest fixing this if you have to respin for some 
other reason.

> +- clocks: Must contain one entry for the ADMA module clock, "adma_ape".
> +- clock-names: Must contain the entry "adma_ape".

Which clock is this in the CAR? I don't see any adma_ape clock 
documented in the TRM.

Is there a dedicated reset signal for this module? If so, we should 
require a resets property.

Re: [PATCH V3 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

[Jon Hunter]

On 16/10/15 17:09, Stephen Warren wrote:
> On 10/16/2015 01:35 AM, Jon Hunter wrote:
>> Add device-tree binding documentation for the Tegra210 Audio DMA
>> controller.
> 
>> diff --git a/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>> b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
> 
>> +Required properties:
> 
>> +- interrupt-parent: Phandle to the interrupt parent controller.
> 
> Nit: Since that is more of a "system level"/standard property, it's
> typical not to document it. The property is not actually required if the
> inherited value is already correct. Still, it's obvious enough what this
> means, so I'd only suggest fixing this if you have to respin for some
> other reason.

Ok.

>> +- clocks: Must contain one entry for the ADMA module clock, "adma_ape".
>> +- clock-names: Must contain the entry "adma_ape".
> 
> Which clock is this in the CAR? I don't see any adma_ape clock
> documented in the TRM.

Darn. I thought I had checked this. Yes it should be the AHUB clock and
looking at the current t210 clock patches for mainline this is
TEGRA210_CLK_D_AUDIO. Ok will fix this.

> Is there a dedicated reset signal for this module? If so, we should
> require a resets property.

No there does not appear to be. Looking at the documentation the ADMA
would be reset by the main APE reset. Seems to be one reset that resets
most modules in the APE.

That raises another issue, the ADMA is in the audio power partition and
currently our downstream driver assumes that this is on. I should at
least check this.

Jon

Re: [PATCH V3 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

[Stephen Warren]

On 10/19/2015 05:22 AM, Jon Hunter wrote:
>
> On 16/10/15 17:09, Stephen Warren wrote:
>> On 10/16/2015 01:35 AM, Jon Hunter wrote:
>>> Add device-tree binding documentation for the Tegra210 Audio DMA
>>> controller.
>>
>>> diff --git a/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>>> b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>>
>>> +Required properties:
>>
>>> +- interrupt-parent: Phandle to the interrupt parent controller.
>>
>> Nit: Since that is more of a "system level"/standard property, it's
>> typical not to document it. The property is not actually required if the
>> inherited value is already correct. Still, it's obvious enough what this
>> means, so I'd only suggest fixing this if you have to respin for some
>> other reason.
>
> Ok.
>
>>> +- clocks: Must contain one entry for the ADMA module clock, "adma_ape".
>>> +- clock-names: Must contain the entry "adma_ape".
>>
>> Which clock is this in the CAR? I don't see any adma_ape clock
>> documented in the TRM.
>
> Darn. I thought I had checked this. Yes it should be the AHUB clock and
> looking at the current t210 clock patches for mainline this is
> TEGRA210_CLK_D_AUDIO. Ok will fix this.
>
>> Is there a dedicated reset signal for this module? If so, we should
>> require a resets property.
>
> No there does not appear to be. Looking at the documentation the ADMA
> would be reset by the main APE reset. Seems to be one reset that resets
> most modules in the APE.
>
> That raises another issue, the ADMA is in the audio power partition and
> currently our downstream driver assumes that this is on. I should at
> least check this.

IIRC the situation downstream w.r.t. the audio power partition is 
something like:

The AGIC is in that power partition. The Linux kernel AGIC driver gets 
probed before the power domain driver for that domain. (I think) 
deferred probe doesn't work for the AGIC driver for some reason (perhaps 
this is just a bug or oversight in our downstream AGIC driver?). So, the 
AGIC driver can't turn on the audio power domain. So, there's a hack in 
the bootloader to turn the power domain on. So, everything in the kernel 
assumes that power domain is on at boot and so drivers don't have to 
explicitly control/request that power domain.

We should fix this properly so that upstream doesn't make the assumption 
that the audio power domain is magically on before the kernel boots.

Re: [PATCH V3 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

[Jon Hunter]

On 19/10/15 17:33, Stephen Warren wrote:
> On 10/19/2015 05:22 AM, Jon Hunter wrote:
>>
>> On 16/10/15 17:09, Stephen Warren wrote:
>>> On 10/16/2015 01:35 AM, Jon Hunter wrote:
>>>> Add device-tree binding documentation for the Tegra210 Audio DMA
>>>> controller.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>>>> b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>>>
>>>> +Required properties:
>>>
>>>> +- interrupt-parent: Phandle to the interrupt parent controller.
>>>
>>> Nit: Since that is more of a "system level"/standard property, it's
>>> typical not to document it. The property is not actually required if the
>>> inherited value is already correct. Still, it's obvious enough what this
>>> means, so I'd only suggest fixing this if you have to respin for some
>>> other reason.
>>
>> Ok.
>>
>>>> +- clocks: Must contain one entry for the ADMA module clock,
>>>> "adma_ape".
>>>> +- clock-names: Must contain the entry "adma_ape".
>>>
>>> Which clock is this in the CAR? I don't see any adma_ape clock
>>> documented in the TRM.
>>
>> Darn. I thought I had checked this. Yes it should be the AHUB clock and
>> looking at the current t210 clock patches for mainline this is
>> TEGRA210_CLK_D_AUDIO. Ok will fix this.
>>
>>> Is there a dedicated reset signal for this module? If so, we should
>>> require a resets property.
>>
>> No there does not appear to be. Looking at the documentation the ADMA
>> would be reset by the main APE reset. Seems to be one reset that resets
>> most modules in the APE.
>>
>> That raises another issue, the ADMA is in the audio power partition and
>> currently our downstream driver assumes that this is on. I should at
>> least check this.
> 
> IIRC the situation downstream w.r.t. the audio power partition is
> something like:
> 
> The AGIC is in that power partition. The Linux kernel AGIC driver gets
> probed before the power domain driver for that domain. (I think)
> deferred probe doesn't work for the AGIC driver for some reason (perhaps
> this is just a bug or oversight in our downstream AGIC driver?). So, the
> AGIC driver can't turn on the audio power domain. So, there's a hack in
> the bootloader to turn the power domain on. So, everything in the kernel
> assumes that power domain is on at boot and so drivers don't have to
> explicitly control/request that power domain.
> 
> We should fix this properly so that upstream doesn't make the assumption
> that the audio power domain is magically on before the kernel boots.

Yes, I agree. Ok, I will look at that some more.

Jon

Re: [PATCH V3 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Vinod Koul]

On Fri, Oct 16, 2015 at 08:35:19AM +0100, Jon Hunter wrote:
> +static inline void tdma_ch_write(struct tegra_adma_chan *tdc,
> +		u32 reg, u32 val)

Coding style issue here

> +static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
> +{
> +	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(tdc2dev(tdc));
> +	if (ret)
> +		return ret;

pm_runtime_get_sync() return postive values on success too, so this can
fail. This check should be for less than zero case

-- 
~Vinod

Re: [PATCH V3 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Jon Hunter]

On 27/10/15 02:10, Vinod Koul wrote:
> On Fri, Oct 16, 2015 at 08:35:19AM +0100, Jon Hunter wrote:
>> +static inline void tdma_ch_write(struct tegra_adma_chan *tdc,
>> +		u32 reg, u32 val)
> 
> Coding style issue here

Yes will correct.

>> +static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
>> +{
>> +	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +	int ret;
>> +
>> +	ret = pm_runtime_get_sync(tdc2dev(tdc));
>> +	if (ret)
>> +		return ret;
> 
> pm_runtime_get_sync() return postive values on success too, so this can
> fail. This check should be for less than zero case

I stumbled upon that recently and will also fix up.

Cheers
Jon

Re: [PATCH V3 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Jon Hunter]

On 16/10/15 08:35, Jon Hunter wrote:
> Add support for the Tegra210 Audio DMA controller that is used for
> transferring data between system memory and the Audio sub-system.
> The driver only supports cyclic transfers because this is being solely
> used for audio.
> 
> This driver is based upon the work by Dara Ramesh <dramesh@nvidia.com>.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/dma/Kconfig         |  13 +
>  drivers/dma/Makefile        |   1 +
>  drivers/dma/tegra210-adma.c | 909 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 923 insertions(+)
>  create mode 100644 drivers/dma/tegra210-adma.c

[snip]

> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_adma_pm_suspend(struct device *dev)
> +{
> +	return pm_runtime_suspended(dev);
> +}
> +#endif

The above is also wrong. I will fix this up as well.

Jon

Re: [PATCH V3 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Andy Shevchenko]

On Fri, Oct 16, 2015 at 10:35 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> Add support for the Tegra210 Audio DMA controller that is used for
> transferring data between system memory and the Audio sub-system.
> The driver only supports cyclic transfers because this is being solely
> used for audio.
>
> This driver is based upon the work by Dara Ramesh <dramesh@nvidia.com>.
>


> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/clk/tegra.h>

Do we really need all of them present here?

> +
> +#include "dmaengine.h"
> +#include "virt-dma.h"
> +
> +#define ADMA_CH_CMD                                    0x00
> +#define ADMA_CH_STATUS                                 0x0c
> +#define ADMA_CH_STATUS_XFER_EN                         BIT(0)
> +
> +#define ADMA_CH_INT_STATUS                             0x10
> +#define ADMA_CH_INT_STATUS_XFER_DONE                   BIT(0)
> +
> +#define ADMA_CH_INT_CLEAR                              0x1c
> +#define ADMA_CH_CTRL                                   0x24
> +#define ADMA_CH_CTRL_TX_REQ(val)                       (((val) & 0xf) << 28)
> +#define ADMA_CH_CTRL_TX_REQ_MAX                                10
> +#define ADMA_CH_CTRL_RX_REQ(val)                       (((val) & 0xf) << 24)
> +#define ADMA_CH_CTRL_RX_REQ_MAX                                10
> +#define ADMA_CH_CTRL_DIR(val)                          (((val) & 0xf) << 12)
> +#define ADMA_CH_CTRL_DIR_AHUB2MEM                      2
> +#define ADMA_CH_CTRL_DIR_MEM2AHUB                      4
> +#define ADMA_CH_CTRL_MODE_CONTINUOUS                   (2 << 8)
> +#define ADMA_CH_CTRL_FLOWCTRL_EN                       BIT(1)
> +
> +#define ADMA_CH_CONFIG                                 0x28
> +#define ADMA_CH_CONFIG_SRC_BUF(val)                    (((val) & 0x7) << 28)
> +#define ADMA_CH_CONFIG_TRG_BUF(val)                    (((val) & 0x7) << 24)
> +#define ADMA_CH_CONFIG_BURST_SIZE(val)                 (((val) & 0x7) << 20)
> +#define ADMA_CH_CONFIG_BURST_16                                5
> +#define ADMA_CH_CONFIG_WEIGHT_FOR_WRR(val)             ((val) & 0xf)
> +#define ADMA_CH_CONFIG_MAX_BUFS                                8
> +
> +#define ADMA_CH_FIFO_CTRL                              0x2c
> +#define ADMA_CH_FIFO_CTRL_OVRFW_THRES(val)             (((val) & 0xf) << 24)
> +#define ADMA_CH_FIFO_CTRL_STARV_THRES(val)             (((val) & 0xf) << 16)
> +#define ADMA_CH_FIFO_CTRL_TX_SIZE(val)                 (((val) & 0xf) << 8)
> +#define ADMA_CH_FIFO_CTRL_RX_SIZE(val)                 ((val) & 0xf)
> +
> +#define ADMA_CH_TC_STATUS                              0x30
> +#define ADMA_CH_LOWER_SRC_ADDR                         0x34
> +#define ADMA_CH_LOWER_TRG_ADDR                         0x3c
> +#define ADMA_CH_TC                                     0x44
> +#define ADMA_CH_TC_COUNT_MASK                          0x3ffffffc
> +
> +#define ADMA_CH_XFER_STATUS                            0x54
> +#define ADMA_CH_XFER_STATUS_COUNT_MASK                 0xffff
> +
> +#define ADMA_GLOBAL_CMD                                        0xc00
> +#define ADMA_GLOBAL_SOFT_RESET                         0xc04
> +#define ADMA_GLOBAL_INT_CLEAR                          0xc20
> +#define ADMA_GLOBAL_CTRL                               0xc24
> +
> +#define ADMA_CH_REG_OFFSET(a)                          (a * 0x80)
> +
> +#define ADMA_CH_FIFO_CTRL_DEFAULT      (ADMA_CH_FIFO_CTRL_OVRFW_THRES(1) | \
> +                                        ADMA_CH_FIFO_CTRL_STARV_THRES(1) | \
> +                                        ADMA_CH_FIFO_CTRL_TX_SIZE(3)     | \
> +                                        ADMA_CH_FIFO_CTRL_RX_SIZE(3))
> +struct tegra_adma;
> +
> +/*
> + * struct tegra_adma_chip_data - Tegra chip specific data
> + * @nr_channels: Number of DMA channels available.
> + */
> +struct tegra_adma_chip_data {
> +       int nr_channels;
> +};
> +
> +/*
> + * struct tegra_adma_chan_regs - Tegra ADMA channel registers
> + */
> +struct tegra_adma_chan_regs {
> +       unsigned int ctrl;
> +       unsigned int config;
> +       unsigned int src_addr;
> +       unsigned int trg_addr;
> +       unsigned int fifo_ctrl;
> +       unsigned int tc;
> +};
> +
> +/*
> + * struct tegra_adma_desc - Tegra ADMA descriptor to manage transfer requests.
> + */
> +struct tegra_adma_desc {
> +       struct virt_dma_desc            vd;
> +       struct tegra_adma_chan_regs     ch_regs;
> +       unsigned long                   bytes_requested;
> +       unsigned long                   bytes_transferred;
> +};
> +
> +/*
> + * struct tegra_adma_chan - Tegra ADMA channel information
> + */
> +struct tegra_adma_chan {
> +       struct virt_dma_chan            vc;
> +       struct tegra_adma_desc          *desc;
> +       struct tegra_adma               *tdma;
> +       char                            name[30];

Is it default naming scheme not enough here?

> +       int                             irq;
> +       void __iomem                    *chan_addr;
> +       spinlock_t                      lock;

Do the virtual channel's lock is not enough?

> +
> +       /* Slave channel configuration info */
> +       struct dma_slave_config         sconfig;
> +       bool                            sconfig_valid;
> +       unsigned int                    sreq_dir;
> +       unsigned int                    sreq_index;
> +       bool                            sreq_reserved;
> +
> +       /* Transfer count and position info */
> +       unsigned int                    tx_buf_count;
> +       unsigned int                    tx_buf_pos;
> +};
> +
> +/*
> + * struct tegra_adma - Tegra ADMA controller information
> + */
> +struct tegra_adma {
> +       struct dma_device                       dma_dev;
> +       struct device                           *dev;
> +       struct clk                              *adma_clk;
> +       void __iomem                            *base_addr;
> +       unsigned int                            nr_channels;
> +       unsigned long                           rx_requests_reserved;
> +       unsigned long                           tx_requests_reserved;
> +
> +       /* Used to store global command register state when suspending */
> +       unsigned int                            global_cmd;
> +
> +       /* Last member of the structure */
> +       struct tegra_adma_chan                  channels[0];
> +};
> +
> +static inline void tdma_write(struct tegra_adma *tdma, u32 reg, u32 val)
> +{
> +       writel(val, tdma->base_addr + reg);
> +}
> +
> +static inline u32 tdma_read(struct tegra_adma *tdma, u32 reg)
> +{
> +       return readl(tdma->base_addr + reg);
> +}
> +
> +static inline void tdma_ch_write(struct tegra_adma_chan *tdc,
> +               u32 reg, u32 val)
> +{
> +       writel(val, tdc->chan_addr + reg);
> +}
> +
> +static inline u32 tdma_ch_read(struct tegra_adma_chan *tdc, u32 reg)
> +{
> +       return readl(tdc->chan_addr + reg);
> +}
> +
> +static inline struct tegra_adma_chan *to_tegra_adma_chan(struct dma_chan *dc)
> +{
> +       return container_of(dc, struct tegra_adma_chan, vc.chan);
> +}
> +
> +static inline struct tegra_adma_desc *to_tegra_adma_desc(
> +               struct dma_async_tx_descriptor *td)
> +{
> +       return container_of(td, struct tegra_adma_desc, vd.tx);
> +}
> +
> +static inline struct device *tdc2dev(struct tegra_adma_chan *tdc)
> +{
> +       return tdc->tdma->dev;
> +}
> +
> +static void tegra_adma_desc_free(struct virt_dma_desc *vd)
> +{
> +       kfree(container_of(vd, struct tegra_adma_desc, vd));
> +}
> +
> +static int tegra_adma_slave_config(struct dma_chan *dc,
> +                                  struct dma_slave_config *sconfig)
> +{
> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +
> +       memcpy(&tdc->sconfig, sconfig, sizeof(*sconfig));
> +       tdc->sconfig_valid = true;

What kind of workflow may end up with wrong slave configuration?

> +
> +       return 0;
> +}
> +
> +static int tegra_adma_init(struct tegra_adma *tdma)
> +{
> +       u32 status;
> +       int ret;
> +
> +       /* Clear any interrupts */
> +       tdma_write(tdma, ADMA_GLOBAL_INT_CLEAR, 0x1);
> +
> +       /* Assert soft reset */
> +       tdma_write(tdma, ADMA_GLOBAL_SOFT_RESET, 0x1);
> +
> +       /* Wait for reset to clear */
> +       ret = readx_poll_timeout(readl,
> +                                tdma->base_addr + ADMA_GLOBAL_SOFT_RESET,
> +                                status, status == 0, 20, 10000);
> +       if (ret)
> +               return ret;
> +
> +       /* Enable global ADMA registers */
> +       tdma_write(tdma, ADMA_GLOBAL_CMD, 1);
> +
> +       return 0;
> +}
> +
> +static int tegra_adma_request_alloc(struct tegra_adma_chan *tdc,
> +                                   unsigned int sreq_dir)
> +{
> +       struct tegra_adma *tdma = tdc->tdma;
> +       unsigned int sreq_index = tdc->sreq_index;
> +
> +       if (tdc->sreq_reserved)
> +               return tdc->sreq_dir == sreq_dir ? 0 : -EINVAL;
> +
> +       switch (sreq_dir) {
> +       case ADMA_CH_CTRL_DIR_MEM2AHUB:
> +               if (sreq_index > ADMA_CH_CTRL_TX_REQ_MAX) {
> +                       dev_err(tdma->dev, "invalid DMA request\n");
> +                       return -EINVAL;
> +               }
> +
> +               if (test_and_set_bit(sreq_index, &tdma->tx_requests_reserved)) {
> +                       dev_err(tdma->dev, "DMA request reserved\n");
> +                       return -EINVAL;
> +               }
> +               break;
> +
> +       case ADMA_CH_CTRL_DIR_AHUB2MEM:
> +               if (sreq_index > ADMA_CH_CTRL_RX_REQ_MAX) {
> +                       dev_err(tdma->dev, "invalid DMA request\n");
> +                       return -EINVAL;
> +               }
> +
> +               if (test_and_set_bit(sreq_index, &tdma->rx_requests_reserved)) {
> +                       dev_err(tdma->dev, "DMA request reserved\n");
> +                       return -EINVAL;
> +               }
> +               break;
> +
> +       default:
> +               dev_WARN(tdma->dev, "channel %s has invalid transfer type\n",
> +                        tdc->name);

Can you use classical enum dma_direction and do conversion to your
values exactly when it's needed?
In such case before you may call helper is_slave_direction(dir).

> +               return -EINVAL;
> +       }
> +
> +       tdc->sreq_dir = sreq_dir;
> +       tdc->sreq_reserved = true;
> +
> +       return 0;
> +}
> +
> +static void tegra_adma_request_free(struct tegra_adma_chan *tdc)
> +{
> +       struct tegra_adma *tdma = tdc->tdma;
> +
> +       if (!tdc->sreq_reserved)
> +               return;
> +
> +       switch (tdc->sreq_dir) {
> +       case ADMA_CH_CTRL_DIR_MEM2AHUB:
> +               clear_bit(tdc->sreq_index, &tdma->tx_requests_reserved);
> +               break;
> +       case ADMA_CH_CTRL_DIR_AHUB2MEM:
> +               clear_bit(tdc->sreq_index, &tdma->rx_requests_reserved);
> +               break;
> +       default:
> +               dev_WARN(tdma->dev, "channel %s has invalid transfer type\n",
> +                        tdc->name);
> +               return;

Ditto.

> +       }
> +
> +       tdc->sreq_reserved = false;
> +}
> +
> +static u32 tegra_adma_irq_status(struct tegra_adma_chan *tdc)
> +{
> +       u32 status = tdma_ch_read(tdc, ADMA_CH_INT_STATUS);
> +
> +       return status & ADMA_CH_INT_STATUS_XFER_DONE;
> +}
> +
> +static u32 tegra_adma_irq_clear(struct tegra_adma_chan *tdc)
> +{
> +       u32 status = tegra_adma_irq_status(tdc);
> +
> +       if (status)
> +               tdma_ch_write(tdc, ADMA_CH_INT_CLEAR, status);
> +
> +       return status;
> +}
> +
> +static void tegra_adma_stop(struct tegra_adma_chan *tdc)
> +{
> +       unsigned int status;
> +
> +       /* Disable ADMA */
> +       tdma_ch_write(tdc, ADMA_CH_CMD, 0);
> +
> +       /* Clear interrupt status */
> +       tegra_adma_irq_clear(tdc);
> +
> +       if (readx_poll_timeout_atomic(readl, tdc->chan_addr + ADMA_CH_STATUS,
> +                       status, !(status & ADMA_CH_STATUS_XFER_EN),
> +                       20, 10000)) {
> +               dev_err(tdc2dev(tdc), "unable to stop DMA channel\n");
> +               return;
> +       }
> +

> +       tdc->desc = NULL;

Would it be memory leak here when called from terminate_all() ?

> +}
> +
> +static void tegra_adma_start(struct tegra_adma_chan *tdc)
> +{
> +       struct virt_dma_desc *vd = vchan_next_desc(&tdc->vc);
> +       struct tegra_adma_chan_regs *ch_regs;
> +       struct tegra_adma_desc *desc;
> +
> +       if (!vd)
> +               return;
> +
> +       list_del(&vd->node);
> +
> +       desc = to_tegra_adma_desc(&vd->tx);
> +

Redundant empty line.

> +       if (!desc) {
> +               dev_warn(tdc2dev(tdc), "unable to start DMA, no descriptor\n");
> +               return;
> +       }
> +
> +       ch_regs = &desc->ch_regs;
> +
> +       tdc->tx_buf_pos = 0;
> +       tdc->tx_buf_count = 0;
> +       tdma_ch_write(tdc, ADMA_CH_TC, ch_regs->tc);
> +       tdma_ch_write(tdc, ADMA_CH_CTRL, ch_regs->ctrl);
> +       tdma_ch_write(tdc, ADMA_CH_LOWER_SRC_ADDR, ch_regs->src_addr);
> +       tdma_ch_write(tdc, ADMA_CH_LOWER_TRG_ADDR, ch_regs->trg_addr);
> +       tdma_ch_write(tdc, ADMA_CH_FIFO_CTRL, ch_regs->fifo_ctrl);
> +       tdma_ch_write(tdc, ADMA_CH_CONFIG, ch_regs->config);
> +
> +       /* Start ADMA */
> +       tdma_ch_write(tdc, ADMA_CH_CMD, 1);
> +
> +       tdc->desc = desc;
> +}
> +
> +static void tegra_adma_update_position(struct tegra_adma_chan *tdc)
> +{
> +       struct tegra_adma_desc *desc = tdc->desc;
> +       unsigned int max = ADMA_CH_XFER_STATUS_COUNT_MASK + 1;
> +       unsigned int pos = tdma_ch_read(tdc, ADMA_CH_XFER_STATUS);
> +
> +       /*
> +        * Handle wrap around of buffer count register
> +        */
> +       if (pos < tdc->tx_buf_pos)
> +               tdc->tx_buf_count += pos + (max - tdc->tx_buf_pos);
> +       else
> +               tdc->tx_buf_count += pos - tdc->tx_buf_pos;
> +
> +       tdc->tx_buf_pos = pos;
> +
> +       desc->bytes_transferred = tdc->tx_buf_count * desc->ch_regs.tc;
> +
> +       /*
> +        * If we are not currently active, then it is safe to read the
> +        * remaining words from the TC_STATUS register and add the partial
> +        * buffer to the total transferred.
> +        */
> +       if (!tdc->desc)

if (desc)
return;
...

?

> +               desc->bytes_transferred += desc->ch_regs.tc -
> +                                          tdma_ch_read(tdc, ADMA_CH_TC_STATUS);
> +}
> +
> +static unsigned int tegra_adma_get_residue(struct tegra_adma_desc *desc)
> +{
> +       return desc->bytes_requested - (desc->bytes_transferred %
> +                                       desc->bytes_requested);
> +}
> +
> +static irqreturn_t tegra_adma_isr(int irq, void *dev_id)
> +{
> +       struct tegra_adma_chan *tdc = dev_id;
> +       unsigned long status;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&tdc->lock, flags);
> +
> +       status = tegra_adma_irq_clear(tdc);
> +       if (status == 0 || !tdc->desc) {
> +               spin_unlock_irqrestore(&tdc->lock, flags);
> +               return IRQ_NONE;
> +       }
> +
> +       vchan_cyclic_callback(&tdc->desc->vd);

> +
> +       spin_unlock_irqrestore(&tdc->lock, flags);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void tegra_adma_issue_pending(struct dma_chan *dc)
> +{
> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&tdc->lock, flags);
> +
> +       if (vchan_issue_pending(&tdc->vc)) {
> +               if (tdc->desc)
> +                       dev_warn(tdc2dev(tdc), "DMA already running\n");

The message makes not much sense here. User can call this as many
times as they want to.

> +               else
> +                       tegra_adma_start(tdc);
> +       }
> +
> +       spin_unlock_irqrestore(&tdc->lock, flags);
> +}
> +
> +static int tegra_adma_terminate_all(struct dma_chan *dc)
> +{
> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +       unsigned long flags;
> +       LIST_HEAD(head);
> +
> +       spin_lock_irqsave(&tdc->lock, flags);
> +
> +       if (tdc->desc)
> +               tegra_adma_stop(tdc);
> +
> +       tegra_adma_request_free(tdc);
> +       vchan_get_all_descriptors(&tdc->vc, &head);
> +       spin_unlock_irqrestore(&tdc->lock, flags);
> +       vchan_dma_desc_free_list(&tdc->vc, &head);
> +
> +       return 0;
> +}
> +
> +static enum dma_status tegra_adma_tx_status(struct dma_chan *dc,
> +                                           dma_cookie_t cookie,
> +                                           struct dma_tx_state *txstate)
> +{
> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +       struct tegra_adma_desc *desc;
> +       struct virt_dma_desc *vd;
> +       enum dma_status ret;
> +       unsigned long flags;
> +       unsigned int residual;
> +
> +       spin_lock_irqsave(&tdc->lock, flags);
> +
> +       ret = dma_cookie_status(dc, cookie, txstate);

No need to run this under spin lock.

> +       if (ret == DMA_COMPLETE || !txstate) {
> +               spin_unlock_irqrestore(&tdc->lock, flags);
> +               return ret;
> +       }
> +
> +       vd = vchan_find_desc(&tdc->vc, cookie);
> +       if (vd) {
> +               desc = to_tegra_adma_desc(&vd->tx);
> +               residual = desc->ch_regs.tc;
> +       } else if (tdc->desc && tdc->desc->vd.tx.cookie == cookie) {
> +               tegra_adma_update_position(tdc);
> +               residual = tegra_adma_get_residue(tdc->desc);
> +       } else {
> +               residual = 0;
> +       }
> +
> +       dma_set_residue(txstate, residual);

This could be out of spin lock. We are protecting data, not the code.

> +
> +       spin_unlock_irqrestore(&tdc->lock, flags);
> +
> +       return ret;
> +}
> +
> +static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc,
> +                                     struct tegra_adma_desc *desc,
> +                                     dma_addr_t buf_addr, size_t buf_len,
> +                                     size_t period_len,
> +                                     enum dma_transfer_direction direction)
> +{
> +       struct tegra_adma_chan_regs *ch_regs = &desc->ch_regs;
> +       unsigned int burst_size, num_bufs, sreq_dir;
> +
> +       num_bufs = buf_len / period_len;
> +
> +       if (num_bufs > ADMA_CH_CONFIG_MAX_BUFS)
> +               return -EINVAL;
> +
> +       switch (direction) {
> +       case DMA_MEM_TO_DEV:
> +               sreq_dir = ADMA_CH_CTRL_DIR_MEM2AHUB;
> +               burst_size = fls(tdc->sconfig.dst_maxburst);
> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
> +               ch_regs->ctrl = ADMA_CH_CTRL_TX_REQ(tdc->sreq_index);
> +               ch_regs->src_addr = buf_addr;
> +               break;
> +
> +       case DMA_DEV_TO_MEM:
> +               sreq_dir = ADMA_CH_CTRL_DIR_AHUB2MEM;
> +               burst_size = fls(tdc->sconfig.src_maxburst);
> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
> +               ch_regs->ctrl = ADMA_CH_CTRL_RX_REQ(tdc->sreq_index);
> +               ch_regs->trg_addr = buf_addr;
> +               break;
> +
> +       default:
> +               dev_err(tdc2dev(tdc), "DMA direction is not supported\n");
> +               return -EINVAL;

Call to is_slave_direction() before switch-case might look better.

> +       }
> +
> +       if (!burst_size || burst_size > ADMA_CH_CONFIG_BURST_16)
> +               burst_size = ADMA_CH_CONFIG_BURST_16;
> +
> +       ch_regs->ctrl |= ADMA_CH_CTRL_DIR(sreq_dir) |
> +                        ADMA_CH_CTRL_MODE_CONTINUOUS |
> +                        ADMA_CH_CTRL_FLOWCTRL_EN;
> +       ch_regs->config |= ADMA_CH_CONFIG_BURST_SIZE(burst_size);
> +       ch_regs->config |= ADMA_CH_CONFIG_WEIGHT_FOR_WRR(1);
> +       ch_regs->fifo_ctrl = ADMA_CH_FIFO_CTRL_DEFAULT;
> +       ch_regs->tc = period_len & ADMA_CH_TC_COUNT_MASK;
> +
> +       return tegra_adma_request_alloc(tdc, sreq_dir);
> +}
> +
> +static struct dma_async_tx_descriptor *tegra_adma_prep_slave_sg(
> +       struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
> +       enum dma_transfer_direction direction, unsigned long flags,
> +       void *context)
> +{
> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +
> +       dev_warn(tdc2dev(tdc), "scatter-gather transfers are not supported\n");

Any plans to do that? If no, just remove the entire function.

> +
> +       return NULL;
> +}
> +
> +static struct dma_async_tx_descriptor *tegra_adma_prep_dma_cyclic(
> +       struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
> +       size_t period_len, enum dma_transfer_direction direction,
> +       unsigned long flags)
> +{
> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +       struct tegra_adma_desc *desc = NULL;
> +
> +       if (!tdc->sconfig_valid) {

Looks like excessive check. If user didn't call slave_config(), it's a
problem of user and it should be fixed. Protective programming here
seems not needed.

> +               dev_err(tdc2dev(tdc), "ADMA slave configuration not set\n");
> +               return NULL;
> +       }
> +
> +       if (!buf_len || !period_len || period_len > ADMA_CH_TC_COUNT_MASK) {
> +               dev_err(tdc2dev(tdc), "invalid buffer/period len\n");
> +               return NULL;
> +       }
> +
> +       if (buf_len % period_len) {
> +               dev_err(tdc2dev(tdc), "buf_len not a multiple of period_len\n");
> +               return NULL;
> +       }
> +
> +       if (!IS_ALIGNED(buf_addr, 4)) {
> +               dev_err(tdc2dev(tdc), "invalid buffer alignment\n");
> +               return NULL;
> +       }
> +
> +       desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
> +       if (!desc)
> +               return NULL;
> +
> +       desc->bytes_transferred = 0;
> +       desc->bytes_requested = buf_len;
> +
> +       if (tegra_adma_set_xfer_params(tdc, desc, buf_addr, buf_len, period_len,
> +                                      direction)) {
> +               kfree(desc);
> +               return NULL;
> +       }
> +
> +       return vchan_tx_prep(&tdc->vc, &desc->vd, flags);
> +}
> +
> +static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
> +{
> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(tdc2dev(tdc));
> +       if (ret)
> +               return ret;
> +
> +       dma_cookie_init(&tdc->vc.chan);
> +       tdc->sconfig_valid = false;
> +
> +       return 0;
> +}
> +
> +static void tegra_adma_free_chan_resources(struct dma_chan *dc)
> +{
> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +
> +       if (tdc->desc)
> +               tegra_adma_terminate_all(dc);

Seems after Lars' patchset be merged this will become redundant.
And you may call this unconditionally of course.

> +
> +       tdc->sconfig_valid = false;
> +       vchan_free_chan_resources(&tdc->vc);
> +
> +       pm_runtime_put(tdc2dev(tdc));

pm_runtime_get_sync() in alloc() till pm_runtime_put() in free() seems
too much to cover in time. Imagine if user allocates resources, but
will never use them. How possible to suspend device?

> +
> +       tegra_adma_request_free(tdc);
> +
> +       tdc->sreq_index = 0;
> +       tdc->sreq_dir = 0;
> +}
> +
> +static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
> +                                          struct of_dma *ofdma)
> +{
> +       struct tegra_adma *tdma = ofdma->of_dma_data;
> +       struct tegra_adma_chan *tdc;
> +       struct dma_chan *chan;
> +       unsigned int sreq_index;
> +
> +       if (dma_spec->args_count != 1)
> +               return NULL;

Shouldn't be already checked by of-dma library?

> +
> +       sreq_index = dma_spec->args[0];
> +
> +       if (sreq_index == 0) {
> +               dev_err(tdma->dev, "DMA request must not be 0\n");

Why not? HW limitation?

> +               return NULL;
> +       }
> +
> +       chan = dma_get_any_slave_channel(&tdma->dma_dev);
> +       if (!chan)
> +               return NULL;
> +
> +       tdc = to_tegra_adma_chan(chan);
> +       tdc->sreq_index = sreq_index;
> +
> +       return chan;
> +}
> +
> +static int tegra_adma_runtime_suspend(struct device *dev)
> +{
> +       struct tegra_adma *tdma = dev_get_drvdata(dev);
> +
> +       tdma->global_cmd = tdma_read(tdma, ADMA_GLOBAL_CMD);
> +
> +       clk_disable_unprepare(tdma->adma_clk);
> +
> +       return 0;
> +}
> +
> +static int tegra_adma_runtime_resume(struct device *dev)
> +{
> +       struct tegra_adma *tdma = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = clk_prepare_enable(tdma->adma_clk);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to enable ADMA clock: %d\n", ret);
> +               return ret;
> +       }
> +
> +       tdma_write(tdma, ADMA_GLOBAL_CMD, tdma->global_cmd);
> +
> +       return 0;
> +}
> +
> +static const struct tegra_adma_chip_data tegra210_chip_data = {
> +       .nr_channels = 22,
> +};
> +
> +static const struct of_device_id tegra_adma_of_match[] = {
> +       { .compatible = "nvidia,tegra210-adma", .data = &tegra210_chip_data },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, tegra_adma_of_match);
> +
> +static int tegra_adma_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *match;
> +       const struct tegra_adma_chip_data *cdata;
> +       struct tegra_adma *tdma;
> +       struct resource *res;
> +       int ret, i;
> +
> +       if (!pdev->dev.of_node) {
> +               dev_err(&pdev->dev, "no device tree node for ADMA\n");
> +               return -ENODEV;
> +       }

Do you need this check since you later call of_match_device() ?

> +
> +       match = of_match_device(tegra_adma_of_match, &pdev->dev);
> +       if (!match) {
> +               dev_err(&pdev->dev, "no device match found\n");
> +               return -ENODEV;
> +       }
> +       cdata = match->data;
> +
> +       tdma = devm_kzalloc(&pdev->dev, sizeof(*tdma) + cdata->nr_channels *
> +                           sizeof(struct tegra_adma_chan), GFP_KERNEL);
> +       if (!tdma)
> +               return -ENOMEM;
> +
> +       tdma->dev = &pdev->dev;
> +       tdma->nr_channels = cdata->nr_channels;
> +       platform_set_drvdata(pdev, tdma);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       tdma->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(tdma->base_addr))
> +               return PTR_ERR(tdma->base_addr);
> +
> +       tdma->adma_clk = devm_clk_get(&pdev->dev, "adma_ape");
> +       if (IS_ERR(tdma->adma_clk)) {
> +               dev_err(&pdev->dev, "ADMA clock not found\n");
> +               return PTR_ERR(tdma->adma_clk);
> +       }
> +
> +       pm_runtime_enable(&pdev->dev);
> +       if (pm_runtime_enabled(&pdev->dev))

> +               ret = pm_runtime_get_sync(&pdev->dev);
> +       else
> +               ret = tegra_adma_runtime_resume(&pdev->dev);
> +
> +       if (ret) {
> +               pm_runtime_disable(&pdev->dev);
> +               return ret;
> +       }
> +
> +       ret = tegra_adma_init(tdma);
> +       if (ret)
> +               goto err_pm_disable;
> +
> +       INIT_LIST_HEAD(&tdma->dma_dev.channels);
> +       for (i = 0; i < tdma->nr_channels; i++) {
> +               struct tegra_adma_chan *tdc = &tdma->channels[i];
> +
> +               tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i);
> +
> +               snprintf(tdc->name, sizeof(tdc->name), "adma.%d", i);
> +
> +               tdc->irq = platform_get_irq(pdev, i);
> +               if (tdc->irq < 0) {

> +                       ret = -EPROBE_DEFER;

So, any failure of getting an IRQ resource will be considered as
deferral? I doubt it's a good idea.

> +                       goto err_irq;
> +               }
> +
> +               ret = devm_request_irq(&pdev->dev, tdc->irq, tegra_adma_isr, 0,
> +                                      tdc->name, tdc);

Better to use request_irq(). There is no benefit of devm_ variant in
few cases (not only DMA Engine drivers).

> +               if (ret) {
> +                       dev_err(&pdev->dev,
> +                               "failed to get interrupt for channel %d\n", i);


> +                       goto err_irq;
> +               }
> +
> +               spin_lock_init(&tdc->lock);
> +               vchan_init(&tdc->vc, &tdma->dma_dev);
> +               tdc->vc.desc_free = tegra_adma_desc_free;
> +               tdc->tdma = tdma;
> +       }
> +
> +       dma_cap_set(DMA_SLAVE, tdma->dma_dev.cap_mask);
> +       dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
> +       dma_cap_set(DMA_CYCLIC, tdma->dma_dev.cap_mask);
> +
> +       tdma->dma_dev.dev = &pdev->dev;
> +       tdma->dma_dev.device_alloc_chan_resources =
> +                                       tegra_adma_alloc_chan_resources;
> +       tdma->dma_dev.device_free_chan_resources =
> +                                       tegra_adma_free_chan_resources;
> +       tdma->dma_dev.device_issue_pending = tegra_adma_issue_pending;
> +       tdma->dma_dev.device_prep_slave_sg = tegra_adma_prep_slave_sg;
> +       tdma->dma_dev.device_prep_dma_cyclic = tegra_adma_prep_dma_cyclic;
> +       tdma->dma_dev.device_config = tegra_adma_slave_config;
> +       tdma->dma_dev.device_tx_status = tegra_adma_tx_status;
> +       tdma->dma_dev.device_terminate_all = tegra_adma_terminate_all;
> +       tdma->dma_dev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> +       tdma->dma_dev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> +
> +       ret = dma_async_device_register(&tdma->dma_dev);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "ADMA registration failed: %d\n", ret);
> +               goto err_irq;
> +       }
> +
> +       ret = of_dma_controller_register(pdev->dev.of_node,
> +                                        tegra_dma_of_xlate, tdma);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "ADMA OF registration failed %d\n", ret);
> +               goto err_unregister_dma_dev;
> +       }
> +
> +       pm_runtime_put(&pdev->dev);

It might be called earlier, mightn't it?

> +
> +       dev_info(&pdev->dev, "Tegra210 ADMA driver registered %d channels\n",
> +                tdma->nr_channels);
> +
> +       return 0;
> +
> +err_unregister_dma_dev:
> +       dma_async_device_unregister(&tdma->dma_dev);
> +err_irq:
> +       while (--i >= 0) {
> +               struct tegra_adma_chan *tdc = &tdma->channels[i];
> +
> +               tasklet_kill(&tdc->vc.task);
> +       }
> +err_pm_disable:
> +       pm_runtime_disable(&pdev->dev);
> +       if (!pm_runtime_status_suspended(&pdev->dev))
> +               tegra_adma_runtime_suspend(&pdev->dev);
> +
> +       return ret;
> +}
> +
> +static int tegra_adma_remove(struct platform_device *pdev)
> +{
> +       struct tegra_adma *tdma = platform_get_drvdata(pdev);
> +       struct tegra_adma_chan *tdc;
> +       int i;
> +
> +       dma_async_device_unregister(&tdma->dma_dev);
> +
> +       for (i = 0; i < tdma->nr_channels; ++i) {
> +               tdc = &tdma->channels[i];
> +               disable_irq(tdc->irq);
> +               tasklet_kill(&tdc->vc.task);
> +       }
> +
> +       pm_runtime_disable(&pdev->dev);
> +       if (!pm_runtime_status_suspended(&pdev->dev))
> +               tegra_adma_runtime_suspend(&pdev->dev);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_adma_pm_suspend(struct device *dev)
> +{
> +       return pm_runtime_suspended(dev);
> +}
> +#endif
> +
> +static const struct dev_pm_ops tegra_adma_dev_pm_ops = {
> +       SET_RUNTIME_PM_OPS(tegra_adma_runtime_suspend,
> +                          tegra_adma_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(tegra_adma_pm_suspend, NULL)

The runtime PM calls and system suspend ones are more or less
equivalent on LATE stage. Also, it's usually a problem with DMA that
you may try to suspend active device used by others.

> +};
> +
> +static struct platform_driver tegra_admac_driver = {
> +       .driver = {
> +               .name   = "tegra-adma",
> +               .pm     = &tegra_adma_dev_pm_ops,
> +               .of_match_table = tegra_adma_of_match,
> +       },
> +       .probe          = tegra_adma_probe,
> +       .remove         = tegra_adma_remove,
> +};
> +
> +module_platform_driver(tegra_admac_driver);
> +
> +MODULE_ALIAS("platform:tegra210-adma");
> +MODULE_DESCRIPTION("NVIDIA Tegra ADMA driver");
> +MODULE_AUTHOR("Dara Ramesh <dramesh@nvidia.com>");
> +MODULE_AUTHOR("Jon Hunter <jonathanh@nvidia.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Jon Hunter]

On 29/10/15 21:03, Andy Shevchenko wrote:
> On Fri, Oct 16, 2015 at 10:35 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Add support for the Tegra210 Audio DMA controller that is used for
>> transferring data between system memory and the Audio sub-system.
>> The driver only supports cyclic transfers because this is being solely
>> used for audio.
>>
>> This driver is based upon the work by Dara Ramesh <dramesh@nvidia.com>.
>>
> 
> 
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/mm.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_dma.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk/tegra.h>
> 
> Do we really need all of them present here?

Good point. I can trim this down quite a bit.

>> +
>> +#include "dmaengine.h"
>> +#include "virt-dma.h"
>> +
>> +#define ADMA_CH_CMD                                    0x00
>> +#define ADMA_CH_STATUS                                 0x0c
>> +#define ADMA_CH_STATUS_XFER_EN                         BIT(0)
>> +
>> +#define ADMA_CH_INT_STATUS                             0x10
>> +#define ADMA_CH_INT_STATUS_XFER_DONE                   BIT(0)
>> +
>> +#define ADMA_CH_INT_CLEAR                              0x1c
>> +#define ADMA_CH_CTRL                                   0x24
>> +#define ADMA_CH_CTRL_TX_REQ(val)                       (((val) & 0xf) << 28)
>> +#define ADMA_CH_CTRL_TX_REQ_MAX                                10
>> +#define ADMA_CH_CTRL_RX_REQ(val)                       (((val) & 0xf) << 24)
>> +#define ADMA_CH_CTRL_RX_REQ_MAX                                10
>> +#define ADMA_CH_CTRL_DIR(val)                          (((val) & 0xf) << 12)
>> +#define ADMA_CH_CTRL_DIR_AHUB2MEM                      2
>> +#define ADMA_CH_CTRL_DIR_MEM2AHUB                      4
>> +#define ADMA_CH_CTRL_MODE_CONTINUOUS                   (2 << 8)
>> +#define ADMA_CH_CTRL_FLOWCTRL_EN                       BIT(1)
>> +
>> +#define ADMA_CH_CONFIG                                 0x28
>> +#define ADMA_CH_CONFIG_SRC_BUF(val)                    (((val) & 0x7) << 28)
>> +#define ADMA_CH_CONFIG_TRG_BUF(val)                    (((val) & 0x7) << 24)
>> +#define ADMA_CH_CONFIG_BURST_SIZE(val)                 (((val) & 0x7) << 20)
>> +#define ADMA_CH_CONFIG_BURST_16                                5
>> +#define ADMA_CH_CONFIG_WEIGHT_FOR_WRR(val)             ((val) & 0xf)
>> +#define ADMA_CH_CONFIG_MAX_BUFS                                8
>> +
>> +#define ADMA_CH_FIFO_CTRL                              0x2c
>> +#define ADMA_CH_FIFO_CTRL_OVRFW_THRES(val)             (((val) & 0xf) << 24)
>> +#define ADMA_CH_FIFO_CTRL_STARV_THRES(val)             (((val) & 0xf) << 16)
>> +#define ADMA_CH_FIFO_CTRL_TX_SIZE(val)                 (((val) & 0xf) << 8)
>> +#define ADMA_CH_FIFO_CTRL_RX_SIZE(val)                 ((val) & 0xf)
>> +
>> +#define ADMA_CH_TC_STATUS                              0x30
>> +#define ADMA_CH_LOWER_SRC_ADDR                         0x34
>> +#define ADMA_CH_LOWER_TRG_ADDR                         0x3c
>> +#define ADMA_CH_TC                                     0x44
>> +#define ADMA_CH_TC_COUNT_MASK                          0x3ffffffc
>> +
>> +#define ADMA_CH_XFER_STATUS                            0x54
>> +#define ADMA_CH_XFER_STATUS_COUNT_MASK                 0xffff
>> +
>> +#define ADMA_GLOBAL_CMD                                        0xc00
>> +#define ADMA_GLOBAL_SOFT_RESET                         0xc04
>> +#define ADMA_GLOBAL_INT_CLEAR                          0xc20
>> +#define ADMA_GLOBAL_CTRL                               0xc24
>> +
>> +#define ADMA_CH_REG_OFFSET(a)                          (a * 0x80)
>> +
>> +#define ADMA_CH_FIFO_CTRL_DEFAULT      (ADMA_CH_FIFO_CTRL_OVRFW_THRES(1) | \
>> +                                        ADMA_CH_FIFO_CTRL_STARV_THRES(1) | \
>> +                                        ADMA_CH_FIFO_CTRL_TX_SIZE(3)     | \
>> +                                        ADMA_CH_FIFO_CTRL_RX_SIZE(3))
>> +struct tegra_adma;
>> +
>> +/*
>> + * struct tegra_adma_chip_data - Tegra chip specific data
>> + * @nr_channels: Number of DMA channels available.
>> + */
>> +struct tegra_adma_chip_data {
>> +       int nr_channels;
>> +};
>> +
>> +/*
>> + * struct tegra_adma_chan_regs - Tegra ADMA channel registers
>> + */
>> +struct tegra_adma_chan_regs {
>> +       unsigned int ctrl;
>> +       unsigned int config;
>> +       unsigned int src_addr;
>> +       unsigned int trg_addr;
>> +       unsigned int fifo_ctrl;
>> +       unsigned int tc;
>> +};
>> +
>> +/*
>> + * struct tegra_adma_desc - Tegra ADMA descriptor to manage transfer requests.
>> + */
>> +struct tegra_adma_desc {
>> +       struct virt_dma_desc            vd;
>> +       struct tegra_adma_chan_regs     ch_regs;
>> +       unsigned long                   bytes_requested;
>> +       unsigned long                   bytes_transferred;
>> +};
>> +
>> +/*
>> + * struct tegra_adma_chan - Tegra ADMA channel information
>> + */
>> +struct tegra_adma_chan {
>> +       struct virt_dma_chan            vc;
>> +       struct tegra_adma_desc          *desc;
>> +       struct tegra_adma               *tdma;
>> +       char                            name[30];
> 
> Is it default naming scheme not enough here?

Yes, will remove this and use dma_chan_name().

>> +       int                             irq;
>> +       void __iomem                    *chan_addr;
>> +       spinlock_t                      lock;
> 
> Do the virtual channel's lock is not enough?

Yes, will remove this as well.

>> +
>> +       /* Slave channel configuration info */
>> +       struct dma_slave_config         sconfig;
>> +       bool                            sconfig_valid;
>> +       unsigned int                    sreq_dir;
>> +       unsigned int                    sreq_index;
>> +       bool                            sreq_reserved;
>> +
>> +       /* Transfer count and position info */
>> +       unsigned int                    tx_buf_count;
>> +       unsigned int                    tx_buf_pos;
>> +};
>> +
>> +/*
>> + * struct tegra_adma - Tegra ADMA controller information
>> + */
>> +struct tegra_adma {
>> +       struct dma_device                       dma_dev;
>> +       struct device                           *dev;
>> +       struct clk                              *adma_clk;
>> +       void __iomem                            *base_addr;
>> +       unsigned int                            nr_channels;
>> +       unsigned long                           rx_requests_reserved;
>> +       unsigned long                           tx_requests_reserved;
>> +
>> +       /* Used to store global command register state when suspending */
>> +       unsigned int                            global_cmd;
>> +
>> +       /* Last member of the structure */
>> +       struct tegra_adma_chan                  channels[0];
>> +};
>> +
>> +static inline void tdma_write(struct tegra_adma *tdma, u32 reg, u32 val)
>> +{
>> +       writel(val, tdma->base_addr + reg);
>> +}
>> +
>> +static inline u32 tdma_read(struct tegra_adma *tdma, u32 reg)
>> +{
>> +       return readl(tdma->base_addr + reg);
>> +}
>> +
>> +static inline void tdma_ch_write(struct tegra_adma_chan *tdc,
>> +               u32 reg, u32 val)
>> +{
>> +       writel(val, tdc->chan_addr + reg);
>> +}
>> +
>> +static inline u32 tdma_ch_read(struct tegra_adma_chan *tdc, u32 reg)
>> +{
>> +       return readl(tdc->chan_addr + reg);
>> +}
>> +
>> +static inline struct tegra_adma_chan *to_tegra_adma_chan(struct dma_chan *dc)
>> +{
>> +       return container_of(dc, struct tegra_adma_chan, vc.chan);
>> +}
>> +
>> +static inline struct tegra_adma_desc *to_tegra_adma_desc(
>> +               struct dma_async_tx_descriptor *td)
>> +{
>> +       return container_of(td, struct tegra_adma_desc, vd.tx);
>> +}
>> +
>> +static inline struct device *tdc2dev(struct tegra_adma_chan *tdc)
>> +{
>> +       return tdc->tdma->dev;
>> +}
>> +
>> +static void tegra_adma_desc_free(struct virt_dma_desc *vd)
>> +{
>> +       kfree(container_of(vd, struct tegra_adma_desc, vd));
>> +}
>> +
>> +static int tegra_adma_slave_config(struct dma_chan *dc,
>> +                                  struct dma_slave_config *sconfig)
>> +{
>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +
>> +       memcpy(&tdc->sconfig, sconfig, sizeof(*sconfig));
>> +       tdc->sconfig_valid = true;
> 
> What kind of workflow may end up with wrong slave configuration?

It is really used to verify that the user has called slave_config before
calling the prep_dma_cyclic(). However, I would agree that this would be
a user issue and be be excessive. So I am happy to get rid of this
completely.

>> +
>> +       return 0;
>> +}
>> +
>> +static int tegra_adma_init(struct tegra_adma *tdma)
>> +{
>> +       u32 status;
>> +       int ret;
>> +
>> +       /* Clear any interrupts */
>> +       tdma_write(tdma, ADMA_GLOBAL_INT_CLEAR, 0x1);
>> +
>> +       /* Assert soft reset */
>> +       tdma_write(tdma, ADMA_GLOBAL_SOFT_RESET, 0x1);
>> +
>> +       /* Wait for reset to clear */
>> +       ret = readx_poll_timeout(readl,
>> +                                tdma->base_addr + ADMA_GLOBAL_SOFT_RESET,
>> +                                status, status == 0, 20, 10000);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Enable global ADMA registers */
>> +       tdma_write(tdma, ADMA_GLOBAL_CMD, 1);
>> +
>> +       return 0;
>> +}
>> +
>> +static int tegra_adma_request_alloc(struct tegra_adma_chan *tdc,
>> +                                   unsigned int sreq_dir)
>> +{
>> +       struct tegra_adma *tdma = tdc->tdma;
>> +       unsigned int sreq_index = tdc->sreq_index;
>> +
>> +       if (tdc->sreq_reserved)
>> +               return tdc->sreq_dir == sreq_dir ? 0 : -EINVAL;
>> +
>> +       switch (sreq_dir) {
>> +       case ADMA_CH_CTRL_DIR_MEM2AHUB:
>> +               if (sreq_index > ADMA_CH_CTRL_TX_REQ_MAX) {
>> +                       dev_err(tdma->dev, "invalid DMA request\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               if (test_and_set_bit(sreq_index, &tdma->tx_requests_reserved)) {
>> +                       dev_err(tdma->dev, "DMA request reserved\n");
>> +                       return -EINVAL;
>> +               }
>> +               break;
>> +
>> +       case ADMA_CH_CTRL_DIR_AHUB2MEM:
>> +               if (sreq_index > ADMA_CH_CTRL_RX_REQ_MAX) {
>> +                       dev_err(tdma->dev, "invalid DMA request\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               if (test_and_set_bit(sreq_index, &tdma->rx_requests_reserved)) {
>> +                       dev_err(tdma->dev, "DMA request reserved\n");
>> +                       return -EINVAL;
>> +               }
>> +               break;
>> +
>> +       default:
>> +               dev_WARN(tdma->dev, "channel %s has invalid transfer type\n",
>> +                        tdc->name);
> 
> Can you use classical enum dma_direction and do conversion to your
> values exactly when it's needed?
> In such case before you may call helper is_slave_direction(dir).

Ok, but I prefer to use the case statement.

>> +               return -EINVAL;
>> +       }
>> +
>> +       tdc->sreq_dir = sreq_dir;
>> +       tdc->sreq_reserved = true;
>> +
>> +       return 0;
>> +}
>> +
>> +static void tegra_adma_request_free(struct tegra_adma_chan *tdc)
>> +{
>> +       struct tegra_adma *tdma = tdc->tdma;
>> +
>> +       if (!tdc->sreq_reserved)
>> +               return;
>> +
>> +       switch (tdc->sreq_dir) {
>> +       case ADMA_CH_CTRL_DIR_MEM2AHUB:
>> +               clear_bit(tdc->sreq_index, &tdma->tx_requests_reserved);
>> +               break;
>> +       case ADMA_CH_CTRL_DIR_AHUB2MEM:
>> +               clear_bit(tdc->sreq_index, &tdma->rx_requests_reserved);
>> +               break;
>> +       default:
>> +               dev_WARN(tdma->dev, "channel %s has invalid transfer type\n",
>> +                        tdc->name);
>> +               return;
> 
> Ditto.

Ok.

>> +       }
>> +
>> +       tdc->sreq_reserved = false;
>> +}
>> +
>> +static u32 tegra_adma_irq_status(struct tegra_adma_chan *tdc)
>> +{
>> +       u32 status = tdma_ch_read(tdc, ADMA_CH_INT_STATUS);
>> +
>> +       return status & ADMA_CH_INT_STATUS_XFER_DONE;
>> +}
>> +
>> +static u32 tegra_adma_irq_clear(struct tegra_adma_chan *tdc)
>> +{
>> +       u32 status = tegra_adma_irq_status(tdc);
>> +
>> +       if (status)
>> +               tdma_ch_write(tdc, ADMA_CH_INT_CLEAR, status);
>> +
>> +       return status;
>> +}
>> +
>> +static void tegra_adma_stop(struct tegra_adma_chan *tdc)
>> +{
>> +       unsigned int status;
>> +
>> +       /* Disable ADMA */
>> +       tdma_ch_write(tdc, ADMA_CH_CMD, 0);
>> +
>> +       /* Clear interrupt status */
>> +       tegra_adma_irq_clear(tdc);
>> +
>> +       if (readx_poll_timeout_atomic(readl, tdc->chan_addr + ADMA_CH_STATUS,
>> +                       status, !(status & ADMA_CH_STATUS_XFER_EN),
>> +                       20, 10000)) {
>> +               dev_err(tdc2dev(tdc), "unable to stop DMA channel\n");
>> +               return;
>> +       }
>> +
> 
>> +       tdc->desc = NULL;
> 
> Would it be memory leak here when called from terminate_all() ?

Yes, good point. Will fix.

>> +}
>> +
>> +static void tegra_adma_start(struct tegra_adma_chan *tdc)
>> +{
>> +       struct virt_dma_desc *vd = vchan_next_desc(&tdc->vc);
>> +       struct tegra_adma_chan_regs *ch_regs;
>> +       struct tegra_adma_desc *desc;
>> +
>> +       if (!vd)
>> +               return;
>> +
>> +       list_del(&vd->node);
>> +
>> +       desc = to_tegra_adma_desc(&vd->tx);
>> +
> 
> Redundant empty line.

Ok.

>> +       if (!desc) {
>> +               dev_warn(tdc2dev(tdc), "unable to start DMA, no descriptor\n");
>> +               return;
>> +       }
>> +
>> +       ch_regs = &desc->ch_regs;
>> +
>> +       tdc->tx_buf_pos = 0;
>> +       tdc->tx_buf_count = 0;
>> +       tdma_ch_write(tdc, ADMA_CH_TC, ch_regs->tc);
>> +       tdma_ch_write(tdc, ADMA_CH_CTRL, ch_regs->ctrl);
>> +       tdma_ch_write(tdc, ADMA_CH_LOWER_SRC_ADDR, ch_regs->src_addr);
>> +       tdma_ch_write(tdc, ADMA_CH_LOWER_TRG_ADDR, ch_regs->trg_addr);
>> +       tdma_ch_write(tdc, ADMA_CH_FIFO_CTRL, ch_regs->fifo_ctrl);
>> +       tdma_ch_write(tdc, ADMA_CH_CONFIG, ch_regs->config);
>> +
>> +       /* Start ADMA */
>> +       tdma_ch_write(tdc, ADMA_CH_CMD, 1);
>> +
>> +       tdc->desc = desc;
>> +}
>> +
>> +static void tegra_adma_update_position(struct tegra_adma_chan *tdc)
>> +{
>> +       struct tegra_adma_desc *desc = tdc->desc;
>> +       unsigned int max = ADMA_CH_XFER_STATUS_COUNT_MASK + 1;
>> +       unsigned int pos = tdma_ch_read(tdc, ADMA_CH_XFER_STATUS);
>> +
>> +       /*
>> +        * Handle wrap around of buffer count register
>> +        */
>> +       if (pos < tdc->tx_buf_pos)
>> +               tdc->tx_buf_count += pos + (max - tdc->tx_buf_pos);
>> +       else
>> +               tdc->tx_buf_count += pos - tdc->tx_buf_pos;
>> +
>> +       tdc->tx_buf_pos = pos;
>> +
>> +       desc->bytes_transferred = tdc->tx_buf_count * desc->ch_regs.tc;
>> +
>> +       /*
>> +        * If we are not currently active, then it is safe to read the
>> +        * remaining words from the TC_STATUS register and add the partial
>> +        * buffer to the total transferred.
>> +        */
>> +       if (!tdc->desc)
> 
> if (desc)
> return;
> ...
> 
> ?

Hmmm ... yes this is wrong.

>> +               desc->bytes_transferred += desc->ch_regs.tc -
>> +                                          tdma_ch_read(tdc, ADMA_CH_TC_STATUS);
>> +}
>> +
>> +static unsigned int tegra_adma_get_residue(struct tegra_adma_desc *desc)
>> +{
>> +       return desc->bytes_requested - (desc->bytes_transferred %
>> +                                       desc->bytes_requested);
>> +}
>> +
>> +static irqreturn_t tegra_adma_isr(int irq, void *dev_id)
>> +{
>> +       struct tegra_adma_chan *tdc = dev_id;
>> +       unsigned long status;
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&tdc->lock, flags);
>> +
>> +       status = tegra_adma_irq_clear(tdc);
>> +       if (status == 0 || !tdc->desc) {
>> +               spin_unlock_irqrestore(&tdc->lock, flags);
>> +               return IRQ_NONE;
>> +       }
>> +
>> +       vchan_cyclic_callback(&tdc->desc->vd);
> 
>> +
>> +       spin_unlock_irqrestore(&tdc->lock, flags);
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static void tegra_adma_issue_pending(struct dma_chan *dc)
>> +{
>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&tdc->lock, flags);
>> +
>> +       if (vchan_issue_pending(&tdc->vc)) {
>> +               if (tdc->desc)
>> +                       dev_warn(tdc2dev(tdc), "DMA already running\n");
> 
> The message makes not much sense here. User can call this as many
> times as they want to.

Ok, will remove.

>> +               else
>> +                       tegra_adma_start(tdc);
>> +       }
>> +
>> +       spin_unlock_irqrestore(&tdc->lock, flags);
>> +}
>> +
>> +static int tegra_adma_terminate_all(struct dma_chan *dc)
>> +{
>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +       unsigned long flags;
>> +       LIST_HEAD(head);
>> +
>> +       spin_lock_irqsave(&tdc->lock, flags);
>> +
>> +       if (tdc->desc)
>> +               tegra_adma_stop(tdc);
>> +
>> +       tegra_adma_request_free(tdc);
>> +       vchan_get_all_descriptors(&tdc->vc, &head);
>> +       spin_unlock_irqrestore(&tdc->lock, flags);
>> +       vchan_dma_desc_free_list(&tdc->vc, &head);
>> +
>> +       return 0;
>> +}
>> +
>> +static enum dma_status tegra_adma_tx_status(struct dma_chan *dc,
>> +                                           dma_cookie_t cookie,
>> +                                           struct dma_tx_state *txstate)
>> +{
>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +       struct tegra_adma_desc *desc;
>> +       struct virt_dma_desc *vd;
>> +       enum dma_status ret;
>> +       unsigned long flags;
>> +       unsigned int residual;
>> +
>> +       spin_lock_irqsave(&tdc->lock, flags);
>> +
>> +       ret = dma_cookie_status(dc, cookie, txstate);
> 
> No need to run this under spin lock.

Ok.

>> +       if (ret == DMA_COMPLETE || !txstate) {
>> +               spin_unlock_irqrestore(&tdc->lock, flags);
>> +               return ret;
>> +       }
>> +
>> +       vd = vchan_find_desc(&tdc->vc, cookie);
>> +       if (vd) {
>> +               desc = to_tegra_adma_desc(&vd->tx);
>> +               residual = desc->ch_regs.tc;
>> +       } else if (tdc->desc && tdc->desc->vd.tx.cookie == cookie) {
>> +               tegra_adma_update_position(tdc);
>> +               residual = tegra_adma_get_residue(tdc->desc);
>> +       } else {
>> +               residual = 0;
>> +       }
>> +
>> +       dma_set_residue(txstate, residual);
> 
> This could be out of spin lock. We are protecting data, not the code.

Ok.

>> +
>> +       spin_unlock_irqrestore(&tdc->lock, flags);
>> +
>> +       return ret;
>> +}
>> +
>> +static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc,
>> +                                     struct tegra_adma_desc *desc,
>> +                                     dma_addr_t buf_addr, size_t buf_len,
>> +                                     size_t period_len,
>> +                                     enum dma_transfer_direction direction)
>> +{
>> +       struct tegra_adma_chan_regs *ch_regs = &desc->ch_regs;
>> +       unsigned int burst_size, num_bufs, sreq_dir;
>> +
>> +       num_bufs = buf_len / period_len;
>> +
>> +       if (num_bufs > ADMA_CH_CONFIG_MAX_BUFS)
>> +               return -EINVAL;
>> +
>> +       switch (direction) {
>> +       case DMA_MEM_TO_DEV:
>> +               sreq_dir = ADMA_CH_CTRL_DIR_MEM2AHUB;
>> +               burst_size = fls(tdc->sconfig.dst_maxburst);
>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
>> +               ch_regs->ctrl = ADMA_CH_CTRL_TX_REQ(tdc->sreq_index);
>> +               ch_regs->src_addr = buf_addr;
>> +               break;
>> +
>> +       case DMA_DEV_TO_MEM:
>> +               sreq_dir = ADMA_CH_CTRL_DIR_AHUB2MEM;
>> +               burst_size = fls(tdc->sconfig.src_maxburst);
>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
>> +               ch_regs->ctrl = ADMA_CH_CTRL_RX_REQ(tdc->sreq_index);
>> +               ch_regs->trg_addr = buf_addr;
>> +               break;
>> +
>> +       default:
>> +               dev_err(tdc2dev(tdc), "DMA direction is not supported\n");
>> +               return -EINVAL;
> 
> Call to is_slave_direction() before switch-case might look better.

I prefer the above.

>> +       }
>> +
>> +       if (!burst_size || burst_size > ADMA_CH_CONFIG_BURST_16)
>> +               burst_size = ADMA_CH_CONFIG_BURST_16;
>> +
>> +       ch_regs->ctrl |= ADMA_CH_CTRL_DIR(sreq_dir) |
>> +                        ADMA_CH_CTRL_MODE_CONTINUOUS |
>> +                        ADMA_CH_CTRL_FLOWCTRL_EN;
>> +       ch_regs->config |= ADMA_CH_CONFIG_BURST_SIZE(burst_size);
>> +       ch_regs->config |= ADMA_CH_CONFIG_WEIGHT_FOR_WRR(1);
>> +       ch_regs->fifo_ctrl = ADMA_CH_FIFO_CTRL_DEFAULT;
>> +       ch_regs->tc = period_len & ADMA_CH_TC_COUNT_MASK;
>> +
>> +       return tegra_adma_request_alloc(tdc, sreq_dir);
>> +}
>> +
>> +static struct dma_async_tx_descriptor *tegra_adma_prep_slave_sg(
>> +       struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
>> +       enum dma_transfer_direction direction, unsigned long flags,
>> +       void *context)
>> +{
>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +
>> +       dev_warn(tdc2dev(tdc), "scatter-gather transfers are not supported\n");
> 
> Any plans to do that? If no, just remove the entire function.

No not at the moment. I thought about that but then I am wondering if we
need to add a check in the include/linux/dmaengine.h that the
prep_slave_sg() is populated?

>> +
>> +       return NULL;
>> +}
>> +
>> +static struct dma_async_tx_descriptor *tegra_adma_prep_dma_cyclic(
>> +       struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
>> +       size_t period_len, enum dma_transfer_direction direction,
>> +       unsigned long flags)
>> +{
>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +       struct tegra_adma_desc *desc = NULL;
>> +
>> +       if (!tdc->sconfig_valid) {
> 
> Looks like excessive check. If user didn't call slave_config(), it's a
> problem of user and it should be fixed. Protective programming here
> seems not needed.

Ok, will remove.

>> +               dev_err(tdc2dev(tdc), "ADMA slave configuration not set\n");
>> +               return NULL;
>> +       }
>> +
>> +       if (!buf_len || !period_len || period_len > ADMA_CH_TC_COUNT_MASK) {
>> +               dev_err(tdc2dev(tdc), "invalid buffer/period len\n");
>> +               return NULL;
>> +       }
>> +
>> +       if (buf_len % period_len) {
>> +               dev_err(tdc2dev(tdc), "buf_len not a multiple of period_len\n");
>> +               return NULL;
>> +       }
>> +
>> +       if (!IS_ALIGNED(buf_addr, 4)) {
>> +               dev_err(tdc2dev(tdc), "invalid buffer alignment\n");
>> +               return NULL;
>> +       }
>> +
>> +       desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
>> +       if (!desc)
>> +               return NULL;
>> +
>> +       desc->bytes_transferred = 0;
>> +       desc->bytes_requested = buf_len;
>> +
>> +       if (tegra_adma_set_xfer_params(tdc, desc, buf_addr, buf_len, period_len,
>> +                                      direction)) {
>> +               kfree(desc);
>> +               return NULL;
>> +       }
>> +
>> +       return vchan_tx_prep(&tdc->vc, &desc->vd, flags);
>> +}
>> +
>> +static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
>> +{
>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +       int ret;
>> +
>> +       ret = pm_runtime_get_sync(tdc2dev(tdc));
>> +       if (ret)
>> +               return ret;
>> +
>> +       dma_cookie_init(&tdc->vc.chan);
>> +       tdc->sconfig_valid = false;
>> +
>> +       return 0;
>> +}
>> +
>> +static void tegra_adma_free_chan_resources(struct dma_chan *dc)
>> +{
>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +
>> +       if (tdc->desc)
>> +               tegra_adma_terminate_all(dc);
> 
> Seems after Lars' patchset be merged this will become redundant.
> And you may call this unconditionally of course.

Ok.

>> +
>> +       tdc->sconfig_valid = false;
>> +       vchan_free_chan_resources(&tdc->vc);
>> +
>> +       pm_runtime_put(tdc2dev(tdc));
> 
> pm_runtime_get_sync() in alloc() till pm_runtime_put() in free() seems
> too much to cover in time. Imagine if user allocates resources, but
> will never use them. How possible to suspend device?

In the current use-case (for audio) the dma channel is allocated and
freed everytime audio is started and stopped. However, if audio is
active at the time of suspend then yes it would block. May be I can move
this to the start/stop of the ADMA.

>> +
>> +       tegra_adma_request_free(tdc);
>> +
>> +       tdc->sreq_index = 0;
>> +       tdc->sreq_dir = 0;
>> +}
>> +
>> +static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
>> +                                          struct of_dma *ofdma)
>> +{
>> +       struct tegra_adma *tdma = ofdma->of_dma_data;
>> +       struct tegra_adma_chan *tdc;
>> +       struct dma_chan *chan;
>> +       unsigned int sreq_index;
>> +
>> +       if (dma_spec->args_count != 1)
>> +               return NULL;
> 
> Shouldn't be already checked by of-dma library?

Right so the of-dma library checks for #dma-cells which should be 1 and
this is another test to ensure that the #dma-cells is actually 1. So
this may be redundant.

>> +
>> +       sreq_index = dma_spec->args[0];
>> +
>> +       if (sreq_index == 0) {
>> +               dev_err(tdma->dev, "DMA request must not be 0\n");
> 
> Why not? HW limitation?

Yes, the requests start at 1.

>> +               return NULL;
>> +       }
>> +
>> +       chan = dma_get_any_slave_channel(&tdma->dma_dev);
>> +       if (!chan)
>> +               return NULL;
>> +
>> +       tdc = to_tegra_adma_chan(chan);
>> +       tdc->sreq_index = sreq_index;
>> +
>> +       return chan;
>> +}
>> +
>> +static int tegra_adma_runtime_suspend(struct device *dev)
>> +{
>> +       struct tegra_adma *tdma = dev_get_drvdata(dev);
>> +
>> +       tdma->global_cmd = tdma_read(tdma, ADMA_GLOBAL_CMD);
>> +
>> +       clk_disable_unprepare(tdma->adma_clk);
>> +
>> +       return 0;
>> +}
>> +
>> +static int tegra_adma_runtime_resume(struct device *dev)
>> +{
>> +       struct tegra_adma *tdma = dev_get_drvdata(dev);
>> +       int ret;
>> +
>> +       ret = clk_prepare_enable(tdma->adma_clk);
>> +       if (ret < 0) {
>> +               dev_err(dev, "failed to enable ADMA clock: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       tdma_write(tdma, ADMA_GLOBAL_CMD, tdma->global_cmd);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct tegra_adma_chip_data tegra210_chip_data = {
>> +       .nr_channels = 22,
>> +};
>> +
>> +static const struct of_device_id tegra_adma_of_match[] = {
>> +       { .compatible = "nvidia,tegra210-adma", .data = &tegra210_chip_data },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, tegra_adma_of_match);
>> +
>> +static int tegra_adma_probe(struct platform_device *pdev)
>> +{
>> +       const struct of_device_id *match;
>> +       const struct tegra_adma_chip_data *cdata;
>> +       struct tegra_adma *tdma;
>> +       struct resource *res;
>> +       int ret, i;
>> +
>> +       if (!pdev->dev.of_node) {
>> +               dev_err(&pdev->dev, "no device tree node for ADMA\n");
>> +               return -ENODEV;
>> +       }
> 
> Do you need this check since you later call of_match_device() ?

No, I will drop this.

>> +
>> +       match = of_match_device(tegra_adma_of_match, &pdev->dev);
>> +       if (!match) {
>> +               dev_err(&pdev->dev, "no device match found\n");
>> +               return -ENODEV;
>> +       }
>> +       cdata = match->data;
>> +
>> +       tdma = devm_kzalloc(&pdev->dev, sizeof(*tdma) + cdata->nr_channels *
>> +                           sizeof(struct tegra_adma_chan), GFP_KERNEL);
>> +       if (!tdma)
>> +               return -ENOMEM;
>> +
>> +       tdma->dev = &pdev->dev;
>> +       tdma->nr_channels = cdata->nr_channels;
>> +       platform_set_drvdata(pdev, tdma);
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       tdma->base_addr = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(tdma->base_addr))
>> +               return PTR_ERR(tdma->base_addr);
>> +
>> +       tdma->adma_clk = devm_clk_get(&pdev->dev, "adma_ape");
>> +       if (IS_ERR(tdma->adma_clk)) {
>> +               dev_err(&pdev->dev, "ADMA clock not found\n");
>> +               return PTR_ERR(tdma->adma_clk);
>> +       }
>> +
>> +       pm_runtime_enable(&pdev->dev);
>> +       if (pm_runtime_enabled(&pdev->dev))
> 
>> +               ret = pm_runtime_get_sync(&pdev->dev);
>> +       else
>> +               ret = tegra_adma_runtime_resume(&pdev->dev);
>> +
>> +       if (ret) {
>> +               pm_runtime_disable(&pdev->dev);
>> +               return ret;
>> +       }
>> +
>> +       ret = tegra_adma_init(tdma);
>> +       if (ret)
>> +               goto err_pm_disable;
>> +
>> +       INIT_LIST_HEAD(&tdma->dma_dev.channels);
>> +       for (i = 0; i < tdma->nr_channels; i++) {
>> +               struct tegra_adma_chan *tdc = &tdma->channels[i];
>> +
>> +               tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i);
>> +
>> +               snprintf(tdc->name, sizeof(tdc->name), "adma.%d", i);
>> +
>> +               tdc->irq = platform_get_irq(pdev, i);
>> +               if (tdc->irq < 0) {
> 
>> +                       ret = -EPROBE_DEFER;
> 
> So, any failure of getting an IRQ resource will be considered as
> deferral? I doubt it's a good idea.

Yes. Why not? What if the interrupt controller has not been probed yet?

>> +                       goto err_irq;
>> +               }
>> +
>> +               ret = devm_request_irq(&pdev->dev, tdc->irq, tegra_adma_isr, 0,
>> +                                      tdc->name, tdc);
> 
> Better to use request_irq(). There is no benefit of devm_ variant in
> few cases (not only DMA Engine drivers).

Yes I am changing that.

>> +               if (ret) {
>> +                       dev_err(&pdev->dev,
>> +                               "failed to get interrupt for channel %d\n", i);
> 
> 
>> +                       goto err_irq;
>> +               }
>> +
>> +               spin_lock_init(&tdc->lock);
>> +               vchan_init(&tdc->vc, &tdma->dma_dev);
>> +               tdc->vc.desc_free = tegra_adma_desc_free;
>> +               tdc->tdma = tdma;
>> +       }
>> +
>> +       dma_cap_set(DMA_SLAVE, tdma->dma_dev.cap_mask);
>> +       dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
>> +       dma_cap_set(DMA_CYCLIC, tdma->dma_dev.cap_mask);
>> +
>> +       tdma->dma_dev.dev = &pdev->dev;
>> +       tdma->dma_dev.device_alloc_chan_resources =
>> +                                       tegra_adma_alloc_chan_resources;
>> +       tdma->dma_dev.device_free_chan_resources =
>> +                                       tegra_adma_free_chan_resources;
>> +       tdma->dma_dev.device_issue_pending = tegra_adma_issue_pending;
>> +       tdma->dma_dev.device_prep_slave_sg = tegra_adma_prep_slave_sg;
>> +       tdma->dma_dev.device_prep_dma_cyclic = tegra_adma_prep_dma_cyclic;
>> +       tdma->dma_dev.device_config = tegra_adma_slave_config;
>> +       tdma->dma_dev.device_tx_status = tegra_adma_tx_status;
>> +       tdma->dma_dev.device_terminate_all = tegra_adma_terminate_all;
>> +       tdma->dma_dev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> +       tdma->dma_dev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> +
>> +       ret = dma_async_device_register(&tdma->dma_dev);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev, "ADMA registration failed: %d\n", ret);
>> +               goto err_irq;
>> +       }
>> +
>> +       ret = of_dma_controller_register(pdev->dev.of_node,
>> +                                        tegra_dma_of_xlate, tdma);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev, "ADMA OF registration failed %d\n", ret);
>> +               goto err_unregister_dma_dev;
>> +       }
>> +
>> +       pm_runtime_put(&pdev->dev);
> 
> It might be called earlier, mightn't it?

It should not be.

>> +
>> +       dev_info(&pdev->dev, "Tegra210 ADMA driver registered %d channels\n",
>> +                tdma->nr_channels);
>> +
>> +       return 0;
>> +
>> +err_unregister_dma_dev:
>> +       dma_async_device_unregister(&tdma->dma_dev);
>> +err_irq:
>> +       while (--i >= 0) {
>> +               struct tegra_adma_chan *tdc = &tdma->channels[i];
>> +
>> +               tasklet_kill(&tdc->vc.task);
>> +       }
>> +err_pm_disable:
>> +       pm_runtime_disable(&pdev->dev);
>> +       if (!pm_runtime_status_suspended(&pdev->dev))
>> +               tegra_adma_runtime_suspend(&pdev->dev);
>> +
>> +       return ret;
>> +}
>> +
>> +static int tegra_adma_remove(struct platform_device *pdev)
>> +{
>> +       struct tegra_adma *tdma = platform_get_drvdata(pdev);
>> +       struct tegra_adma_chan *tdc;
>> +       int i;
>> +
>> +       dma_async_device_unregister(&tdma->dma_dev);
>> +
>> +       for (i = 0; i < tdma->nr_channels; ++i) {
>> +               tdc = &tdma->channels[i];
>> +               disable_irq(tdc->irq);
>> +               tasklet_kill(&tdc->vc.task);
>> +       }
>> +
>> +       pm_runtime_disable(&pdev->dev);
>> +       if (!pm_runtime_status_suspended(&pdev->dev))
>> +               tegra_adma_runtime_suspend(&pdev->dev);
>> +
>> +       return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int tegra_adma_pm_suspend(struct device *dev)
>> +{
>> +       return pm_runtime_suspended(dev);
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops tegra_adma_dev_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(tegra_adma_runtime_suspend,
>> +                          tegra_adma_runtime_resume, NULL)
>> +       SET_SYSTEM_SLEEP_PM_OPS(tegra_adma_pm_suspend, NULL)
> 
> The runtime PM calls and system suspend ones are more or less
> equivalent on LATE stage. Also, it's usually a problem with DMA that
> you may try to suspend active device used by others.

I am not sure I follow your point here.

Thanks for the feedback.

Cheers
Jon

Re: [PATCH V3 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Jon Hunter]

On 02/11/15 16:22, Jon Hunter wrote:
> On 29/10/15 21:03, Andy Shevchenko wrote:
>> On Fri, Oct 16, 2015 at 10:35 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> Add support for the Tegra210 Audio DMA controller that is used for
>>> transferring data between system memory and the Audio sub-system.
>>> The driver only supports cyclic transfers because this is being solely
>>> used for audio.
>>>
>>> This driver is based upon the work by Dara Ramesh <dramesh@nvidia.com>.

[snip]

>>> +static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
>>> +{
>>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>>> +       int ret;
>>> +
>>> +       ret = pm_runtime_get_sync(tdc2dev(tdc));
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       dma_cookie_init(&tdc->vc.chan);
>>> +       tdc->sconfig_valid = false;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static void tegra_adma_free_chan_resources(struct dma_chan *dc)
>>> +{
>>> +       struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>>> +
>>> +       if (tdc->desc)
>>> +               tegra_adma_terminate_all(dc);
>>
>> Seems after Lars' patchset be merged this will become redundant.
>> And you may call this unconditionally of course.
> 
> Ok.
> 
>>> +
>>> +       tdc->sconfig_valid = false;
>>> +       vchan_free_chan_resources(&tdc->vc);
>>> +
>>> +       pm_runtime_put(tdc2dev(tdc));
>>
>> pm_runtime_get_sync() in alloc() till pm_runtime_put() in free() seems
>> too much to cover in time. Imagine if user allocates resources, but
>> will never use them. How possible to suspend device?
> 
> In the current use-case (for audio) the dma channel is allocated and
> freed everytime audio is started and stopped. However, if audio is
> active at the time of suspend then yes it would block. May be I can move
> this to the start/stop of the ADMA.

I have been looking at this and although I could potentially improve
this from a power perspective, I would need to ensure I handle all
potential races (ie between interrupts and turning off clocks). It is
do-able, but for now I would prefer to leave as is. The rationale being
that the sole purpose of the DMA is for audio and in the current use
case the DMA channels are acquired and freed when audio is started and
stopped.

I have also looked at a few other DMA drivers and several all seem to do
the same. That is turn on clocks in allocate and turn them off in the free.

Cheers
Jon

Re: [PATCH V3 0/2] Add support for Tegra210 ADMA

[Paul Gortmaker]

On Fri, Oct 16, 2015 at 3:35 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> Add support for the Tegra210 Audio DMA (ADMA) controller. This was
> originally distributed as an RFC [0] based upon the existing tegra
> APB-DMA driver. Since then the driver has been significantly
> re-worked to remove a lot of the unused/unnecessary functionality
> that was carried over from the APB-DMA. This version is no longer
> derived from the APB-DMA driver and has been updated to use the
> virt-dma helpers.

Does this driver make sense to support modular use?  I ask because
it inherits the inconsistency of being a bool Kconfig but using some
modular infrastructure from the APB driver.

Sometime ago  when I sent patches to dma/* to "demodularize" other
bools, the general consensus of reviewers/maintainers was that
(to paraphrase) "if it can be modular, it should be modular" -- in
order to keep the multi platform bzImage size as small as can be.

I did a quick test with it tristate, but that fails as follows:

ERROR: "pm_clk_add_clk" [drivers/dma/tegra210-adma.ko] undefined!
ERROR: "pm_clk_create" [drivers/dma/tegra210-adma.ko] undefined!
ERROR: "pm_clk_destroy" [drivers/dma/tegra210-adma.ko] undefined!
ERROR: "pm_clk_suspend" [drivers/dma/tegra210-adma.ko] undefined!
ERROR: "pm_clk_resume" [drivers/dma/tegra210-adma.ko] undefined!

...since drivers/base/power/clock_ops.c does not currently have any of
those fcns as EXPORT_SYMBOL.  Not a show-stopper, but I don't
want to blindly export those fcns if it doesn't make sense.

Thanks,
Paul.
--

>
> V3 changes:
> - Updated DT binding per feedback from Mark and Stephen
> - Fixed up items mentioned by Vinod
>
> V2 changes:
> - Re-worked device-tree binding
>
> [0] https://lkml.org/lkml/2015/8/18/237
>
> Jon Hunter (2):
>   Documentation: DT: Add binding documentation for NVIDIA ADMA
>   dmaengine: tegra-adma: Add support for Tegra210 ADMA
>
>  .../devicetree/bindings/dma/tegra210-adma.txt      |  50 ++
>  drivers/dma/Kconfig                                |  13 +
>  drivers/dma/Makefile                               |   1 +
>  drivers/dma/tegra210-adma.c                        | 909 +++++++++++++++++++++
>  4 files changed, 973 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
>  create mode 100644 drivers/dma/tegra210-adma.c
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V3 0/2] Add support for Tegra210 ADMA

[Vinod Koul]

On Fri, May 20, 2016 at 02:51:14PM -0400, Paul Gortmaker wrote:
> On Fri, Oct 16, 2015 at 3:35 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> > Add support for the Tegra210 Audio DMA (ADMA) controller. This was
> > originally distributed as an RFC [0] based upon the existing tegra
> > APB-DMA driver. Since then the driver has been significantly
> > re-worked to remove a lot of the unused/unnecessary functionality
> > that was carried over from the APB-DMA. This version is no longer
> > derived from the APB-DMA driver and has been updated to use the
> > virt-dma helpers.
> 
> Does this driver make sense to support modular use?  I ask because
> it inherits the inconsistency of being a bool Kconfig but using some
> modular infrastructure from the APB driver.
> 
> Sometime ago  when I sent patches to dma/* to "demodularize" other
> bools, the general consensus of reviewers/maintainers was that
> (to paraphrase) "if it can be modular, it should be modular" -- in
> order to keep the multi platform bzImage size as small as can be.
> 
> I did a quick test with it tristate, but that fails as follows:
> 
> ERROR: "pm_clk_add_clk" [drivers/dma/tegra210-adma.ko] undefined!
> ERROR: "pm_clk_create" [drivers/dma/tegra210-adma.ko] undefined!
> ERROR: "pm_clk_destroy" [drivers/dma/tegra210-adma.ko] undefined!
> ERROR: "pm_clk_suspend" [drivers/dma/tegra210-adma.ko] undefined!
> ERROR: "pm_clk_resume" [drivers/dma/tegra210-adma.ko] undefined!
> 
> ...since drivers/base/power/clock_ops.c does not currently have any of
> those fcns as EXPORT_SYMBOL.  Not a show-stopper, but I don't
> want to blindly export those fcns if it doesn't make sense.

Sure, patch is welcome!

-- 
~Vinod

Re: [PATCH V3 0/2] Add support for Tegra210 ADMA

[Jon Hunter]

Hi Paul,

On 24/05/16 04:53, Vinod Koul wrote:
> On Fri, May 20, 2016 at 02:51:14PM -0400, Paul Gortmaker wrote:
>> On Fri, Oct 16, 2015 at 3:35 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> Add support for the Tegra210 Audio DMA (ADMA) controller. This was
>>> originally distributed as an RFC [0] based upon the existing tegra
>>> APB-DMA driver. Since then the driver has been significantly
>>> re-worked to remove a lot of the unused/unnecessary functionality
>>> that was carried over from the APB-DMA. This version is no longer
>>> derived from the APB-DMA driver and has been updated to use the
>>> virt-dma helpers.
>>
>> Does this driver make sense to support modular use?  I ask because
>> it inherits the inconsistency of being a bool Kconfig but using some
>> modular infrastructure from the APB driver.
>>
>> Sometime ago  when I sent patches to dma/* to "demodularize" other
>> bools, the general consensus of reviewers/maintainers was that
>> (to paraphrase) "if it can be modular, it should be modular" -- in
>> order to keep the multi platform bzImage size as small as can be.
>>
>> I did a quick test with it tristate, but that fails as follows:
>>
>> ERROR: "pm_clk_add_clk" [drivers/dma/tegra210-adma.ko] undefined!
>> ERROR: "pm_clk_create" [drivers/dma/tegra210-adma.ko] undefined!
>> ERROR: "pm_clk_destroy" [drivers/dma/tegra210-adma.ko] undefined!
>> ERROR: "pm_clk_suspend" [drivers/dma/tegra210-adma.ko] undefined!
>> ERROR: "pm_clk_resume" [drivers/dma/tegra210-adma.ko] undefined!

Thanks for reporting.

>> ...since drivers/base/power/clock_ops.c does not currently have any of
>> those fcns as EXPORT_SYMBOL.  Not a show-stopper, but I don't
>> want to blindly export those fcns if it doesn't make sense.
> 
> Sure, patch is welcome!

I don't have strong feeling either way. I think that long-term it would
be nice to add EXPORTS because I could see a lot of drivers starting to
use these. However, seeing that the ADMA is currently the only one it is
fine to demodularise the ADMA driver for now. Let me know if you want me
to send a patch or not.

Cheers
Jon

-- 
nvpublic