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

[PATCH V2 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.

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      |  63 ++
 drivers/dma/Kconfig                                |  13 +
 drivers/dma/Makefile                               |   1 +
 drivers/dma/tegra210-adma.c                        | 888 +++++++++++++++++++++
 4 files changed, 965 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
 create mode 100644 drivers/dma/tegra210-adma.c

-- 
2.1.4

[PATCH V2 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      | 63 ++++++++++++++++++++++
 1 file changed, 63 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..df0e46868a63
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
@@ -0,0 +1,63 @@
+* 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-channels: Must be 22. Defines the number of DMA channels supported
+  by the DMA controller.
+- dma-rx-requests: Must be 10. Defines the number of receive request
+  signals supported by the DMA controller.
+- dma-tx-requests: Must be 10. Defines the number of transmit request
+  signals supported by the DMA controller.
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum number
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
+  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+  the channel is a receive or transmit channel and must be either 2 for
+  a receive channel and 4 for a transmit channel. These values correspond
+  to the TRANSFER_DIRECTION field of 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-channels = <22>;
+	dma-rx-requests = <10>;
+	dma-tx-requests = <10>;
+	#dma-cells = <2>;
+};
-- 
2.1.4

[PATCH V2 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 | 888 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 902 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..fb6a66106162
--- /dev/null
+++ b/drivers/dma/tegra210-adma.c
@@ -0,0 +1,888 @@
+/*
+ * 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_RX_REQ(val)			(((val) & 0xf) << 24)
+#define ADMA_CH_CTRL_XFER_DIR(val)			(((val) & 0xf) << 12)
+#define ADMA_CH_CTRL_XFER_MODE_CONTINUOUS		(2 << 8)
+#define ADMA_CH_CTRL_XFER_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_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_BURSTSIZE_16				5
+#define ADMA_AHUB_TO_MEM				2
+#define ADMA_MEM_TO_AHUB				4
+
+#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;
+
+/*
+ * 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		config;
+	bool				config_valid;
+	unsigned int			slave_dir;
+	unsigned int			slave_req;
+
+	/* 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 int				rx_requests;
+	unsigned int				rx_requests_reserved;
+	unsigned int				tx_requests;
+	unsigned int				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 *config)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+
+	memcpy(&tdc->config, config, sizeof(*config));
+	tdc->config_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 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);
+
+	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) {
+		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;
+
+	num_bufs = buf_len / period_len;
+
+	if (num_bufs > ADMA_CH_CONFIG_MAX_BUFS)
+		return -EINVAL;
+
+	switch (direction) {
+	case DMA_MEM_TO_DEV:
+		if (tdc->slave_dir != ADMA_MEM_TO_AHUB)
+			return -EINVAL;
+
+		burst_size = fls(tdc->config.dst_maxburst);
+		ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
+		ch_regs->ctrl = ADMA_CH_CTRL_TX_REQ(tdc->slave_req);
+		ch_regs->src_addr = buf_addr;
+		break;
+
+	case DMA_DEV_TO_MEM:
+		if (tdc->slave_dir != ADMA_AHUB_TO_MEM)
+			return -EINVAL;
+
+		burst_size = fls(tdc->config.src_maxburst);
+		ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
+		ch_regs->ctrl = ADMA_CH_CTRL_RX_REQ(tdc->slave_req);
+		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_BURSTSIZE_16)
+		burst_size = ADMA_BURSTSIZE_16;
+
+	ch_regs->ctrl |= ADMA_CH_CTRL_XFER_DIR(tdc->slave_dir) |
+			 ADMA_CH_CTRL_XFER_MODE_CONTINUOUS |
+			 ADMA_CH_CTRL_XFER_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 0;
+}
+
+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->config_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_ATOMIC);
+	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->config_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);
+	struct tegra_adma *tdma = tdc->tdma;
+
+	if (tdc->desc)
+		tegra_adma_terminate_all(dc);
+
+	tdc->config_valid = false;
+	vchan_free_chan_resources(&tdc->vc);
+
+	pm_runtime_put(tdc2dev(tdc));
+
+	if (tdc->slave_req) {
+		switch (tdc->slave_dir) {
+		case ADMA_AHUB_TO_MEM:
+			tdma->rx_requests_reserved &= ~BIT(tdc->slave_req);
+			break;
+		case ADMA_MEM_TO_AHUB:
+			tdma->tx_requests_reserved &= ~BIT(tdc->slave_req);
+			break;
+		default:
+			dev_WARN(tdc2dev(tdc),
+				 "channel freed with invalid transfer type\n");
+			break;
+		}
+
+		tdc->slave_req = 0;
+		tdc->slave_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 slave_dir, slave_req;
+
+	if (dma_spec->args_count != 2)
+		return NULL;
+
+	slave_req = dma_spec->args[0];
+	slave_dir = dma_spec->args[1];
+
+	if (slave_req == 0)
+		return NULL;
+
+	switch (slave_dir) {
+	case ADMA_AHUB_TO_MEM:
+		if (slave_req > tdma->rx_requests)
+			return NULL;
+
+		if (tdma->rx_requests_reserved & BIT(slave_req))
+			return NULL;
+
+		tdma->rx_requests_reserved |= BIT(slave_req);
+		break;
+	case ADMA_MEM_TO_AHUB:
+		if (slave_req > tdma->tx_requests)
+			return NULL;
+
+		if (tdma->tx_requests_reserved & BIT(slave_req))
+			return NULL;
+
+		tdma->tx_requests_reserved |= BIT(slave_req);
+		break;
+	default:
+		dev_err(tdma->dev, "invalid transfer type\n");
+		return NULL;
+	}
+
+	chan = dma_get_any_slave_channel(&tdma->dma_dev);
+	if (!chan)
+		return NULL;
+
+	tdc = to_tegra_adma_chan(chan);
+	tdc->slave_req = slave_req;
+	tdc->slave_dir = slave_dir;
+
+	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 of_device_id tegra_adma_of_match[] = {
+	{ .compatible = "nvidia,tegra210-adma", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_adma_of_match);
+
+static int tegra_adma_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct tegra_adma *tdma;
+	struct resource	*res;
+	unsigned int nr_channels;
+	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;
+	}
+
+	ret = of_property_read_u32(pdev->dev.of_node, "dma-channels",
+				   &nr_channels);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to find dma-channels property\n");
+		return -EINVAL;
+	}
+
+	tdma = devm_kzalloc(&pdev->dev, sizeof(*tdma) + nr_channels *
+			    sizeof(struct tegra_adma_chan), GFP_KERNEL);
+	if (!tdma)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(pdev->dev.of_node, "dma-rx-requests",
+				   &tdma->rx_requests);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to find dma-rx-requests property\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(pdev->dev.of_node, "dma-tx-requests",
+				   &tdma->tx_requests);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to find dma-tx-requests property\n");
+		return -EINVAL;
+	}
+
+	tdma->dev = &pdev->dev;
+	tdma->nr_channels = 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];
+		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 V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

[Mark Rutland]

On Mon, Oct 05, 2015 at 01:10:06PM +0100, Jon Hunter wrote:
> 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      | 63 ++++++++++++++++++++++
>  1 file changed, 63 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..df0e46868a63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
> @@ -0,0 +1,63 @@
> +* 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-channels: Must be 22. Defines the number of DMA channels supported
> +  by the DMA controller.

If this has to be a fixed value, why is it necessary? Why does the
driver not just know this?

Are there other instances of this IP block where this differs?

> +- dma-rx-requests: Must be 10. Defines the number of receive request
> +  signals supported by the DMA controller.
> +- dma-tx-requests: Must be 10. Defines the number of transmit request
> +  signals supported by the DMA controller.

Likewise for these two?

Mark.

> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
> +  receive request number and should be between 1 and the maximum number
> +  of requests supported (see properties "dma-rx-requests" and
> +  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
> +  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
> +  the channel is a receive or transmit channel and must be either 2 for
> +  a receive channel and 4 for a transmit channel. These values correspond
> +  to the TRANSFER_DIRECTION field of 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-channels = <22>;
> +	dma-rx-requests = <10>;
> +	dma-tx-requests = <10>;
> +	#dma-cells = <2>;
> +};
> -- 
> 2.1.4
> 

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

[Jon Hunter]

On 05/10/15 14:12, Mark Rutland wrote:
> On Mon, Oct 05, 2015 at 01:10:06PM +0100, Jon Hunter wrote:
>> 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      | 63 ++++++++++++++++++++++
>>  1 file changed, 63 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..df0e46868a63
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>> @@ -0,0 +1,63 @@
>> +* 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-channels: Must be 22. Defines the number of DMA channels supported
>> +  by the DMA controller.
> 
> If this has to be a fixed value, why is it necessary? Why does the
> driver not just know this?
> 
> Are there other instances of this IP block where this differs?

So this will change for future devices and yes it may seem silly now to
have something that fixed and appears to be constant but I was trying to
future proof the binding. May be the comment should read "For tegra210
must be 22", however, I thought the compatible string would imply this.

>> +- dma-rx-requests: Must be 10. Defines the number of receive request
>> +  signals supported by the DMA controller.
>> +- dma-tx-requests: Must be 10. Defines the number of transmit request
>> +  signals supported by the DMA controller.
> 
> Likewise for these two?

For these I am not sure if they will change in the future and so yes I
could simply place these in the driver. The register would need to
change in the future for these to change and so that would mean that the
driver would need to be updated. So may be this does not make sense.

Cheers
Jon

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

[Arnd Bergmann]

On Monday 05 October 2015 13:10:07 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>

This version looks much better!

Just one small comment:

> +	slave_req = dma_spec->args[0];
> +	slave_dir = dma_spec->args[1];
> +
> +	if (slave_req == 0)
> +		return NULL;
> +
> +	switch (slave_dir) {
> +	case ADMA_AHUB_TO_MEM:
> +		if (slave_req > tdma->rx_requests)
> +			return NULL;
> +
> +		if (tdma->rx_requests_reserved & BIT(slave_req))
> +			return NULL;
> +
> +		tdma->rx_requests_reserved |= BIT(slave_req);
> +		break;

The rx_requests_reserved logic looks racy, if you have more than
one concurrent request or release. Better use atomic_test_and_set()
here and turn the variable into an atomic_t.

	Arbd

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

[Jon Hunter]

On 06/10/15 10:32, Arnd Bergmann wrote:
> On Monday 05 October 2015 13:10:07 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>
> 
> This version looks much better!
> 
> Just one small comment:
> 
>> +	slave_req = dma_spec->args[0];
>> +	slave_dir = dma_spec->args[1];
>> +
>> +	if (slave_req == 0)
>> +		return NULL;
>> +
>> +	switch (slave_dir) {
>> +	case ADMA_AHUB_TO_MEM:
>> +		if (slave_req > tdma->rx_requests)
>> +			return NULL;
>> +
>> +		if (tdma->rx_requests_reserved & BIT(slave_req))
>> +			return NULL;
>> +
>> +		tdma->rx_requests_reserved |= BIT(slave_req);
>> +		break;
> 
> The rx_requests_reserved logic looks racy, if you have more than
> one concurrent request or release. Better use atomic_test_and_set()
> here and turn the variable into an atomic_t.

Ah yes, will update.

Thanks
Jon

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

[Stephen Warren]

On 10/06/2015 03:16 AM, Jon Hunter wrote:
>
> On 05/10/15 14:12, Mark Rutland wrote:
>> On Mon, Oct 05, 2015 at 01:10:06PM +0100, Jon Hunter wrote:
>>> 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      | 63 ++++++++++++++++++++++
>>>   1 file changed, 63 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..df0e46868a63
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>>> @@ -0,0 +1,63 @@
>>> +* 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-channels: Must be 22. Defines the number of DMA channels supported
>>> +  by the DMA controller.
>>
>> If this has to be a fixed value, why is it necessary? Why does the
>> driver not just know this?
>>
>> Are there other instances of this IP block where this differs?
>
> So this will change for future devices and yes it may seem silly now to
> have something that fixed and appears to be constant but I was trying to
> future proof the binding. May be the comment should read "For tegra210
> must be 22", however, I thought the compatible string would imply this.

Typically you'd want a table in the driver that maps from compatible 
value to the set of per-SoC data that's associated with the compatible 
value. Then, you don't need to put this data into the DT.

Even if you don't want to do that (e.g. so the driver can work on future 
HW without code changes), then you can always add this property in the 
future if needed; if the property is present the value in it will be 
used, but if missing, any driver should default to the value in use 
for/on HW  before the binding update.

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

[Stephen Warren]

On 10/05/2015 06:10 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

> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
> +  receive request number and should be between 1 and the maximum number
> +  of requests supported (see properties "dma-rx-requests" and
> +  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
> +  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
> +  the channel is a receive or transmit channel and must be either 2 for
> +  a receive channel and 4 for a transmit channel. These values correspond
> +  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Is it typical to encode the direction into the dma cells? I would have 
thought the client would provide that information at run-time when 
requesting a DMA channel.

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

[Jon Hunter]

On 07/10/15 00:04, Stephen Warren wrote:
> On 10/05/2015 06:10 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
> 
>> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
>> +  receive request number and should be between 1 and the maximum number
>> +  of requests supported (see properties "dma-rx-requests" and
>> +  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
>> +  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
>> +  the channel is a receive or transmit channel and must be either 2 for
>> +  a receive channel and 4 for a transmit channel. These values
>> correspond
>> +  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
> 
> Is it typical to encode the direction into the dma cells? I would have
> thought the client would provide that information at run-time when
> requesting a DMA channel.

I have seen other dma bindings that do [0]. If we don't put the
direction in the client binding, then it would appear as ...

tegra_admaif: admaif@0x702d0000 {
    ...
    dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
           <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
           <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
           <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
           <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
    dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                "rx10", "tx10";
    ...
};

... where "rxN" and "txN" appear to use the same request, but the
reality is that "rxN" is using rx-request-N and "txN" is using
tx-request-N. Arnd questioned this before. Obviously I can explain this
in the binding document if the above is preferred. However, given that
they are named "rx1", "rx2", etc, why not put the direction in the binding?

By the way, in the ADMA driver, the driver does check that the direction
specified when configuring the transfer (ie. via
tegra_adma_prep_dma_cyclic()) matches the adma channel being used.

Cheers
Jon

[0]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/dma/ste-dma40.txt

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

[Jon Hunter]

On 06/10/15 23:57, Stephen Warren wrote:
> On 10/06/2015 03:16 AM, Jon Hunter wrote:
>>
>> On 05/10/15 14:12, Mark Rutland wrote:
>>> On Mon, Oct 05, 2015 at 01:10:06PM +0100, Jon Hunter wrote:
>>>> 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      | 63
>>>> ++++++++++++++++++++++
>>>>   1 file changed, 63 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..df0e46868a63
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>>>> @@ -0,0 +1,63 @@
>>>> +* 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-channels: Must be 22. Defines the number of DMA channels
>>>> supported
>>>> +  by the DMA controller.
>>>
>>> If this has to be a fixed value, why is it necessary? Why does the
>>> driver not just know this?
>>>
>>> Are there other instances of this IP block where this differs?
>>
>> So this will change for future devices and yes it may seem silly now to
>> have something that fixed and appears to be constant but I was trying to
>> future proof the binding. May be the comment should read "For tegra210
>> must be 22", however, I thought the compatible string would imply this.
> 
> Typically you'd want a table in the driver that maps from compatible
> value to the set of per-SoC data that's associated with the compatible
> value. Then, you don't need to put this data into the DT.

Yes I have seen that which I was not sure that I was a fan of, given
that we have DT and its purpose is to describe the hardware. So may be
the problem I have is deciding on which hardware parameters should be
described in DT versus those that should be place in the driver itself.
I am not sure if there is a rule of thumb for this type of thing?

Cheers
Jon

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

[Stephen Warren]

On 10/07/2015 09:26 AM, Jon Hunter wrote:
>
> On 06/10/15 23:57, Stephen Warren wrote:
>> On 10/06/2015 03:16 AM, Jon Hunter wrote:
>>>
>>> On 05/10/15 14:12, Mark Rutland wrote:
>>>> On Mon, Oct 05, 2015 at 01:10:06PM +0100, Jon Hunter wrote:
>>>>> 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      | 63
>>>>> ++++++++++++++++++++++
>>>>>    1 file changed, 63 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..df0e46868a63
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
>>>>> @@ -0,0 +1,63 @@
>>>>> +* 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-channels: Must be 22. Defines the number of DMA channels
>>>>> supported
>>>>> +  by the DMA controller.
>>>>
>>>> If this has to be a fixed value, why is it necessary? Why does the
>>>> driver not just know this?
>>>>
>>>> Are there other instances of this IP block where this differs?
>>>
>>> So this will change for future devices and yes it may seem silly now to
>>> have something that fixed and appears to be constant but I was trying to
>>> future proof the binding. May be the comment should read "For tegra210
>>> must be 22", however, I thought the compatible string would imply this.
>>
>> Typically you'd want a table in the driver that maps from compatible
>> value to the set of per-SoC data that's associated with the compatible
>> value. Then, you don't need to put this data into the DT.
>
> Yes I have seen that which I was not sure that I was a fan of, given
> that we have DT and its purpose is to describe the hardware. So may be
> the problem I have is deciding on which hardware parameters should be
> described in DT versus those that should be place in the driver itself.
> I am not sure if there is a rule of thumb for this type of thing?

It seems to be a matter of preference.

I think DT is mainly about identifying which HW is present. Facets of 
the HW that are fixed (100% derivable from the compatible value) belong 
in the driver. If we took the opposite view, we should put a description 
of every register/field layout in the DT in case they change on new HW 
and we don't want to edit the driver code to take account of that, and 
in fact should put byte code into the DT since simple data might not be 
enough to allow us not to edit the driver. Facets that vary between 
boards, use-cases, etc. are the main benefit of DT in my opinion, not 
slow-moving data specific to SoCs.

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

[Stephen Warren]

On 10/07/2015 02:43 AM, Jon Hunter wrote:
>
> On 07/10/15 00:04, Stephen Warren wrote:
>> On 10/05/2015 06:10 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
>>
>>> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
>>> +  receive request number and should be between 1 and the maximum number
>>> +  of requests supported (see properties "dma-rx-requests" and
>>> +  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
>>> +  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
>>> +  the channel is a receive or transmit channel and must be either 2 for
>>> +  a receive channel and 4 for a transmit channel. These values
>>> correspond
>>> +  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
>>
>> Is it typical to encode the direction into the dma cells? I would have
>> thought the client would provide that information at run-time when
>> requesting a DMA channel.
>
> I have seen other dma bindings that do [0]. If we don't put the
> direction in the client binding, then it would appear as ...
>
> tegra_admaif: admaif@0x702d0000 {
>      ...
>      dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>             <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>             <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>             <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>             <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>      dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>                  "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>                  "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>                  "rx10", "tx10";
>      ...
> };
>
> ... where "rxN" and "txN" appear to use the same request, but the
> reality is that "rxN" is using rx-request-N and "txN" is using
> tx-request-N. Arnd questioned this before. Obviously I can explain this
> in the binding document if the above is preferred. However, given that
> they are named "rx1", "rx2", etc, why not put the direction in the binding?

Why would we need to duplicate the request IDs? I'd expect to have the 
following property content:

dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
dma-names = "1", "2", "3"...;

*and* not have a cell to represent the direction in DT. After all, the 
direction of the channel is 100% implied by the use-case (and hence 
defined by the DMA client's own DT binding); it is known by the client 
driver and can be supplied at run-time.

Perhaps the core DMA DT bindings are not designed that way though, in 
which case I suppose the patch you sent makes sense. If so though, that 
seems like a bug in the core DMA DT bindings.

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

[Jon Hunter]

On 07/10/15 17:09, Stephen Warren wrote:
> On 10/07/2015 02:43 AM, Jon Hunter wrote:
>>
>> On 07/10/15 00:04, Stephen Warren wrote:
>>> On 10/05/2015 06:10 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
>>>
>>>> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
>>>> +  receive request number and should be between 1 and the maximum
>>>> number
>>>> +  of requests supported (see properties "dma-rx-requests" and
>>>> +  "dma-tx-requests"). This value corresponds to the
>>>> RX/TX_REQUEST_SELECT
>>>> +  fields in the ADMA_CHn_CTRL register. The second cell denotes
>>>> whether
>>>> +  the channel is a receive or transmit channel and must be either 2
>>>> for
>>>> +  a receive channel and 4 for a transmit channel. These values
>>>> correspond
>>>> +  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
>>>
>>> Is it typical to encode the direction into the dma cells? I would have
>>> thought the client would provide that information at run-time when
>>> requesting a DMA channel.
>>
>> I have seen other dma bindings that do [0]. If we don't put the
>> direction in the client binding, then it would appear as ...
>>
>> tegra_admaif: admaif@0x702d0000 {
>>      ...
>>      dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>             <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>             <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>             <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>             <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>      dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>                  "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>                  "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>                  "rx10", "tx10";
>>      ...
>> };
>>
>> ... where "rxN" and "txN" appear to use the same request, but the
>> reality is that "rxN" is using rx-request-N and "txN" is using
>> tx-request-N. Arnd questioned this before. Obviously I can explain this
>> in the binding document if the above is preferred. However, given that
>> they are named "rx1", "rx2", etc, why not put the direction in the
>> binding?
> 
> Why would we need to duplicate the request IDs? I'd expect to have the
> following property content:
> 
> dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
> dma-names = "1", "2", "3"...;
> 
> *and* not have a cell to represent the direction in DT. After all, the
> direction of the channel is 100% implied by the use-case (and hence
> defined by the DMA client's own DT binding); it is known by the client
> driver and can be supplied at run-time.

Right, but what does the 1, 2, 3, etc in the specifier represent? If it
is the request signal then I don't see how this would work because there
are 10 rx request signals and 10 tx requests signal and both are 1-10.
If you look at the ADMA_CH<n>_CTRL_0 register you will see there are a
fields for the TX_REQUEST_SELECT, RX_REQUEST_SELECT and
TRANSFER_DIRECTION. It seems a bit silly to have both TX_REQUEST_SELECT
and RX_REQUEST_SELECT as the channel can only work with one direction at
any given time.

> Perhaps the core DMA DT bindings are not designed that way though, in
> which case I suppose the patch you sent makes sense. If so though, that
> seems like a bug in the core DMA DT bindings.

I think it is more of a nuance in how this DMA controller is configured.

Jon

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

[Mark Rutland]

[...]

> >>>>>+- dma-channels: Must be 22. Defines the number of DMA channels
> >>>>>supported
> >>>>>+  by the DMA controller.
> >>>>
> >>>>If this has to be a fixed value, why is it necessary? Why does the
> >>>>driver not just know this?
> >>>>
> >>>>Are there other instances of this IP block where this differs?
> >>>
> >>>So this will change for future devices and yes it may seem silly now to
> >>>have something that fixed and appears to be constant but I was trying to
> >>>future proof the binding. May be the comment should read "For tegra210
> >>>must be 22", however, I thought the compatible string would imply this.
> >>
> >>Typically you'd want a table in the driver that maps from compatible
> >>value to the set of per-SoC data that's associated with the compatible
> >>value. Then, you don't need to put this data into the DT.
> >
> >Yes I have seen that which I was not sure that I was a fan of, given
> >that we have DT and its purpose is to describe the hardware. So may be
> >the problem I have is deciding on which hardware parameters should be
> >described in DT versus those that should be place in the driver itself.
> >I am not sure if there is a rule of thumb for this type of thing?
> 
> It seems to be a matter of preference.
> 
> I think DT is mainly about identifying which HW is present. Facets
> of the HW that are fixed (100% derivable from the compatible value)
> belong in the driver. If we took the opposite view, we should put a
> description of every register/field layout in the DT in case they
> change on new HW and we don't want to edit the driver code to take
> account of that, and in fact should put byte code into the DT since
> simple data might not be enough to allow us not to edit the driver.
> Facets that vary between boards, use-cases, etc. are the main
> benefit of DT in my opinion, not slow-moving data specific to SoCs.

I think it depends on what the difference is as opposed to how quickly
it changes.

If there's some known-configurable but non-probeable property of an IP
block, then describing that explicitly makes sense if you can predict
what the other configurations will look like in terms of the programming
interface (e.g. the register layout). That gives you a degree of forward
compatibility, with less churn and a smaller driver.

If the register layout changes in a non-predictable fashion, you cannot
capture that in a binding a need more code. There will always be
elements which are non-predictable and need changes, but that need not
be the default case.

Mark.

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

[Mark Rutland]

On Wed, Oct 07, 2015 at 09:43:45AM +0100, Jon Hunter wrote:
> 
> On 07/10/15 00:04, Stephen Warren wrote:
> > On 10/05/2015 06:10 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
> > 
> >> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
> >> +  receive request number and should be between 1 and the maximum number
> >> +  of requests supported (see properties "dma-rx-requests" and
> >> +  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
> >> +  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
> >> +  the channel is a receive or transmit channel and must be either 2 for
> >> +  a receive channel and 4 for a transmit channel. These values
> >> correspond
> >> +  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
> > 
> > Is it typical to encode the direction into the dma cells? I would have
> > thought the client would provide that information at run-time when
> > requesting a DMA channel.
> 
> I have seen other dma bindings that do [0]. If we don't put the
> direction in the client binding, then it would appear as ...
> 
> tegra_admaif: admaif@0x702d0000 {
>     ...
>     dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>            <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>            <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>            <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>            <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>     dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>                 "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>                 "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>                 "rx10", "tx10";
>     ...
> };
> 
> ... where "rxN" and "txN" appear to use the same request, but the
> reality is that "rxN" is using rx-request-N and "txN" is using
> tx-request-N. Arnd questioned this before. Obviously I can explain this
> in the binding document if the above is preferred. However, given that
> they are named "rx1", "rx2", etc, why not put the direction in the binding?

The client shouldn't know anything about the format of the specifier. If
the client expects separate rx and tx DMA channels, it should describe
these separately with names as above, regardless of whether the DMA
controller requires TX and RX channels to be described differently.

I don't know if it make sense to also verify this from the DMA
controller's PoV.

Mark.

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

[Stephen Warren]

On 10/07/2015 10:19 AM, Jon Hunter wrote:
>
> On 07/10/15 17:09, Stephen Warren wrote:
>> On 10/07/2015 02:43 AM, Jon Hunter wrote:
>>>
>>> On 07/10/15 00:04, Stephen Warren wrote:
>>>> On 10/05/2015 06:10 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
>>>>
>>>>> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
>>>>> +  receive request number and should be between 1 and the maximum
>>>>> number
>>>>> +  of requests supported (see properties "dma-rx-requests" and
>>>>> +  "dma-tx-requests"). This value corresponds to the
>>>>> RX/TX_REQUEST_SELECT
>>>>> +  fields in the ADMA_CHn_CTRL register. The second cell denotes
>>>>> whether
>>>>> +  the channel is a receive or transmit channel and must be either 2
>>>>> for
>>>>> +  a receive channel and 4 for a transmit channel. These values
>>>>> correspond
>>>>> +  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
>>>>
>>>> Is it typical to encode the direction into the dma cells? I would have
>>>> thought the client would provide that information at run-time when
>>>> requesting a DMA channel.
>>>
>>> I have seen other dma bindings that do [0]. If we don't put the
>>> direction in the client binding, then it would appear as ...
>>>
>>> tegra_admaif: admaif@0x702d0000 {
>>>       ...
>>>       dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>>              <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>>              <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>>              <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>>              <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>>       dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>>                   "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>>                   "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>>                   "rx10", "tx10";
>>>       ...
>>> };
>>>
>>> ... where "rxN" and "txN" appear to use the same request, but the
>>> reality is that "rxN" is using rx-request-N and "txN" is using
>>> tx-request-N. Arnd questioned this before. Obviously I can explain this
>>> in the binding document if the above is preferred. However, given that
>>> they are named "rx1", "rx2", etc, why not put the direction in the
>>> binding?
>>
>> Why would we need to duplicate the request IDs? I'd expect to have the
>> following property content:
>>
>> dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
>> dma-names = "1", "2", "3"...;
>>
>> *and* not have a cell to represent the direction in DT. After all, the
>> direction of the channel is 100% implied by the use-case (and hence
>> defined by the DMA client's own DT binding); it is known by the client
>> driver and can be supplied at run-time.
>
> Right, but what does the 1, 2, 3, etc in the specifier represent?

Aren't they the ADMAIF FIFO IDs?

We know the set/number of ADMAIF FIFOs, and each FIFO needs a request 
selector ID. The list of those can be indexed by the identity of the 
FIFO that is accessed via DMA.

Thinking about this more, I think actually that the dmas/dma-names 
property example that you posted above is exactly what is required here. 
The counter-example I wrote makes this assumption and hence is invalid. 
The ADMAIF binding should not assume that the RX and TX request selector 
IDs are identical. As such, dmas/dma-names should have both an RX and TX 
entry for each ADMAIF FIFO.

Still, there's no need to encode the DMA direction into the #dma-cells. 
The client code will know that if it wants to configure DMA into (TX to) 
FIFO ID 5, it must query dma-names entry "tx5", and simply use whatever 
is in the DT. When it passes that DMA specifier to the DMA API, the 
ADMAIF driver knows that it will be for TX, and can pass that 
information to the DMA code.

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

[Jon Hunter]

On 07/10/15 20:36, Stephen Warren wrote:
> On 10/07/2015 10:19 AM, Jon Hunter wrote:
>>
>> On 07/10/15 17:09, Stephen Warren wrote:
>>> On 10/07/2015 02:43 AM, Jon Hunter wrote:
>>>>
>>>> On 07/10/15 00:04, Stephen Warren wrote:
>>>>> On 10/05/2015 06:10 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
>>>>>
>>>>>> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
>>>>>> +  receive request number and should be between 1 and the maximum
>>>>>> number
>>>>>> +  of requests supported (see properties "dma-rx-requests" and
>>>>>> +  "dma-tx-requests"). This value corresponds to the
>>>>>> RX/TX_REQUEST_SELECT
>>>>>> +  fields in the ADMA_CHn_CTRL register. The second cell denotes
>>>>>> whether
>>>>>> +  the channel is a receive or transmit channel and must be either 2
>>>>>> for
>>>>>> +  a receive channel and 4 for a transmit channel. These values
>>>>>> correspond
>>>>>> +  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
>>>>>
>>>>> Is it typical to encode the direction into the dma cells? I would have
>>>>> thought the client would provide that information at run-time when
>>>>> requesting a DMA channel.
>>>>
>>>> I have seen other dma bindings that do [0]. If we don't put the
>>>> direction in the client binding, then it would appear as ...
>>>>
>>>> tegra_admaif: admaif@0x702d0000 {
>>>>       ...
>>>>       dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>>>              <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>>>              <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>>>              <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>>>              <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>>>       dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>>>                   "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>>>                   "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>>>                   "rx10", "tx10";
>>>>       ...
>>>> };
>>>>
>>>> ... where "rxN" and "txN" appear to use the same request, but the
>>>> reality is that "rxN" is using rx-request-N and "txN" is using
>>>> tx-request-N. Arnd questioned this before. Obviously I can explain this
>>>> in the binding document if the above is preferred. However, given that
>>>> they are named "rx1", "rx2", etc, why not put the direction in the
>>>> binding?
>>>
>>> Why would we need to duplicate the request IDs? I'd expect to have the
>>> following property content:
>>>
>>> dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
>>> dma-names = "1", "2", "3"...;
>>>
>>> *and* not have a cell to represent the direction in DT. After all, the
>>> direction of the channel is 100% implied by the use-case (and hence
>>> defined by the DMA client's own DT binding); it is known by the client
>>> driver and can be supplied at run-time.
>>
>> Right, but what does the 1, 2, 3, etc in the specifier represent?
> 
> Aren't they the ADMAIF FIFO IDs?

Yes.

> We know the set/number of ADMAIF FIFOs, and each FIFO needs a request
> selector ID. The list of those can be indexed by the identity of the
> FIFO that is accessed via DMA.

Right, but you have 10 RX FIFOs and 10 TX FIFOs. The FIFOs are
unidirectional. This means that instead of having 20 FIFOs from 1-20
(yes the FIFOs start from 1 and not 0), you have RX FIFOs from 1-10 and
TX FIFOs from 1-10.

> Thinking about this more, I think actually that the dmas/dma-names
> property example that you posted above is exactly what is required here.

Ok, good.

> The counter-example I wrote makes this assumption and hence is invalid.
> The ADMAIF binding should not assume that the RX and TX request selector
> IDs are identical. As such, dmas/dma-names should have both an RX and TX
> entry for each ADMAIF FIFO.

Yes.

> Still, there's no need to encode the DMA direction into the #dma-cells.
> The client code will know that if it wants to configure DMA into (TX to)
> FIFO ID 5, it must query dma-names entry "tx5", and simply use whatever
> is in the DT. When it passes that DMA specifier to the DMA API, the
> ADMAIF driver knows that it will be for TX, and can pass that
> information to the DMA code.

That's fine. From my perspective I don't have a strong objection either
way, however, I can see that given that the name indicates rx or tx,
then the direction in the binding could be seen as redundant.

So to confirm you are happy with the client bindings being as follows?

tegra_admaif: admaif@0x702d0000 {
      ...
     dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
            <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
            <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
            <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
            <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
     dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                 "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                 "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                 "rx10", "tx10";
     ...
};

Jon

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

[Stephen Warren]

On 10/08/2015 03:58 AM, Jon Hunter wrote:
>
> On 07/10/15 20:36, Stephen Warren wrote:
>> On 10/07/2015 10:19 AM, Jon Hunter wrote:
>>>
>>> On 07/10/15 17:09, Stephen Warren wrote:
>>>> On 10/07/2015 02:43 AM, Jon Hunter wrote:
>>>>>
>>>>> On 07/10/15 00:04, Stephen Warren wrote:
>>>>>> On 10/05/2015 06:10 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
>>>>>>
>>>>>>> +- #dma-cells : Must be <2>. The first cell denotes the transmit or
>>>>>>> +  receive request number and should be between 1 and the maximum
>>>>>>> number
>>>>>>> +  of requests supported (see properties "dma-rx-requests" and
>>>>>>> +  "dma-tx-requests"). This value corresponds to the
>>>>>>> RX/TX_REQUEST_SELECT
>>>>>>> +  fields in the ADMA_CHn_CTRL register. The second cell denotes
>>>>>>> whether
>>>>>>> +  the channel is a receive or transmit channel and must be either 2
>>>>>>> for
>>>>>>> +  a receive channel and 4 for a transmit channel. These values
>>>>>>> correspond
>>>>>>> +  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
>>>>>>
>>>>>> Is it typical to encode the direction into the dma cells? I would have
>>>>>> thought the client would provide that information at run-time when
>>>>>> requesting a DMA channel.
>>>>>
>>>>> I have seen other dma bindings that do [0]. If we don't put the
>>>>> direction in the client binding, then it would appear as ...
>>>>>
>>>>> tegra_admaif: admaif@0x702d0000 {
>>>>>        ...
>>>>>        dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>>>>               <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>>>>               <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>>>>               <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>>>>               <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>>>>        dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>>>>                    "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>>>>                    "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>>>>                    "rx10", "tx10";
>>>>>        ...
>>>>> };
>>>>>
>>>>> ... where "rxN" and "txN" appear to use the same request, but the
>>>>> reality is that "rxN" is using rx-request-N and "txN" is using
>>>>> tx-request-N. Arnd questioned this before. Obviously I can explain this
>>>>> in the binding document if the above is preferred. However, given that
>>>>> they are named "rx1", "rx2", etc, why not put the direction in the
>>>>> binding?
>>>>
>>>> Why would we need to duplicate the request IDs? I'd expect to have the
>>>> following property content:
>>>>
>>>> dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
>>>> dma-names = "1", "2", "3"...;
>>>>
>>>> *and* not have a cell to represent the direction in DT. After all, the
>>>> direction of the channel is 100% implied by the use-case (and hence
>>>> defined by the DMA client's own DT binding); it is known by the client
>>>> driver and can be supplied at run-time.
>>>
>>> Right, but what does the 1, 2, 3, etc in the specifier represent?
>>
>> Aren't they the ADMAIF FIFO IDs?
>
> Yes.
>
>> We know the set/number of ADMAIF FIFOs, and each FIFO needs a request
>> selector ID. The list of those can be indexed by the identity of the
>> FIFO that is accessed via DMA.
>
> Right, but you have 10 RX FIFOs and 10 TX FIFOs. The FIFOs are
> unidirectional. This means that instead of having 20 FIFOs from 1-20
> (yes the FIFOs start from 1 and not 0), you have RX FIFOs from 1-10 and
> TX FIFOs from 1-10.
>
>> Thinking about this more, I think actually that the dmas/dma-names
>> property example that you posted above is exactly what is required here.
>
> Ok, good.
>
>> The counter-example I wrote makes this assumption and hence is invalid.
>> The ADMAIF binding should not assume that the RX and TX request selector
>> IDs are identical. As such, dmas/dma-names should have both an RX and TX
>> entry for each ADMAIF FIFO.
>
> Yes.
>
>> Still, there's no need to encode the DMA direction into the #dma-cells.
>> The client code will know that if it wants to configure DMA into (TX to)
>> FIFO ID 5, it must query dma-names entry "tx5", and simply use whatever
>> is in the DT. When it passes that DMA specifier to the DMA API, the
>> ADMAIF driver knows that it will be for TX, and can pass that
>> information to the DMA code.
>
> That's fine. From my perspective I don't have a strong objection either
> way, however, I can see that given that the name indicates rx or tx,
> then the direction in the binding could be seen as redundant.
>
> So to confirm you are happy with the client bindings being as follows?
>
> tegra_admaif: admaif@0x702d0000 {
>        ...
>       dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>              <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>              <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>              <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>              <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>       dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>                   "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>                   "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>                   "rx10", "tx10";
>       ...
> };

Yes, that looks good for the client binding.

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

[Jon Hunter]

On 08/10/15 15:27, Stephen Warren wrote:
> On 10/08/2015 03:58 AM, Jon Hunter wrote:

[snip]

>> That's fine. From my perspective I don't have a strong objection either
>> way, however, I can see that given that the name indicates rx or tx,
>> then the direction in the binding could be seen as redundant.
>>
>> So to confirm you are happy with the client bindings being as follows?
>>
>> tegra_admaif: admaif@0x702d0000 {
>>        ...
>>       dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>              <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>              <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>              <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>              <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>       dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>                   "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>                   "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>                   "rx10", "tx10";
>>       ...
>> };
> 
> Yes, that looks good for the client binding.

One more clarifying question ... should the xlate verify that no other
dma channel is using the same hardware request signal?

I understand that typically the xlate decodes the binding to get the
channel info, but because this is invoked by dmaengine while allocating
a channel, I was wondering if we should prevent dmaengine allocating
more than one channel to be used with the same hardware request? If so,
then passing the direction to the xlate would be necessary (so I can
determine in the xlate that no one else is currently using this, which
is what I currently do).

Alternatively, I could check that no one else is using the request
signal at a later when the transfer is being prepared.

If you are wondering why I am worried about this, I my mind I think that
the driver should be robust enough to check for conflicts in the request
signals used by the various channels.

Jon

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

[Stephen Warren]

On 10/09/2015 04:20 AM, Jon Hunter wrote:
>
> On 08/10/15 15:27, Stephen Warren wrote:
>> On 10/08/2015 03:58 AM, Jon Hunter wrote:
>
> [snip]
>
>>> That's fine. From my perspective I don't have a strong objection either
>>> way, however, I can see that given that the name indicates rx or tx,
>>> then the direction in the binding could be seen as redundant.
>>>
>>> So to confirm you are happy with the client bindings being as follows?
>>>
>>> tegra_admaif: admaif@0x702d0000 {
>>>         ...
>>>        dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>>               <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>>               <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>>               <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>>               <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>>        dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>>                    "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>>                    "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>>                    "rx10", "tx10";
>>>        ...
>>> };
>>
>> Yes, that looks good for the client binding.
>
> One more clarifying question ... should the xlate verify that no other
> dma channel is using the same hardware request signal?
>
> I understand that typically the xlate decodes the binding to get the
> channel info, but because this is invoked by dmaengine while allocating
> a channel, I was wondering if we should prevent dmaengine allocating
> more than one channel to be used with the same hardware request? If so,
> then passing the direction to the xlate would be necessary (so I can
> determine in the xlate that no one else is currently using this, which
> is what I currently do).
>
> Alternatively, I could check that no one else is using the request
> signal at a later when the transfer is being prepared.

I think that handling this at prepare/usage time is probably most 
appropriate. That is the time when the resource conflict /actually/ occurs.

The only time when two clients would be given the same DMA request 
signal is if there are multiple different drivers that can DMA into the 
same FIFO in a time-multiplexed fashion. That seems pretty unlikely off 
the top of my head, but I don't think we want to actively ban that, in 
case we come up with a cunning use-case for it.

> If you are wondering why I am worried about this, I my mind I think that
> the driver should be robust enough to check for conflicts in the request
> signals used by the various channels.

Sure, makes sense.

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

[Jon Hunter]

On 09/10/15 16:26, Stephen Warren wrote:
> On 10/09/2015 04:20 AM, Jon Hunter wrote:
>>
>> On 08/10/15 15:27, Stephen Warren wrote:
>>> On 10/08/2015 03:58 AM, Jon Hunter wrote:
>>
>> [snip]
>>
>>>> That's fine. From my perspective I don't have a strong objection either
>>>> way, however, I can see that given that the name indicates rx or tx,
>>>> then the direction in the binding could be seen as redundant.
>>>>
>>>> So to confirm you are happy with the client bindings being as follows?
>>>>
>>>> tegra_admaif: admaif@0x702d0000 {
>>>>         ...
>>>>        dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>>>               <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>>>               <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>>>               <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>>>               <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>>>        dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>>>                    "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>>>                    "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>>>                    "rx10", "tx10";
>>>>        ...
>>>> };
>>>
>>> Yes, that looks good for the client binding.
>>
>> One more clarifying question ... should the xlate verify that no other
>> dma channel is using the same hardware request signal?
>>
>> I understand that typically the xlate decodes the binding to get the
>> channel info, but because this is invoked by dmaengine while allocating
>> a channel, I was wondering if we should prevent dmaengine allocating
>> more than one channel to be used with the same hardware request? If so,
>> then passing the direction to the xlate would be necessary (so I can
>> determine in the xlate that no one else is currently using this, which
>> is what I currently do).
>>
>> Alternatively, I could check that no one else is using the request
>> signal at a later when the transfer is being prepared.
> 
> I think that handling this at prepare/usage time is probably most
> appropriate. That is the time when the resource conflict /actually/ occurs.

Although that makes sense, the more I look at this, the more I think it
should be handled during the channel allocate/free phases as it makes
sense to allocate the required resources then. It is probably simpler
and safer too.

> The only time when two clients would be given the same DMA request
> signal is if there are multiple different drivers that can DMA into the
> same FIFO in a time-multiplexed fashion. That seems pretty unlikely off
> the top of my head, but I don't think we want to actively ban that, in
> case we come up with a cunning use-case for it.

I know this is purely an example, but if such a time-multiplexed scheme
was a real use-case, then it would seem more likely to have a shim layer
between the clients that talked to the dmaengine and hence, it would
still only be necessary for one client to interface to a given channel.

What I don't like about the above binding is that someone can request
the dma channel "tx5" and then call dmaengine_prep_dma_cyclic() and say
you know what, I am gonna receive data instead. That seems odd and I
think that such a scenario should be greeted with an error code of
-EINVAL. It seems to me that if channels are uni-directional (in the
sense you either use it for tx or rx), you should request the
appropriate channel for the direction you want and then set the
direction in dmaengine_prep_dma_cyclic() so that it matches and if it
does not then we return an error.

So I still like the idea of the direction of the request being in the
binding so we know what the client intends (sorry to keep changing my
mind). Do you completely deplore the idea?

Jon

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

[Stephen Warren]

On 10/12/2015 07:55 AM, Jon Hunter wrote:
>
> On 09/10/15 16:26, Stephen Warren wrote:
>> On 10/09/2015 04:20 AM, Jon Hunter wrote:
>>>
>>> On 08/10/15 15:27, Stephen Warren wrote:
>>>> On 10/08/2015 03:58 AM, Jon Hunter wrote:
>>>
>>> [snip]
>>>
>>>>> That's fine. From my perspective I don't have a strong objection either
>>>>> way, however, I can see that given that the name indicates rx or tx,
>>>>> then the direction in the binding could be seen as redundant.
>>>>>
>>>>> So to confirm you are happy with the client bindings being as follows?
>>>>>
>>>>> tegra_admaif: admaif@0x702d0000 {
>>>>>          ...
>>>>>         dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>>>>                <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>>>>                <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>>>>                <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>>>>                <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>>>>         dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>>>>                     "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>>>>                     "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>>>>                     "rx10", "tx10";
>>>>>         ...
>>>>> };
>>>>
>>>> Yes, that looks good for the client binding.
>>>
>>> One more clarifying question ... should the xlate verify that no other
>>> dma channel is using the same hardware request signal?
>>>
>>> I understand that typically the xlate decodes the binding to get the
>>> channel info, but because this is invoked by dmaengine while allocating
>>> a channel, I was wondering if we should prevent dmaengine allocating
>>> more than one channel to be used with the same hardware request? If so,
>>> then passing the direction to the xlate would be necessary (so I can
>>> determine in the xlate that no one else is currently using this, which
>>> is what I currently do).
>>>
>>> Alternatively, I could check that no one else is using the request
>>> signal at a later when the transfer is being prepared.
>>
>> I think that handling this at prepare/usage time is probably most
>> appropriate. That is the time when the resource conflict /actually/ occurs.
>
> Although that makes sense, the more I look at this, the more I think it
> should be handled during the channel allocate/free phases as it makes
> sense to allocate the required resources then. It is probably simpler
> and safer too.

I fail to see how it's simpler or safer. Everything is still 100% safe 
if the checks are handled when the channel is actually used. I think 
it's simpler too, since there's less to worry about in DT, and less 
state to carry around in the code.

>> The only time when two clients would be given the same DMA request
>> signal is if there are multiple different drivers that can DMA into the
>> same FIFO in a time-multiplexed fashion. That seems pretty unlikely off
>> the top of my head, but I don't think we want to actively ban that, in
>> case we come up with a cunning use-case for it.
>
> I know this is purely an example, but if such a time-multiplexed scheme
> was a real use-case, then it would seem more likely to have a shim layer
> between the clients that talked to the dmaengine and hence, it would
> still only be necessary for one client to interface to a given channel.

I don't agree at all. There's no reason why clients shouldn't simply go 
to the dmaengine code and request to use channels when they need them. 
Why would a shim layer be needed for that?

> What I don't like about the above binding is that someone can request
> the dma channel "tx5" and then call dmaengine_prep_dma_cyclic() and say
> you know what, I am gonna receive data instead.

It's always possible to write bugs. The DMA binding can't fix that.

> That seems odd and I
> think that such a scenario should be greeted with an error code of
> -EINVAL. It seems to me that if channels are uni-directional (in the
> sense you either use it for tx or rx), you should request the
> appropriate channel for the direction you want and then set the
> direction in dmaengine_prep_dma_cyclic() so that it matches and if it
> does not then we return an error.

Channels (in HW) are uni-directional for a particular transfer, but can 
operate in any arbitrary combination of directions for different transfers.

Do note that the name "tx5" is something 100% isolated to the client of 
the DMA channel and meaningless for the DMA controller itself. This is 
simply a name that the client uses to look up data that it must pass to 
the DMA controller.

> So I still like the idea of the direction of the request being in the
> binding so we know what the client intends (sorry to keep changing my
> mind). Do you completely deplore the idea?

I still believe it's wrong yes.

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

[Jon Hunter]

On 12/10/15 18:51, Stephen Warren wrote:
> On 10/12/2015 07:55 AM, Jon Hunter wrote:
>>
>> On 09/10/15 16:26, Stephen Warren wrote:
>>> On 10/09/2015 04:20 AM, Jon Hunter wrote:
>>>>
>>>> On 08/10/15 15:27, Stephen Warren wrote:
>>>>> On 10/08/2015 03:58 AM, Jon Hunter wrote:
>>>>
>>>> [snip]
>>>>
>>>>>> That's fine. From my perspective I don't have a strong objection
>>>>>> either
>>>>>> way, however, I can see that given that the name indicates rx or tx,
>>>>>> then the direction in the binding could be seen as redundant.
>>>>>>
>>>>>> So to confirm you are happy with the client bindings being as
>>>>>> follows?
>>>>>>
>>>>>> tegra_admaif: admaif@0x702d0000 {
>>>>>>          ...
>>>>>>         dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>>>>>                <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>>>>>                <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>>>>>                <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>>>>>                <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>>>>>         dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>>>>>                     "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>>>>>                     "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>>>>>                     "rx10", "tx10";
>>>>>>         ...
>>>>>> };
>>>>>
>>>>> Yes, that looks good for the client binding.
>>>>
>>>> One more clarifying question ... should the xlate verify that no other
>>>> dma channel is using the same hardware request signal?
>>>>
>>>> I understand that typically the xlate decodes the binding to get the
>>>> channel info, but because this is invoked by dmaengine while allocating
>>>> a channel, I was wondering if we should prevent dmaengine allocating
>>>> more than one channel to be used with the same hardware request? If so,
>>>> then passing the direction to the xlate would be necessary (so I can
>>>> determine in the xlate that no one else is currently using this, which
>>>> is what I currently do).
>>>>
>>>> Alternatively, I could check that no one else is using the request
>>>> signal at a later when the transfer is being prepared.
>>>
>>> I think that handling this at prepare/usage time is probably most
>>> appropriate. That is the time when the resource conflict /actually/
>>> occurs.
>>
>> Although that makes sense, the more I look at this, the more I think it
>> should be handled during the channel allocate/free phases as it makes
>> sense to allocate the required resources then. It is probably simpler
>> and safer too.
> 
> I fail to see how it's simpler or safer. Everything is still 100% safe
> if the checks are handled when the channel is actually used. I think
> it's simpler too, since there's less to worry about in DT, and less
> state to carry around in the code.

Safer/simpler in the sense that there are less potential paths to deal
with (ie. completion paths). That's all.

>>> The only time when two clients would be given the same DMA request
>>> signal is if there are multiple different drivers that can DMA into the
>>> same FIFO in a time-multiplexed fashion. That seems pretty unlikely off
>>> the top of my head, but I don't think we want to actively ban that, in
>>> case we come up with a cunning use-case for it.
>>
>> I know this is purely an example, but if such a time-multiplexed scheme
>> was a real use-case, then it would seem more likely to have a shim layer
>> between the clients that talked to the dmaengine and hence, it would
>> still only be necessary for one client to interface to a given channel.
> 
> I don't agree at all. There's no reason why clients shouldn't simply go
> to the dmaengine code and request to use channels when they need them.
> Why would a shim layer be needed for that?

I was just thinking about the granularity of time-multiplexing and
overhead of swapping the sync back and forth versus formatting the
buffer prior to submitting to dmaengine.

>> What I don't like about the above binding is that someone can request
>> the dma channel "tx5" and then call dmaengine_prep_dma_cyclic() and say
>> you know what, I am gonna receive data instead.
> 
> It's always possible to write bugs. The DMA binding can't fix that.

Shame ;-)

>> That seems odd and I
>> think that such a scenario should be greeted with an error code of
>> -EINVAL. It seems to me that if channels are uni-directional (in the
>> sense you either use it for tx or rx), you should request the
>> appropriate channel for the direction you want and then set the
>> direction in dmaengine_prep_dma_cyclic() so that it matches and if it
>> does not then we return an error.
> 
> Channels (in HW) are uni-directional for a particular transfer, but can
> operate in any arbitrary combination of directions for different transfers.
> 
> Do note that the name "tx5" is something 100% isolated to the client of
> the DMA channel and meaningless for the DMA controller itself. This is
> simply a name that the client uses to look up data that it must pass to
> the DMA controller.

Yes.

>> So I still like the idea of the direction of the request being in the
>> binding so we know what the client intends (sorry to keep changing my
>> mind). Do you completely deplore the idea?
> 
> I still believe it's wrong yes.

Won't be my first time ;-)

I was trying to describe the sync and it seems for this DMA, to describe
each hardware sync you would need the index and direction. So putting
both in the binding does not seem entirely bonkers. However, I concede
that in this case the direction is somewhat negligible because as long
as the driver can handle any conflict between clients (w.r.t syncs), the
client just needs the index of the appropriate dma interface.

Jon

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

[Vinod Koul]

On Mon, Oct 05, 2015 at 01:10:07PM +0100, Jon Hunter wrote:

> +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) {
> +		spin_unlock_irqrestore(&tdc->lock, flags);
> +		return ret;
> +	}

txstate can be NULL, upon which below code doesn't help in getting executed
so bailing here or above case is fine

> +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->config_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_ATOMIC);

we recommend GFP_NOWAIT for the subsystem

> +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];
> +		tasklet_kill(&tdc->vc.task);
> +	}
this is good, but your irq is still active and can fire and thus trigger
more tasklets!

-- 
~Vinod

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

[Jon Hunter]

On 14/10/15 12:27, Vinod Koul wrote:
> On Mon, Oct 05, 2015 at 01:10:07PM +0100, Jon Hunter wrote:
> 
>> +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) {
>> +		spin_unlock_irqrestore(&tdc->lock, flags);
>> +		return ret;
>> +	}
> 
> txstate can be NULL, upon which below code doesn't help in getting executed
> so bailing here or above case is fine

Ok, will fix.

>> +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->config_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_ATOMIC);
> 
> we recommend GFP_NOWAIT for the subsystem

Ok, makes sense. I should probably fix up the tegra APB dma driver too.

>> +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];
>> +		tasklet_kill(&tdc->vc.task);
>> +	}
> this is good, but your irq is still active and can fire and thus trigger
> more tasklets!

Ah, yes. Will fix.

Thanks!
Jon