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

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

[Mark Rutland]

On Fri, Sep 25, 2015 at 03:56:39PM +0100, Jon Hunter wrote:
> Add device-tree binding documentation for the Tegra210 Audio DMA
> controller.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/dma/tegra210-adma.txt      | 47 ++++++++++++++++++++++
>  1 file changed, 47 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..af04b3c5a557
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
> @@ -0,0 +1,47 @@
> +* NVIDIA Tegra Audio DMA controller
> +
> +Required properties:
> +- compatible: Should be "nvidia,<chip>-adma"

Where <chip> can be?

> +- reg: Should contain DMA registers location and length. This should include
> +  all of the per-channel registers.

This is one contiguous bank?

> +- interrupt-parent: Phandle to the interrupt parent controller.
> +- interrupts: Should contain all of the per-channel DMA interrupts.

In which particular order?

> +- clocks: Must contain two entries, one for the power-domain clock and one
> +  for the module clock. See ../clocks/clock-bindings.txt for details.

The example dts and driver rely on clock-names.

Please define the set of clock-names, and define clocks in terms of
clock-names.

> +- dma-channels: Number of DMA channels supported by the controller.
> +- #dma-cells : Must be <0>.

As others have pointed out, this doesn't seem right.

Thanks,
Mark.

[PATCH 0/3] 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.

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

Jon Hunter (3):
  dmaengine: of: Allow #dma-cells to be zero
  Documentation: DT: Add binding documentation for NVIDIA ADMA
  dmaengine: tegra-adma: Add support for Tegra210 ADMA

 Documentation/devicetree/bindings/dma/dma.txt      |  12 +-
 .../devicetree/bindings/dma/tegra210-adma.txt      |  47 ++
 drivers/dma/Kconfig                                |  13 +
 drivers/dma/Makefile                               |   1 +
 drivers/dma/of-dma.c                               |  23 +-
 drivers/dma/tegra210-adma.c                        | 820 +++++++++++++++++++++
 6 files changed, 904 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
 create mode 100644 drivers/dma/tegra210-adma.c

-- 
2.1.4

[PATCH 1/3] dmaengine: of: Allow #dma-cells to be zero

[Jon Hunter]

Currently, the #dma-cells property must be 1 or more. However, for some
DMA controllers, where DMA clients may use any DMA channel and it is not
necessary to specify any other DMA information in the device-tree DMA
binding, setting #dma-cells to 0 is desirable. The Tegra210 ADMA
controller is an example of a DMA controller where neither the DMA
channel or the hardware request signal need to be encoded in the
device-tree binding.

By allowing DMA controllers to set #dma-cells to 0, means that the
"dma-names" property for these controllers is not needed. Therefore,
update the of_dma_request_slave_channel() and of_dma_match_channel()
functions to ignore this property if no name is provided.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/dma/dma.txt | 12 ++++++++----
 drivers/dma/of-dma.c                          | 23 +++++++++++++++--------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt
index 6312fb00ce8d..4d73916fc879 100644
--- a/Documentation/devicetree/bindings/dma/dma.txt
+++ b/Documentation/devicetree/bindings/dma/dma.txt
@@ -8,9 +8,12 @@ controller.
 * DMA controller
 
 Required property:
-- #dma-cells: 		Must be at least 1. Used to provide DMA controller
-			specific information. See DMA client binding below for
-			more details.
+- #dma-cells: 		Used to provide DMA controller specific information.
+			Can be 0 for DMA controllers where clients may use
+			any channel and any DMA specific information, such
+			as a hardware request signal, does not need to be
+			encoded in the binding. See DMA client binding below
+			for more details.
 
 Optional properties:
 - dma-channels: 	Number of DMA channels supported by the controller.
@@ -79,7 +82,8 @@ Required property:
 			are defined in the binding of the DMA client device.
 			Multiple DMA specifiers can be used to represent
 			alternatives and in this case the dma-names for those
-			DMA specifiers must be identical (see examples).
+			DMA specifiers must be identical (see examples). This
+			is optional for DMA controllers where #dma-cells is 0.
 
 Examples:
 
diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 1e1f2986eba8..ef3d260610ea 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -214,11 +214,13 @@ static int of_dma_match_channel(struct device_node *np, const char *name,
 {
 	const char *s;
 
-	if (of_property_read_string_index(np, "dma-names", index, &s))
-		return -ENODEV;
+	if (name) {
+		if (of_property_read_string_index(np, "dma-names", index, &s))
+			return -ENODEV;
 
-	if (strcmp(name, s))
-		return -ENODEV;
+		if (strcmp(name, s))
+			return -ENODEV;
+	}
 
 	if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index,
 				       dma_spec))
@@ -230,7 +232,7 @@ static int of_dma_match_channel(struct device_node *np, const char *name,
 /**
  * of_dma_request_slave_channel - Get the DMA slave channel
  * @np:		device node to get DMA request from
- * @name:	name of desired channel
+ * @name:	name of desired channel (optional)
  *
  * Returns pointer to appropriate DMA channel on success or an error pointer.
  */
@@ -243,8 +245,8 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 	int			count, i;
 	int			ret_no_channel = -ENODEV;
 
-	if (!np || !name) {
-		pr_err("%s: not enough information provided\n", __func__);
+	if (!np) {
+		pr_err("%s: device node is not valid\n", __func__);
 		return ERR_PTR(-ENODEV);
 	}
 
@@ -252,7 +254,12 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 	if (!of_find_property(np, "dmas", NULL))
 		return ERR_PTR(-ENODEV);
 
-	count = of_property_count_strings(np, "dma-names");
+	/*
+	 * If a name is not provided, then assume that there is one
+	 * DMA specifier in the list for the client and so set the
+	 * count to 1 and see if this matches.
+	 */
+	count = name ? of_property_count_strings(np, "dma-names") : 1;
 	if (count < 0) {
 		pr_err("%s: dma-names property of node '%s' missing or empty\n",
 			__func__, np->full_name);
-- 
2.1.4

[PATCH 2/3] 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/dma/tegra210-adma.txt      | 47 ++++++++++++++++++++++
 1 file changed, 47 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..af04b3c5a557
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
@@ -0,0 +1,47 @@
+* NVIDIA Tegra Audio DMA controller
+
+Required properties:
+- compatible: Should be "nvidia,<chip>-adma"
+- reg: Should contain DMA registers location and length. This should include
+  all of the per-channel registers.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts.
+- clocks: Must contain two entries, one for the power-domain clock and one
+  for the module clock. See ../clocks/clock-bindings.txt for details.
+- dma-channels: Number of DMA channels supported by the controller.
+- #dma-cells : Must be <0>.
+
+Examples:
+
+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_D_AUDIO>,
+		 <&tegra_car TEGRA210_CLK_ADMA_APE>;
+	clock-names = "adma", "adma.ape";
+	dma-channels = <22>;
+	#dma-cells = <0>;
+};
-- 
2.1.4

[PATCH 3/3] 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/dma/Kconfig         |  13 +
 drivers/dma/Makefile        |   1 +
 drivers/dma/tegra210-adma.c | 820 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 834 insertions(+)
 create mode 100644 drivers/dma/tegra210-adma.c

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index b4584757dae0..5721fc9aae32 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 7711a7180726..4d88c7efc4e1 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..73013fa5fed2
--- /dev/null
+++ b/drivers/dma/tegra210-adma.c
@@ -0,0 +1,820 @@
+/*
+ * 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;
+
+	/* 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				*dma_clk;
+	struct clk				*ape_clk;
+	void __iomem				*base_addr;
+	unsigned int				nr_channels;
+
+	/* 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:
+		burst_size = fls(tdc->config.dst_maxburst);
+		ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
+		ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
+				ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
+		ch_regs->src_addr = buf_addr;
+		break;
+
+	case DMA_DEV_TO_MEM:
+		burst_size = fls(tdc->config.src_maxburst);
+		ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
+		ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
+				ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
+		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_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 transfer 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);
+
+	if (tdc->desc)
+		tegra_adma_terminate_all(dc);
+
+	tdc->config_valid = false;
+	vchan_free_chan_resources(&tdc->vc);
+
+	pm_runtime_put(tdc2dev(tdc));
+}
+
+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 dma_chan *chan;
+
+	chan = dma_get_any_slave_channel(&tdma->dma_dev);
+	if (!chan)
+		return NULL;
+
+	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->dma_clk);
+	clk_disable_unprepare(tdma->ape_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->ape_clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable APE clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(tdma->dma_clk);
+	if (ret < 0) {
+		clk_disable_unprepare(tdma->ape_clk);
+		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;
+
+	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->dma_clk = devm_clk_get(&pdev->dev, "adma");
+	if (IS_ERR(tdma->dma_clk)) {
+		dev_err(&pdev->dev, "ADMA clock not found\n");
+		return PTR_ERR(tdma->dma_clk);
+	}
+
+	tdma->ape_clk = devm_clk_get(&pdev->dev, "adma.ape");
+	if (IS_ERR(tdma->ape_clk)) {
+		dev_err(&pdev->dev, "APE clock not found\n");
+		return PTR_ERR(tdma->ape_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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
+MODULE_AUTHOR("Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

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

[Arnd Bergmann]

On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
> +       case DMA_MEM_TO_DEV:
> +               burst_size = fls(tdc->config.dst_maxburst);
> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
> +               ch_regs->src_addr = buf_addr;
> +               break;
> +
> +       case DMA_DEV_TO_MEM:
> +               burst_size = fls(tdc->config.src_maxburst);
> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
> +               ch_regs->trg_addr = buf_addr;
> +               break;

Do not use the 'slave_id' field here to identify the slave device, that
concept is broken. Instead, put the slave identification into the
dma specifier in DT and read it out in your xlate function.

	Arnd

Re: [PATCH 1/3] dmaengine: of: Allow #dma-cells to be zero

[Arnd Bergmann]

On Friday 25 September 2015 15:56:38 Jon Hunter wrote:
> Currently, the #dma-cells property must be 1 or more. However, for some
> DMA controllers, where DMA clients may use any DMA channel and it is not
> necessary to specify any other DMA information in the device-tree DMA
> binding, setting #dma-cells to 0 is desirable. The Tegra210 ADMA
> controller is an example of a DMA controller where neither the DMA
> channel or the hardware request signal need to be encoded in the
> device-tree binding.
> 
> By allowing DMA controllers to set #dma-cells to 0, means that the
> "dma-names" property for these controllers is not needed. Therefore,
> update the of_dma_request_slave_channel() and of_dma_match_channel()
> functions to ignore this property if no name is provided.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Sorry, but this makes no sense. #dma-cells=0 would imply that there
is only one slave per DMA controller, but that is not the case here.
As commented in patch 3, you actually support multiple slaves, you
just use the wrong interface.

	Arnd

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

[Jon Hunter]

On 25/09/15 16:03, Arnd Bergmann wrote:
> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>> +       case DMA_MEM_TO_DEV:
>> +               burst_size = fls(tdc->config.dst_maxburst);
>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>> +               ch_regs->src_addr = buf_addr;
>> +               break;
>> +
>> +       case DMA_DEV_TO_MEM:
>> +               burst_size = fls(tdc->config.src_maxburst);
>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>> +               ch_regs->trg_addr = buf_addr;
>> +               break;
> 
> Do not use the 'slave_id' field here to identify the slave device, that
> concept is broken. Instead, put the slave identification into the
> dma specifier in DT and read it out in your xlate function.

Why is it broken?

What happens if I don't know the slave-id? In other words, the slave-id
can be dynamically allocated and associated with a given slave.

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

On Friday 25 September 2015 16:17:29 Jon Hunter wrote:
> On 25/09/15 16:03, Arnd Bergmann wrote:
> > On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
> >> +       case DMA_MEM_TO_DEV:
> >> +               burst_size = fls(tdc->config.dst_maxburst);
> >> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
> >> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
> >> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
> >> +               ch_regs->src_addr = buf_addr;
> >> +               break;
> >> +
> >> +       case DMA_DEV_TO_MEM:
> >> +               burst_size = fls(tdc->config.src_maxburst);
> >> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
> >> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
> >> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
> >> +               ch_regs->trg_addr = buf_addr;
> >> +               break;
> > 
> > Do not use the 'slave_id' field here to identify the slave device, that
> > concept is broken. Instead, put the slave identification into the
> > dma specifier in DT and read it out in your xlate function.
> 
> Why is it broken?

This requires the slave driver to pass a slave id through the
dmaengine_slave_config() interface, but we don't want the driver
to know how it is connected to the dma engine. That information is
not a property of the slave itself but rather of the connection
between the slave and the master, and it belongs into DT, similar
to how we deal with IRQ or GPIO numbers.

> What happens if I don't know the slave-id? In other words, the slave-id
> can be dynamically allocated and associated with a given slave.

How do you identify the slave that the slave-id is associated with?

	Arnd

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

[Jon Hunter]

On 25/09/15 16:17, Jon Hunter wrote:
> 
> On 25/09/15 16:03, Arnd Bergmann wrote:
>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>> +       case DMA_MEM_TO_DEV:
>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>> +               ch_regs->src_addr = buf_addr;
>>> +               break;
>>> +
>>> +       case DMA_DEV_TO_MEM:
>>> +               burst_size = fls(tdc->config.src_maxburst);
>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>> +               ch_regs->trg_addr = buf_addr;
>>> +               break;
>>
>> Do not use the 'slave_id' field here to identify the slave device, that
>> concept is broken. Instead, put the slave identification into the
>> dma specifier in DT and read it out in your xlate function.
> 
> Why is it broken?
> 
> What happens if I don't know the slave-id? In other words, the slave-id
> can be dynamically allocated and associated with a given slave.

I guess thinking about it some more, the driver could assign an id
itself to a given channel and I could avoid using slave_id here. There
are 22 channels and 10 tx and 10 rx requests. However, I could also
statically assign a mapping in DT for the clients if that is preferred.

Jon

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

[Stephen Warren]

On 09/25/2015 09:38 AM, Jon Hunter wrote:
>
> On 25/09/15 16:17, Jon Hunter wrote:
>>
>> On 25/09/15 16:03, Arnd Bergmann wrote:
>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>>> +       case DMA_MEM_TO_DEV:
>>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>>> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>>> +               ch_regs->src_addr = buf_addr;
>>>> +               break;
>>>> +
>>>> +       case DMA_DEV_TO_MEM:
>>>> +               burst_size = fls(tdc->config.src_maxburst);
>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>>> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>>> +               ch_regs->trg_addr = buf_addr;
>>>> +               break;
>>>
>>> Do not use the 'slave_id' field here to identify the slave device, that
>>> concept is broken. Instead, put the slave identification into the
>>> dma specifier in DT and read it out in your xlate function.
>>
>> Why is it broken?
>>
>> What happens if I don't know the slave-id? In other words, the slave-id
>> can be dynamically allocated and associated with a given slave.
>
> I guess thinking about it some more, the driver could assign an id
> itself to a given channel and I could avoid using slave_id here. There
> are 22 channels and 10 tx and 10 rx requests. However, I could also
> statically assign a mapping in DT for the clients if that is preferred.

The channel IDs should be dynamically assigned at run-time.

DT should provide the slave ID (if there is one) for each client.

The two concepts are completely orthogonal.

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

[Arnd Bergmann]

On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
> On 25/09/15 16:17, Jon Hunter wrote:
> > 
> > On 25/09/15 16:03, Arnd Bergmann wrote:
> >> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
> >>> +       case DMA_MEM_TO_DEV:
> >>> +               burst_size = fls(tdc->config.dst_maxburst);
> >>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
> >>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
> >>> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
> >>> +               ch_regs->src_addr = buf_addr;
> >>> +               break;
> >>> +
> >>> +       case DMA_DEV_TO_MEM:
> >>> +               burst_size = fls(tdc->config.src_maxburst);
> >>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
> >>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
> >>> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
> >>> +               ch_regs->trg_addr = buf_addr;
> >>> +               break;
> >>
> >> Do not use the 'slave_id' field here to identify the slave device, that
> >> concept is broken. Instead, put the slave identification into the
> >> dma specifier in DT and read it out in your xlate function.
> > 
> > Why is it broken?
> > 
> > What happens if I don't know the slave-id? In other words, the slave-id
> > can be dynamically allocated and associated with a given slave.
> 
> I guess thinking about it some more, the driver could assign an id
> itself to a given channel and I could avoid using slave_id here. There
> are 22 channels and 10 tx and 10 rx requests.

This sounds roughly right. So you could pick the ch_regs->ctrl value
when you allocate the tegra_adma_chan structure, and then map it to
the slave in the xlate() function.

> However, I could also
> statically assign a mapping in DT for the clients if that is preferred.

That's not what I was looking for, the DT should contain the information
that the DMA master uses to identify a device, not a setting that it
can freely choose.

	Arnd

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

[Jon Hunter]

On 25/09/15 16:47, Arnd Bergmann wrote:
> On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
>> On 25/09/15 16:17, Jon Hunter wrote:
>>>
>>> On 25/09/15 16:03, Arnd Bergmann wrote:
>>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>>>> +       case DMA_MEM_TO_DEV:
>>>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
>>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>>>> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>>>> +               ch_regs->src_addr = buf_addr;
>>>>> +               break;
>>>>> +
>>>>> +       case DMA_DEV_TO_MEM:
>>>>> +               burst_size = fls(tdc->config.src_maxburst);
>>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
>>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>>>> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>>>> +               ch_regs->trg_addr = buf_addr;
>>>>> +               break;
>>>>
>>>> Do not use the 'slave_id' field here to identify the slave device, that
>>>> concept is broken. Instead, put the slave identification into the
>>>> dma specifier in DT and read it out in your xlate function.
>>>
>>> Why is it broken?
>>>
>>> What happens if I don't know the slave-id? In other words, the slave-id
>>> can be dynamically allocated and associated with a given slave.
>>
>> I guess thinking about it some more, the driver could assign an id
>> itself to a given channel and I could avoid using slave_id here. There
>> are 22 channels and 10 tx and 10 rx requests.
> 
> This sounds roughly right. So you could pick the ch_regs->ctrl value
> when you allocate the tegra_adma_chan structure, and then map it to
> the slave in the xlate() function.

Actually, what I mentioned about would not work as it is not the DMA
that should assign the requests to the channel.

I think that I have poorly described the hardware and how it works, so
let me try and explain a bit more.

From a hardware perspective it looks like the following ...

memory <-> adma <-> adma-if <-> xbar <-> clients

where:
- memory is the system memory
- adma is the dma controller
- adma-if is the dma interface to a crossbar
- xbar is the crossbar
- clients are various audio interfaces, such as i2s, etc

The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
22 adma channels can be mapped to any of the 10 tx or rx ports. The
request signal used by the adma is determined by which port it is
configured to use. However, what makes this even more configurable is
the xbar is also a mux that can route adma-if ports to the various clients.

So potentially, you could use any adma channel and any port to route
audio to any of the clients. However, the adma-if needs to know which
adma channel is mapped to which port and so it probably makes most sense
for the adma-if binding (not in the mainline yet) so specify a mapping
even though any mapping could be used.

	admaif@0x702d0000 {
		dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
		...
		dma-names = "rx1", "tx1", "rx2", "tx2",
		...
	};

Alternatively, I was thinking that I could set #dma-cells = 0, but what
I should have done is just have something like ...

	admaif@0x702d0000 {
		dmas = <&adma>, <&adma>, <&adma>, <&adma>,
		...
		dma-names = "rx1", "tx1", "rx2", "tx2",
		...
	};

In the above case the adma-if driver can just pick whatever channel it
wants for each of the ports.

Hope this is clearer.

Cheers
Jon

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

[Jon Hunter]

On 25/09/15 16:47, Arnd Bergmann wrote:
> On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
>> On 25/09/15 16:17, Jon Hunter wrote:
>>>
>>> On 25/09/15 16:03, Arnd Bergmann wrote:
>>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>>>> +       case DMA_MEM_TO_DEV:
>>>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
>>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>>>> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>>>> +               ch_regs->src_addr = buf_addr;
>>>>> +               break;
>>>>> +
>>>>> +       case DMA_DEV_TO_MEM:
>>>>> +               burst_size = fls(tdc->config.src_maxburst);
>>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
>>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>>>> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>>>> +               ch_regs->trg_addr = buf_addr;
>>>>> +               break;
>>>>
>>>> Do not use the 'slave_id' field here to identify the slave device, that
>>>> concept is broken. Instead, put the slave identification into the
>>>> dma specifier in DT and read it out in your xlate function.
>>>
>>> Why is it broken?
>>>
>>> What happens if I don't know the slave-id? In other words, the slave-id
>>> can be dynamically allocated and associated with a given slave.
>>
>> I guess thinking about it some more, the driver could assign an id
>> itself to a given channel and I could avoid using slave_id here. There
>> are 22 channels and 10 tx and 10 rx requests.
> 
> This sounds roughly right. So you could pick the ch_regs->ctrl value
> when you allocate the tegra_adma_chan structure, and then map it to
> the slave in the xlate() function.

Actually, what I mentioned about would not work as it is not the DMA
that should assign the requests to the channel.

I think that I have poorly described the hardware and how it works, so
let me try and explain a bit more.

>From a hardware perspective it looks like the following ...

memory <-> adma <-> adma-if <-> xbar <-> clients

where:
- memory is the system memory
- adma is the dma controller
- adma-if is the dma interface to a crossbar
- xbar is the crossbar
- clients are various audio interfaces, such as i2s, etc

The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
22 adma channels can be mapped to any of the 10 tx or rx ports. The
request signal used by the adma is determined by which port it is
configured to use. However, what makes this even more configurable is
the xbar is also a mux that can route adma-if ports to the various clients.

So potentially, you could use any adma channel and any port to route
audio to any of the clients. However, the adma-if needs to know which
adma channel is mapped to which port and so it probably makes most sense
for the adma-if binding (not in the mainline yet) so specify a mapping
even though any mapping could be used.

	admaif@0x702d0000 {
		dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
		...
		dma-names = "rx1", "tx1", "rx2", "tx2",
		...
	};

Alternatively, I was thinking that I could set #dma-cells = 0, but what
I should have done is just have something like ...

	admaif@0x702d0000 {
		dmas = <&adma>, <&adma>, <&adma>, <&adma>,
		...
		dma-names = "rx1", "tx1", "rx2", "tx2",
		...
	};

In the above case the adma-if driver can just pick whatever channel it
wants for each of the ports.

Hope this is clearer.

Cheers
Jon

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

[Arnd Bergmann]

On Monday 28 September 2015 15:57:46 Jon Hunter wrote:
> On 25/09/15 16:47, Arnd Bergmann wrote:
> > On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
> >> On 25/09/15 16:17, Jon Hunter wrote:
> >>> On 25/09/15 16:03, Arnd Bergmann wrote:
> >>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
> >>>>> +       case DMA_MEM_TO_DEV:
> >>>>> +               burst_size = fls(tdc->config.dst_maxburst);
> >>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
> >>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
> >>>>> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
> >>>>> +               ch_regs->src_addr = buf_addr;
> >>>>> +               break;
> >>>>> +
> >>>>> +       case DMA_DEV_TO_MEM:
> >>>>> +               burst_size = fls(tdc->config.src_maxburst);
> >>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
> >>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
> >>>>> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
> >>>>> +               ch_regs->trg_addr = buf_addr;
> >>>>> +               break;
> >>>>
> >>>> Do not use the 'slave_id' field here to identify the slave device, that
> >>>> concept is broken. Instead, put the slave identification into the
> >>>> dma specifier in DT and read it out in your xlate function.
> >>>
> >>> Why is it broken?
> >>>
> >>> What happens if I don't know the slave-id? In other words, the slave-id
> >>> can be dynamically allocated and associated with a given slave.
> >>
> >> I guess thinking about it some more, the driver could assign an id
> >> itself to a given channel and I could avoid using slave_id here. There
> >> are 22 channels and 10 tx and 10 rx requests.
> > 
> > This sounds roughly right. So you could pick the ch_regs->ctrl value
> > when you allocate the tegra_adma_chan structure, and then map it to
> > the slave in the xlate() function.
> 
> Actually, what I mentioned about would not work as it is not the DMA
> that should assign the requests to the channel.
> 
> I think that I have poorly described the hardware and how it works, so
> let me try and explain a bit more.
> 
> From a hardware perspective it looks like the following ...
> 
> memory <-> adma <-> adma-if <-> xbar <-> clients
> 
> where:
> - memory is the system memory
> - adma is the dma controller
> - adma-if is the dma interface to a crossbar
> - xbar is the crossbar
> - clients are various audio interfaces, such as i2s, etc
> 
> The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
> 22 adma channels can be mapped to any of the 10 tx or rx ports. The
> request signal used by the adma is determined by which port it is
> configured to use. However, what makes this even more configurable is
> the xbar is also a mux that can route adma-if ports to the various clients.
> 
> So potentially, you could use any adma channel and any port to route
> audio to any of the clients. However, the adma-if needs to know which
> adma channel is mapped to which port and so it probably makes most sense
> for the adma-if binding (not in the mainline yet) so specify a mapping
> even though any mapping could be used.
> 
> 	admaif@0x702d0000 {
> 		dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
> 		...
> 		dma-names = "rx1", "tx1", "rx2", "tx2",
> 		...
> 	};
> 
> Alternatively, I was thinking that I could set #dma-cells = 0, but what
> I should have done is just have something like ...
> 
> 	admaif@0x702d0000 {
> 		dmas = <&adma>, <&adma>, <&adma>, <&adma>,
> 		...
> 		dma-names = "rx1", "tx1", "rx2", "tx2",
> 		...
> 	};
> 
> In the above case the adma-if driver can just pick whatever channel it
> wants for each of the ports.
> 
> Hope this is clearer.

I think what you are saying here is that the ADMA driver does not talk
to dma slave devices at all, but only talks to the adma-if, which in
turn only talks to the xbar.

That is fine, but the result of that is that there is no point in
using the DMA slave binding for this device, or for the driver to
register itself as a generic slave driver, which it is not.

We should look at this driver in combination with the adma-if and
xbar drivers and then define a binding for the combination of those.

	Arnd

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

[Stephen Warren]

On 09/28/2015 08:57 AM, Jon Hunter wrote:
>
> On 25/09/15 16:47, Arnd Bergmann wrote:
>> On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
>>> On 25/09/15 16:17, Jon Hunter wrote:
>>>>
>>>> On 25/09/15 16:03, Arnd Bergmann wrote:
>>>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>>>>> +       case DMA_MEM_TO_DEV:
>>>>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
>>>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>>>>> +                               ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>>>>> +               ch_regs->src_addr = buf_addr;
>>>>>> +               break;
>>>>>> +
>>>>>> +       case DMA_DEV_TO_MEM:
>>>>>> +               burst_size = fls(tdc->config.src_maxburst);
>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
>>>>>> +               ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>>>>> +                               ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>>>>> +               ch_regs->trg_addr = buf_addr;
>>>>>> +               break;
>>>>>
>>>>> Do not use the 'slave_id' field here to identify the slave device, that
>>>>> concept is broken. Instead, put the slave identification into the
>>>>> dma specifier in DT and read it out in your xlate function.
>>>>
>>>> Why is it broken?
>>>>
>>>> What happens if I don't know the slave-id? In other words, the slave-id
>>>> can be dynamically allocated and associated with a given slave.
>>>
>>> I guess thinking about it some more, the driver could assign an id
>>> itself to a given channel and I could avoid using slave_id here. There
>>> are 22 channels and 10 tx and 10 rx requests.
>>
>> This sounds roughly right. So you could pick the ch_regs->ctrl value
>> when you allocate the tegra_adma_chan structure, and then map it to
>> the slave in the xlate() function.
>
> Actually, what I mentioned about would not work as it is not the DMA
> that should assign the requests to the channel.
>
> I think that I have poorly described the hardware and how it works, so
> let me try and explain a bit more.
>
>  From a hardware perspective it looks like the following ...
>
> memory <-> adma <-> adma-if <-> xbar <-> clients
>
> where:
> - memory is the system memory
> - adma is the dma controller
> - adma-if is the dma interface to a crossbar
> - xbar is the crossbar
> - clients are various audio interfaces, such as i2s, etc
>
> The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
> 22 adma channels can be mapped to any of the 10 tx or rx ports. The
> request signal used by the adma is determined by which port it is
> configured to use. However, what makes this even more configurable is
> the xbar is also a mux that can route adma-if ports to the various clients.
>
> So potentially, you could use any adma channel and any port to route
> audio to any of the clients. However, the adma-if needs to know which
> adma channel is mapped to which port

It does? I'm pretty sure it didn't in earlier chips; what changed?

For earlier chips, I believe all that's required is:

When programming the DMA engine, you need to know which ADMA-IF is in 
use, so the correct DMA request selector can be programmed into the DMA 
engine for flow-control.

ADMA-IF simply receives the data from DMA, and forwards it to the XBAR 
tagged with the ADMA-IF's own ID.

The XBAR programming selects which data source (ADMA-IF TX, I2S RX, ...) 
each sink (ADMARX, I2S TX, ...) receives.

Ideally, when an I2S controller needs to start transmitting data, it 
should dynamically allocate an ADMA-IF, query it for its DMA slave 
request ID, and then forward this information to the ASoC code that sets 
up the DMA transfer.

In practice, this means that since any I2S module could use any ADMA-IF, 
you probably need to list all DMA request selectors possible in the 
I2S's DMA-related properties, so it can choose which one to use.

Or perhaps the XBAR binding should list all the DMA requestors so that 
each I2S node doesn't have to.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jon Hunter]

On 28/09/15 17:36, Stephen Warren wrote:
> On 09/28/2015 08:57 AM, Jon Hunter wrote:
>>
>> On 25/09/15 16:47, Arnd Bergmann wrote:
>>> On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
>>>> On 25/09/15 16:17, Jon Hunter wrote:
>>>>>
>>>>> On 25/09/15 16:03, Arnd Bergmann wrote:
>>>>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>>>>>> +       case DMA_MEM_TO_DEV:
>>>>>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs
>>>>>>> - 1);
>>>>>>> +               ch_regs->ctrl =
>>>>>>> ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>>>>>> +                              
>>>>>>> ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>>>>>> +               ch_regs->src_addr = buf_addr;
>>>>>>> +               break;
>>>>>>> +
>>>>>>> +       case DMA_DEV_TO_MEM:
>>>>>>> +               burst_size = fls(tdc->config.src_maxburst);
>>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs
>>>>>>> - 1);
>>>>>>> +               ch_regs->ctrl =
>>>>>>> ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>>>>>> +                              
>>>>>>> ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>>>>>> +               ch_regs->trg_addr = buf_addr;
>>>>>>> +               break;
>>>>>>
>>>>>> Do not use the 'slave_id' field here to identify the slave device,
>>>>>> that
>>>>>> concept is broken. Instead, put the slave identification into the
>>>>>> dma specifier in DT and read it out in your xlate function.
>>>>>
>>>>> Why is it broken?
>>>>>
>>>>> What happens if I don't know the slave-id? In other words, the
>>>>> slave-id
>>>>> can be dynamically allocated and associated with a given slave.
>>>>
>>>> I guess thinking about it some more, the driver could assign an id
>>>> itself to a given channel and I could avoid using slave_id here. There
>>>> are 22 channels and 10 tx and 10 rx requests.
>>>
>>> This sounds roughly right. So you could pick the ch_regs->ctrl value
>>> when you allocate the tegra_adma_chan structure, and then map it to
>>> the slave in the xlate() function.
>>
>> Actually, what I mentioned about would not work as it is not the DMA
>> that should assign the requests to the channel.
>>
>> I think that I have poorly described the hardware and how it works, so
>> let me try and explain a bit more.
>>
>>  From a hardware perspective it looks like the following ...
>>
>> memory <-> adma <-> adma-if <-> xbar <-> clients
>>
>> where:
>> - memory is the system memory
>> - adma is the dma controller
>> - adma-if is the dma interface to a crossbar
>> - xbar is the crossbar
>> - clients are various audio interfaces, such as i2s, etc
>>
>> The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
>> 22 adma channels can be mapped to any of the 10 tx or rx ports. The
>> request signal used by the adma is determined by which port it is
>> configured to use. However, what makes this even more configurable is
>> the xbar is also a mux that can route adma-if ports to the various
>> clients.
>>
>> So potentially, you could use any adma channel and any port to route
>> audio to any of the clients. However, the adma-if needs to know which
>> adma channel is mapped to which port
> 
> It does? I'm pretty sure it didn't in earlier chips; what changed?

I *believe* that T210 is the first one to have the ADMA controller where
as previous chips used the APB-DMA controller. Looking at the APB-DMA on
T124 I can see that there is a fixed REQ_SEL value for each of the APBIF
(equivalent of the ADMA-IF on T210).

> For earlier chips, I believe all that's required is:
> 
> When programming the DMA engine, you need to know which ADMA-IF is in
> use, so the correct DMA request selector can be programmed into the DMA
> engine for flow-control.
> 
> ADMA-IF simply receives the data from DMA, and forwards it to the XBAR
> tagged with the ADMA-IF's own ID.
> 
> The XBAR programming selects which data source (ADMA-IF TX, I2S RX, ...)
> each sink (ADMARX, I2S TX, ...) receives.

Yes, exactly, this part sounds the same. However, just the ADMA itself
allows for even more configuration.

> Ideally, when an I2S controller needs to start transmitting data, it
> should dynamically allocate an ADMA-IF, query it for its DMA slave
> request ID, and then forward this information to the ASoC code that sets
> up the DMA transfer.

Agree.

> In practice, this means that since any I2S module could use any ADMA-IF,
> you probably need to list all DMA request selectors possible in the
> I2S's DMA-related properties, so it can choose which one to use.

Possibly, but really I think that the i2s only cares about the ADMA-IF
and the hardware request used by the ADMA can be abstracted by the
ADMA-IF. In other words, if you allocate an ADMA channel to work with a
specific ADMA-IF, then let the ADMA-IF select the hardware request
because as long as one is available, you don't care which.

> Or perhaps the XBAR binding should list all the DMA requestors so that
> each I2S node doesn't have to.

Yes, however, I think that the ADMA-IF would make sense as it really
cares about the mapping of hardware request to the ADMA-IF port.

Cheers
Jon

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

[Jon Hunter]

On 28/09/15 16:07, Arnd Bergmann wrote:
> On Monday 28 September 2015 15:57:46 Jon Hunter wrote:

[snip]

>> Actually, what I mentioned about would not work as it is not the DMA
>> that should assign the requests to the channel.
>>
>> I think that I have poorly described the hardware and how it works, so
>> let me try and explain a bit more.
>>
>> From a hardware perspective it looks like the following ...
>>
>> memory <-> adma <-> adma-if <-> xbar <-> clients
>>
>> where:
>> - memory is the system memory
>> - adma is the dma controller
>> - adma-if is the dma interface to a crossbar
>> - xbar is the crossbar
>> - clients are various audio interfaces, such as i2s, etc
>>
>> The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
>> 22 adma channels can be mapped to any of the 10 tx or rx ports. The
>> request signal used by the adma is determined by which port it is
>> configured to use. However, what makes this even more configurable is
>> the xbar is also a mux that can route adma-if ports to the various clients.
>>
>> So potentially, you could use any adma channel and any port to route
>> audio to any of the clients. However, the adma-if needs to know which
>> adma channel is mapped to which port and so it probably makes most sense
>> for the adma-if binding (not in the mainline yet) so specify a mapping
>> even though any mapping could be used.
>>
>> 	admaif@0x702d0000 {
>> 		dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>> 		...
>> 		dma-names = "rx1", "tx1", "rx2", "tx2",
>> 		...
>> 	};
>>
>> Alternatively, I was thinking that I could set #dma-cells = 0, but what
>> I should have done is just have something like ...
>>
>> 	admaif@0x702d0000 {
>> 		dmas = <&adma>, <&adma>, <&adma>, <&adma>,
>> 		...
>> 		dma-names = "rx1", "tx1", "rx2", "tx2",
>> 		...
>> 	};
>>
>> In the above case the adma-if driver can just pick whatever channel it
>> wants for each of the ports.
>>
>> Hope this is clearer.
> 
> I think what you are saying here is that the ADMA driver does not talk
> to dma slave devices at all, but only talks to the adma-if, which in
> turn only talks to the xbar.

Yes, this is the use-case that this current version of the driver
supports. However ...

> That is fine, but the result of that is that there is no point in
> using the DMA slave binding for this device, or for the driver to
> register itself as a generic slave driver, which it is not.

... the DMA controller itself has a source and destination register and
from a hardware perspective, the source and/or destination does not need
to be the ADMA-IF. Potentially, we could support memory to memory
transfers, but currently we are not using the DMA for this. So I can see
that with the current usage it may not make sense for it to be
registered as a generic slave driver, but from a hardware perspective I
think it still does.

The current tegra audio drivers use the snd_dmaengine_xxx helpers and so
it would be nice to do the same here.

> We should look at this driver in combination with the adma-if and
> xbar drivers and then define a binding for the combination of those.

Yes that makes sense, but I think that I have confused matters here a
bit and not thought through this entirely. So while we could configure
an audio interface, such as i2s, to use any adma-if port and hence any
dma channel with the appropriate hardware request signal, the mapping
between the adma-if port and request signal is fixed. For example,

adma-if rx1 port uses adma request signal 1
adma-if rx2 port uses adma request signal 2
adma-if rx3 port uses adma request signal 3
...
adma-if rx10 port uses adma request signal 10

and

adma-if tx1 port uses adma request signal 1
adma-if tx2 port uses adma request signal 2
adma-if tx3 port uses adma request signal 3
...
adma-if tx10 port uses adma request signal 10

What is connected to these adma-if tx and rx ports who knows but I think
that is where I was going wrong and made this more complex than it
should have been. So I think that the adma binding should have
#dma-cells = 1 and the adma-if binding have the following ...

 	admaif@0x702d0000 {
 		dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
 		...
 		dma-names = "rx1", "tx1", "rx2", "tx2",
 		...
 	};


Sorry for all the confusion.

Jon

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

[Arnd Bergmann]

On Tuesday 29 September 2015 13:18:06 Jon Hunter wrote:
> 
> On 28/09/15 16:07, Arnd Bergmann wrote:
> > On Monday 28 September 2015 15:57:46 Jon Hunter wrote:
> 
> [snip]
> 
> >> Actually, what I mentioned about would not work as it is not the DMA
> >> that should assign the requests to the channel.
> >>
> >> I think that I have poorly described the hardware and how it works, so
> >> let me try and explain a bit more.
> >>
> >> From a hardware perspective it looks like the following ...
> >>
> >> memory <-> adma <-> adma-if <-> xbar <-> clients
> >>
> >> where:
> >> - memory is the system memory
> >> - adma is the dma controller
> >> - adma-if is the dma interface to a crossbar
> >> - xbar is the crossbar
> >> - clients are various audio interfaces, such as i2s, etc
> >>
> >> The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
> >> 22 adma channels can be mapped to any of the 10 tx or rx ports. The
> >> request signal used by the adma is determined by which port it is
> >> configured to use. However, what makes this even more configurable is
> >> the xbar is also a mux that can route adma-if ports to the various clients.
> >>
> >> So potentially, you could use any adma channel and any port to route
> >> audio to any of the clients. However, the adma-if needs to know which
> >> adma channel is mapped to which port and so it probably makes most sense
> >> for the adma-if binding (not in the mainline yet) so specify a mapping
> >> even though any mapping could be used.
> >>
> >> 	admaif@0x702d0000 {
> >> 		dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
> >> 		...
> >> 		dma-names = "rx1", "tx1", "rx2", "tx2",
> >> 		...
> >> 	};
> >>
> >> Alternatively, I was thinking that I could set #dma-cells = 0, but what
> >> I should have done is just have something like ...
> >>
> >> 	admaif@0x702d0000 {
> >> 		dmas = <&adma>, <&adma>, <&adma>, <&adma>,
> >> 		...
> >> 		dma-names = "rx1", "tx1", "rx2", "tx2",
> >> 		...
> >> 	};
> >>
> >> In the above case the adma-if driver can just pick whatever channel it
> >> wants for each of the ports.
> >>
> >> Hope this is clearer.
> > 
> > I think what you are saying here is that the ADMA driver does not talk
> > to dma slave devices at all, but only talks to the adma-if, which in
> > turn only talks to the xbar.
> 
> Yes, this is the use-case that this current version of the driver
> supports. However ...
> 
> > That is fine, but the result of that is that there is no point in
> > using the DMA slave binding for this device, or for the driver to
> > register itself as a generic slave driver, which it is not.
> 
> ... the DMA controller itself has a source and destination register and
> from a hardware perspective, the source and/or destination does not need
> to be the ADMA-IF. Potentially, we could support memory to memory
> transfers, but currently we are not using the DMA for this. So I can see
> that with the current usage it may not make sense for it to be
> registered as a generic slave driver, but from a hardware perspective I
> think it still does.

Ok, but imagine a device where the ADMA is actually wired up to a slave
directly. In this case, you need #dma-cells=<1> (or higher) to identify
which slave that is. If you want the binding to be generic enough to
handle that case, it needs to correctly describe the identification of
the request lines.
 
> The current tegra audio drivers use the snd_dmaengine_xxx helpers and so
> it would be nice to do the same here.

That seems to be a separate (non-)problem, as the audio devices would
have their 'dmas' properties pointing to the adma-if or the xbar (depending
on how you define those bindings), and don't need to change either way.

> > We should look at this driver in combination with the adma-if and
> > xbar drivers and then define a binding for the combination of those.
> 
> Yes that makes sense, but I think that I have confused matters here a
> bit and not thought through this entirely. So while we could configure
> an audio interface, such as i2s, to use any adma-if port and hence any
> dma channel with the appropriate hardware request signal, the mapping
> between the adma-if port and request signal is fixed. For example,
> 
> adma-if rx1 port uses adma request signal 1
> adma-if rx2 port uses adma request signal 2
> adma-if rx3 port uses adma request signal 3
> ...
> adma-if rx10 port uses adma request signal 10
> 
> and
> 
> adma-if tx1 port uses adma request signal 1
> adma-if tx2 port uses adma request signal 2
> adma-if tx3 port uses adma request signal 3
> ...
> adma-if tx10 port uses adma request signal 10
> 
> What is connected to these adma-if tx and rx ports who knows but I think
> that is where I was going wrong and made this more complex than it
> should have been. So I think that the adma binding should have
> #dma-cells = 1 and the adma-if binding have the following ...
> 
>  	admaif@0x702d0000 {
>  		dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>  		...
>  		dma-names = "rx1", "tx1", "rx2", "tx2",
>  		...
>  	};

Ok, that makes more sense then. A few questions still:

* Would the admaif in turn provide a #dma-cells and have the real slaves
  hooked up to it?
* How do you model the xbar in this scenario?
* How does the adma driver know whether you are using a rx or tx channel
  in the above example, should that perhaps be another cell in dma
  specifier? It seems that the numbers are all overlapping between rx and
  tx (each has 1 through 10).

	Arnd

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

[Jon Hunter]

On 29/09/15 13:39, Arnd Bergmann wrote:

[snip]

> Ok, that makes more sense then. A few questions still:
> 
> * Would the admaif in turn provide a #dma-cells and have the real slaves
>   hooked up to it?

I don't think that that would be necessary. If you look at the existing
tegra i2s binding [0], there is a ahub-cif-ids property which maps the
actual slave to the apbif (equivalent of the adma-if). This ID is used
to get the appropriate dma channel for this client interface (cif).

> * How do you model the xbar in this scenario?

The xbar is common to existing tegra devices and today is configured via
the ahub-cif-ids property.

> * How does the adma driver know whether you are using a rx or tx channel
>   in the above example, should that perhaps be another cell in dma
>   specifier? It seems that the numbers are all overlapping between rx and
>   tx (each has 1 through 10).

The channels can be configured to be either rx or tx and yes the IDs do
overlap so you have 1-10 for both rx and tx. However, if you are asking
how do I ensure that only one channel uses a specific hardware request,
then yes I probably need to add the direction to the specifier to ensure
only one channel uses a request at any given time.

Cheers
Jon

[0]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.txt

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

[Jon Hunter]

On 29/09/15 09:13, Jon Hunter wrote:
> 
> On 28/09/15 17:36, Stephen Warren wrote:
>> On 09/28/2015 08:57 AM, Jon Hunter wrote:
>>>
>>> On 25/09/15 16:47, Arnd Bergmann wrote:
>>>> On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
>>>>> On 25/09/15 16:17, Jon Hunter wrote:
>>>>>>
>>>>>> On 25/09/15 16:03, Arnd Bergmann wrote:
>>>>>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>>>>>>> +       case DMA_MEM_TO_DEV:
>>>>>>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs
>>>>>>>> - 1);
>>>>>>>> +               ch_regs->ctrl =
>>>>>>>> ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>>>>>>> +                              
>>>>>>>> ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>>>>>>> +               ch_regs->src_addr = buf_addr;
>>>>>>>> +               break;
>>>>>>>> +
>>>>>>>> +       case DMA_DEV_TO_MEM:
>>>>>>>> +               burst_size = fls(tdc->config.src_maxburst);
>>>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs
>>>>>>>> - 1);
>>>>>>>> +               ch_regs->ctrl =
>>>>>>>> ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>>>>>>> +                              
>>>>>>>> ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>>>>>>> +               ch_regs->trg_addr = buf_addr;
>>>>>>>> +               break;
>>>>>>>
>>>>>>> Do not use the 'slave_id' field here to identify the slave device,
>>>>>>> that
>>>>>>> concept is broken. Instead, put the slave identification into the
>>>>>>> dma specifier in DT and read it out in your xlate function.
>>>>>>
>>>>>> Why is it broken?
>>>>>>
>>>>>> What happens if I don't know the slave-id? In other words, the
>>>>>> slave-id
>>>>>> can be dynamically allocated and associated with a given slave.
>>>>>
>>>>> I guess thinking about it some more, the driver could assign an id
>>>>> itself to a given channel and I could avoid using slave_id here. There
>>>>> are 22 channels and 10 tx and 10 rx requests.
>>>>
>>>> This sounds roughly right. So you could pick the ch_regs->ctrl value
>>>> when you allocate the tegra_adma_chan structure, and then map it to
>>>> the slave in the xlate() function.
>>>
>>> Actually, what I mentioned about would not work as it is not the DMA
>>> that should assign the requests to the channel.
>>>
>>> I think that I have poorly described the hardware and how it works, so
>>> let me try and explain a bit more.
>>>
>>>  From a hardware perspective it looks like the following ...
>>>
>>> memory <-> adma <-> adma-if <-> xbar <-> clients
>>>
>>> where:
>>> - memory is the system memory
>>> - adma is the dma controller
>>> - adma-if is the dma interface to a crossbar
>>> - xbar is the crossbar
>>> - clients are various audio interfaces, such as i2s, etc
>>>
>>> The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
>>> 22 adma channels can be mapped to any of the 10 tx or rx ports. The
>>> request signal used by the adma is determined by which port it is
>>> configured to use. However, what makes this even more configurable is
>>> the xbar is also a mux that can route adma-if ports to the various
>>> clients.
>>>
>>> So potentially, you could use any adma channel and any port to route
>>> audio to any of the clients. However, the adma-if needs to know which
>>> adma channel is mapped to which port
>>
>> It does? I'm pretty sure it didn't in earlier chips; what changed?
> 
> I *believe* that T210 is the first one to have the ADMA controller where
> as previous chips used the APB-DMA controller. Looking at the APB-DMA on
> T124 I can see that there is a fixed REQ_SEL value for each of the APBIF
> (equivalent of the ADMA-IF on T210).

Actually, I think that T210 is the same as the previous chips. Because
of the xbar you could potentially have various different routing
possibilities between the DMA, CIF and client. However, the request
signal between the DMA and CIF is fixed.

Sorry, I think my brain melted while traversing the audio subsystem.

Jon

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

[Arnd Bergmann]

On Tuesday 29 September 2015 15:18:07 Jon Hunter wrote:
> On 29/09/15 13:39, Arnd Bergmann wrote:
> > Ok, that makes more sense then. A few questions still:
> > 
> > * Would the admaif in turn provide a #dma-cells and have the real slaves
> >   hooked up to it?
> 
> I don't think that that would be necessary. If you look at the existing
> tegra i2s binding [0], there is a ahub-cif-ids property which maps the
> actual slave to the apbif (equivalent of the adma-if). This ID is used
> to get the appropriate dma channel for this client interface (cif).
> 
> > * How do you model the xbar in this scenario?
> 
> The xbar is common to existing tegra devices and today is configured via
> the ahub-cif-ids property.

I see, so instead of modeling the xbar as part of the DMA engine in DT, you
model it as part of the slave. This probably adds a bit of complexity
and a somewhat nonstandard binding, but it's too late to change that anyway.

> > * How does the adma driver know whether you are using a rx or tx channel
> >   in the above example, should that perhaps be another cell in dma
> >   specifier? It seems that the numbers are all overlapping between rx and
> >   tx (each has 1 through 10).
> 
> The channels can be configured to be either rx or tx and yes the IDs do
> overlap so you have 1-10 for both rx and tx. However, if you are asking
> how do I ensure that only one channel uses a specific hardware request,
> then yes I probably need to add the direction to the specifier to ensure
> only one channel uses a request at any given time.

If each channel is either rx or tx, but the two cannot be used simultaneously,
you can just list each of them only once, like this:

	dmas = <&adma 0>, <&adma 1>, <&adma 2>, ...
	dma-names = "if0", "if1", "if2", ...

Otherwise you end up allocating twice the number of channels that you
really need.

	Arnd

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

[Jon Hunter]

On 29/09/15 16:34, Arnd Bergmann wrote:
> On Tuesday 29 September 2015 15:18:07 Jon Hunter wrote:
>> On 29/09/15 13:39, Arnd Bergmann wrote:
>>> Ok, that makes more sense then. A few questions still:
>>>
>>> * Would the admaif in turn provide a #dma-cells and have the real slaves
>>>   hooked up to it?
>>
>> I don't think that that would be necessary. If you look at the existing
>> tegra i2s binding [0], there is a ahub-cif-ids property which maps the
>> actual slave to the apbif (equivalent of the adma-if). This ID is used
>> to get the appropriate dma channel for this client interface (cif).
>>
>>> * How do you model the xbar in this scenario?
>>
>> The xbar is common to existing tegra devices and today is configured via
>> the ahub-cif-ids property.
> 
> I see, so instead of modeling the xbar as part of the DMA engine in DT, you
> model it as part of the slave. This probably adds a bit of complexity
> and a somewhat nonstandard binding, but it's too late to change that anyway.
> 
>>> * How does the adma driver know whether you are using a rx or tx channel
>>>   in the above example, should that perhaps be another cell in dma
>>>   specifier? It seems that the numbers are all overlapping between rx and
>>>   tx (each has 1 through 10).
>>
>> The channels can be configured to be either rx or tx and yes the IDs do
>> overlap so you have 1-10 for both rx and tx. However, if you are asking
>> how do I ensure that only one channel uses a specific hardware request,
>> then yes I probably need to add the direction to the specifier to ensure
>> only one channel uses a request at any given time.
> 
> If each channel is either rx or tx, but the two cannot be used simultaneously,
> you can just list each of them only once, like this:
> 
> 	dmas = <&adma 0>, <&adma 1>, <&adma 2>, ...
> 	dma-names = "if0", "if1", "if2", ...
> 
> Otherwise you end up allocating twice the number of channels that you
> really need.

Each dma channel can be configured for either tx or rx. There are 10 tx
ports on the adma-if and 10 rx. So you could have 20 dma channels
configured for each one of the tx and rx ports. It is a bit confusing
because rx port 1 has a request signal of 1 and tx port 1 has a request
signal of 1 as well. So if you are wondering how the dma distinguishes
between rx port 1 and tx port 1, it does so by having a separate field
for the tx and rx request in the dma channel control register.

Cheers
Jon

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

[Stephen Warren]

On 09/29/2015 02:13 AM, Jon Hunter wrote:
>
> On 28/09/15 17:36, Stephen Warren wrote:
>> On 09/28/2015 08:57 AM, Jon Hunter wrote:
>>>
>>> On 25/09/15 16:47, Arnd Bergmann wrote:
>>>> On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
>>>>> On 25/09/15 16:17, Jon Hunter wrote:
>>>>>>
>>>>>> On 25/09/15 16:03, Arnd Bergmann wrote:
>>>>>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>>>>>>> +       case DMA_MEM_TO_DEV:
>>>>>>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs
>>>>>>>> - 1);
>>>>>>>> +               ch_regs->ctrl =
>>>>>>>> ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>>>>>>> +
>>>>>>>> ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>>>>>>> +               ch_regs->src_addr = buf_addr;
>>>>>>>> +               break;
>>>>>>>> +
>>>>>>>> +       case DMA_DEV_TO_MEM:
>>>>>>>> +               burst_size = fls(tdc->config.src_maxburst);
>>>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs
>>>>>>>> - 1);
>>>>>>>> +               ch_regs->ctrl =
>>>>>>>> ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>>>>>>> +
>>>>>>>> ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>>>>>>> +               ch_regs->trg_addr = buf_addr;
>>>>>>>> +               break;
>>>>>>>
>>>>>>> Do not use the 'slave_id' field here to identify the slave device,
>>>>>>> that
>>>>>>> concept is broken. Instead, put the slave identification into the
>>>>>>> dma specifier in DT and read it out in your xlate function.
>>>>>>
>>>>>> Why is it broken?
>>>>>>
>>>>>> What happens if I don't know the slave-id? In other words, the
>>>>>> slave-id
>>>>>> can be dynamically allocated and associated with a given slave.
>>>>>
>>>>> I guess thinking about it some more, the driver could assign an id
>>>>> itself to a given channel and I could avoid using slave_id here. There
>>>>> are 22 channels and 10 tx and 10 rx requests.
>>>>
>>>> This sounds roughly right. So you could pick the ch_regs->ctrl value
>>>> when you allocate the tegra_adma_chan structure, and then map it to
>>>> the slave in the xlate() function.
>>>
>>> Actually, what I mentioned about would not work as it is not the DMA
>>> that should assign the requests to the channel.
>>>
>>> I think that I have poorly described the hardware and how it works, so
>>> let me try and explain a bit more.
>>>
>>>   From a hardware perspective it looks like the following ...
>>>
>>> memory <-> adma <-> adma-if <-> xbar <-> clients
>>>
>>> where:
>>> - memory is the system memory
>>> - adma is the dma controller
>>> - adma-if is the dma interface to a crossbar
>>> - xbar is the crossbar
>>> - clients are various audio interfaces, such as i2s, etc
>>>
>>> The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
>>> 22 adma channels can be mapped to any of the 10 tx or rx ports. The
>>> request signal used by the adma is determined by which port it is
>>> configured to use. However, what makes this even more configurable is
>>> the xbar is also a mux that can route adma-if ports to the various
>>> clients.
>>>
>>> So potentially, you could use any adma channel and any port to route
>>> audio to any of the clients. However, the adma-if needs to know which
>>> adma channel is mapped to which port
>>
>> It does? I'm pretty sure it didn't in earlier chips; what changed?
>
> I *believe* that T210 is the first one to have the ADMA controller where
> as previous chips used the APB-DMA controller.

Yes, I believe that's true.

> Looking at the APB-DMA on
> T124 I can see that there is a fixed REQ_SEL value for each of the APBIF
> (equivalent of the ADMA-IF on T210).

I believe that is still true on T210. I don't see any difference in the 
way request select values are used/configured/... The only difference is 
that we're using a different DMA engine, but that DMA engine is 
configured in the same way.

>> For earlier chips, I believe all that's required is:
>>
>> When programming the DMA engine, you need to know which ADMA-IF is in
>> use, so the correct DMA request selector can be programmed into the DMA
>> engine for flow-control.
>>
>> ADMA-IF simply receives the data from DMA, and forwards it to the XBAR
>> tagged with the ADMA-IF's own ID.
>>
>> The XBAR programming selects which data source (ADMA-IF TX, I2S RX, ...)
>> each sink (ADMARX, I2S TX, ...) receives.
>
> Yes, exactly, this part sounds the same. However, just the ADMA itself
> allows for even more configuration.
>
>> Ideally, when an I2S controller needs to start transmitting data, it
>> should dynamically allocate an ADMA-IF, query it for its DMA slave
>> request ID, and then forward this information to the ASoC code that sets
>> up the DMA transfer.
>
> Agree.
>
>> In practice, this means that since any I2S module could use any ADMA-IF,
>> you probably need to list all DMA request selectors possible in the
>> I2S's DMA-related properties, so it can choose which one to use.
>
> Possibly, but really I think that the i2s only cares about the ADMA-IF
> and the hardware request used by the ADMA can be abstracted by the
> ADMA-IF. In other words, if you allocate an ADMA channel to work with a
> specific ADMA-IF, then let the ADMA-IF select the hardware request
> because as long as one is available, you don't care which.

Each ADMAIF (FIFO) has a single request select value statically assigned 
to it as far as I can tell, just like in previous chips.

>> Or perhaps the XBAR binding should list all the DMA requestors so that
>> each I2S node doesn't have to.
>
> Yes, however, I think that the ADMA-IF would make sense as it really
> cares about the mapping of hardware request to the ADMA-IF port.
>
> Cheers
> Jon

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Warren]

On 09/29/2015 06:18 AM, Jon Hunter wrote:
> On 28/09/15 16:07, Arnd Bergmann wrote:
>> On Monday 28 September 2015 15:57:46 Jon Hunter wrote:
...
> Yes that makes sense, but I think that I have confused matters here a
> bit and not thought through this entirely. So while we could configure
> an audio interface, such as i2s, to use any adma-if port and hence any
> dma channel with the appropriate hardware request signal, the mapping
> between the adma-if port and request signal is fixed. For example,
>
> adma-if rx1 port uses adma request signal 1
> adma-if rx2 port uses adma request signal 2
> adma-if rx3 port uses adma request signal 3
> ...
> adma-if rx10 port uses adma request signal 10
>
> and
>
> adma-if tx1 port uses adma request signal 1
> adma-if tx2 port uses adma request signal 2
> adma-if tx3 port uses adma request signal 3
> ...
> adma-if tx10 port uses adma request signal 10
>
> What is connected to these adma-if tx and rx ports who knows but I think
> that is where I was going wrong and made this more complex than it
> should have been. So I think that the adma binding should have
> #dma-cells = 1 and the adma-if binding have the following ...
>
>   	admaif@0x702d0000 {
>   		dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>   		...
>   		dma-names = "rx1", "tx1", "rx2", "tx2",
>   		...
>   	};

Yes, that sounds about right.

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

[Stephen Warren]

On 09/29/2015 09:34 AM, Arnd Bergmann wrote:
> On Tuesday 29 September 2015 15:18:07 Jon Hunter wrote:
>> On 29/09/15 13:39, Arnd Bergmann wrote:
>>> Ok, that makes more sense then. A few questions still:
>>>
>>> * Would the admaif in turn provide a #dma-cells and have the real slaves
>>>    hooked up to it?
>>
>> I don't think that that would be necessary. If you look at the existing
>> tegra i2s binding [0], there is a ahub-cif-ids property which maps the
>> actual slave to the apbif (equivalent of the adma-if). This ID is used
>> to get the appropriate dma channel for this client interface (cif).
>>
>>> * How do you model the xbar in this scenario?
>>
>> The xbar is common to existing tegra devices and today is configured via
>> the ahub-cif-ids property.
>
> I see, so instead of modeling the xbar as part of the DMA engine in DT, you
> model it as part of the slave. This probably adds a bit of complexity
> and a somewhat nonstandard binding, but it's too late to change that anyway.

The XBAR shouldn't be modelled as part of the DMA engine; it's something 
entirely separate.

The data flow (for TX) is:

There's a FIFO in the ADMA-IF. This has a register address. That address 
can be written to by either PIO (CPU writes) or a DMA engine (in which 
case a "request selector" is used to communicate flow control 
information from the ADMI-IF to the DMA engine). Any DMA engine that may 
be used to write into this FIFO is an entirely separate HW module from 
the ADMA-IF itself.

The ADMA-IF HW drains data from this FIFO and pushes it into the XBAR. 
ADMA-IF is an XBAR data source.

The XBAR is a programmable cross-bar matrix. Many sinks are attached to 
it such as I2S, SPDIF, SRC (sample rate converters), mux/demux, etc. The 
data flow within the XBAR is purely in logic or internal RAM/flops. 
There is no transfer to/from DRAM within the XBAR.

For each XBAR sink, the XBAR has registers to select which XBAR source 
provides data to it.

Thus the ADMA-IF is a DMA sink, and XBAR is unrelated to DMA; it's 
simply part of the HW processing of the data once it gets into the ADMA-IF.

The RX flow is essentially the same in reverse; the I2S RX being an XBAR 
source, and a second FIFO in the ADMA-IF being an XBAR sink.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html