[RESEND PATCH 0/5] ipq8064 NAND support

[RESEND PATCH 0/5] ipq8064 NAND support

[Thomas Pedersen]

These patches complete NAND flash support for ipq8064 based boards.

Andy Gross (3):
  dtbindings: qcom_adm: Fix channel specifiers
  dmaengine: Add ADM driver
  arm: qcom: dts: ipq8064: Add ADM device node

Archit Taneja (2):
  arm: qcom: dts: Add NAND controller node for ipq806x
  arm: qcom: dts: Enable NAND node on IPQ8064 AP148 platform

 Documentation/devicetree/bindings/dma/qcom_adm.txt |  16 +-
 arch/arm/boot/dts/qcom-ipq8064-ap148.dts           |  42 +
 arch/arm/boot/dts/qcom-ipq8064.dtsi                |  38 +
 drivers/dma/qcom/Kconfig                           |  10 +
 drivers/dma/qcom/Makefile                          |   1 +
 drivers/dma/qcom/qcom_adm.c                        | 900 +++++++++++++++++++++
 6 files changed, 997 insertions(+), 10 deletions(-)
 create mode 100644 drivers/dma/qcom/qcom_adm.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers

[Thomas Pedersen]

From: Andy Gross <andy.gross@linaro.org>

This patch removes the crci information from the dma
channel property.  At least one client device requires
using more than one CRCI value for a channel.  This does
not match the current binding and the crci information
needs to be removed.

Instead, the client device will provide this information
via other means.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
---
 Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt b/Documentation/devicetree/bindings/dma/qcom_adm.txt
index 9bcab91..38d45f8 100644
--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt
+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt
@@ -4,8 +4,7 @@ Required properties:
 - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960
 - reg: Address range for DMA registers
 - interrupts: Should contain one interrupt shared by all channels
-- #dma-cells: must be <2>.  First cell denotes the channel number.  Second cell
-  denotes CRCI (client rate control interface) flow control assignment.
+- #dma-cells: must be <1>.  First cell denotes the channel number.
 - clocks: Should contain the core clock and interface clock.
 - clock-names: Must contain "core" for the core clock and "iface" for the
   interface clock.
@@ -22,7 +21,7 @@ Example:
 			compatible = "qcom,adm";
 			reg = <0x18300000 0x100000>;
 			interrupts = <0 170 0>;
-			#dma-cells = <2>;
+			#dma-cells = <1>;
 
 			clocks = <&gcc ADM0_CLK>, <&gcc ADM0_PBUS_CLK>;
 			clock-names = "core", "iface";
@@ -35,15 +34,12 @@ Example:
 			qcom,ee = <0>;
 		};
 
-DMA clients must use the format descripted in the dma.txt file, using a three
+DMA clients must use the format descripted in the dma.txt file, using a two
 cell specifier for each channel.
 
-Each dmas request consists of 3 cells:
+Each dmas request consists of two cells:
  1. phandle pointing to the DMA controller
  2. channel number
- 3. CRCI assignment, if applicable.  If no CRCI flow control is required, use 0.
-    The CRCI is used for flow control.  It identifies the peripheral device that
-    is the source/destination for the transferred data.
 
 Example:
 
@@ -56,7 +52,7 @@ Example:
 
 		cs-gpios = <&qcom_pinmux 20 0>;
 
-		dmas = <&adm_dma 6 9>,
-			<&adm_dma 5 10>;
+		dmas = <&adm_dma 6>,
+			<&adm_dma 5>;
 		dma-names = "rx", "tx";
 	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH 2/5] dmaengine: Add ADM driver

[Thomas Pedersen]

From: Andy Gross <andy.gross@linaro.org>

Add the DMA engine driver for the QCOM Application Data Mover (ADM) DMA
controller found in the MSM8x60 and IPQ/APQ8064 platforms.

The ADM supports both memory to memory transactions and memory
to/from peripheral device transactions.  The controller also provides flow
control capabilities for transactions to/from peripheral devices.

The initial release of this driver supports slave transfers to/from peripherals
and also incorporates CRCI (client rate control interface) flow control.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
---
 drivers/dma/qcom/Kconfig    |  10 +
 drivers/dma/qcom/Makefile   |   1 +
 drivers/dma/qcom/qcom_adm.c | 900 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 911 insertions(+)
 create mode 100644 drivers/dma/qcom/qcom_adm.c

diff --git a/drivers/dma/qcom/Kconfig b/drivers/dma/qcom/Kconfig
index a7761c4..a88769e 100644
--- a/drivers/dma/qcom/Kconfig
+++ b/drivers/dma/qcom/Kconfig
@@ -27,3 +27,13 @@ config QCOM_HIDMA
 	  (user to kernel, kernel to kernel, etc.).  It only supports
 	  memcpy interface. The core is not intended for general
 	  purpose slave DMA.
+
+config QCOM_ADM
+	tristate "Qualcomm ADM support"
+	depends on ARCH_QCOM || (COMPILE_TEST && OF && ARM)
+	select DMA_ENGINE
+	select DMA_VIRTUAL_CHANNELS
+	---help---
+	  Enable support for the Qualcomm ADM DMA controller.  This controller
+	  provides DMA capabilities for both general purpose and on-chip
+	  peripheral devices.
diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile
index 4bfc38b..b557769 100644
--- a/drivers/dma/qcom/Makefile
+++ b/drivers/dma/qcom/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_QCOM_HIDMA_MGMT) += hdma_mgmt.o
 hdma_mgmt-objs	 := hidma_mgmt.o hidma_mgmt_sys.o
 obj-$(CONFIG_QCOM_HIDMA) +=  hdma.o
 hdma-objs        := hidma_ll.o hidma.o hidma_dbg.o
+obj-$(CONFIG_QCOM_ADM) += qcom_adm.o
diff --git a/drivers/dma/qcom/qcom_adm.c b/drivers/dma/qcom/qcom_adm.c
new file mode 100644
index 0000000..e4485f3
--- /dev/null
+++ b/drivers/dma/qcom/qcom_adm.c
@@ -0,0 +1,900 @@
+/*
+ * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/dma-mapping.h>
+#include <linux/scatterlist.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_dma.h>
+#include <linux/reset.h>
+#include <linux/clk.h>
+#include <linux/dmaengine.h>
+
+#include "../dmaengine.h"
+#include "../virt-dma.h"
+
+/* ADM registers - calculated from channel number and security domain */
+#define ADM_CHAN_MULTI			0x4
+#define ADM_CI_MULTI			0x4
+#define ADM_CRCI_MULTI			0x4
+#define ADM_EE_MULTI			0x800
+#define ADM_CHAN_OFFS(chan)		(ADM_CHAN_MULTI * chan)
+#define ADM_EE_OFFS(ee)			(ADM_EE_MULTI * ee)
+#define ADM_CHAN_EE_OFFS(chan, ee)	(ADM_CHAN_OFFS(chan) + ADM_EE_OFFS(ee))
+#define ADM_CHAN_OFFS(chan)		(ADM_CHAN_MULTI * chan)
+#define ADM_CI_OFFS(ci)			(ADM_CHAN_OFF(ci))
+#define ADM_CH_CMD_PTR(chan, ee)	(ADM_CHAN_EE_OFFS(chan, ee))
+#define ADM_CH_RSLT(chan, ee)		(0x40 + ADM_CHAN_EE_OFFS(chan, ee))
+#define ADM_CH_FLUSH_STATE0(chan, ee)	(0x80 + ADM_CHAN_EE_OFFS(chan, ee))
+#define ADM_CH_STATUS_SD(chan, ee)	(0x200 + ADM_CHAN_EE_OFFS(chan, ee))
+#define ADM_CH_CONF(chan)		(0x240 + ADM_CHAN_OFFS(chan))
+#define ADM_CH_RSLT_CONF(chan, ee)	(0x300 + ADM_CHAN_EE_OFFS(chan, ee))
+#define ADM_SEC_DOMAIN_IRQ_STATUS(ee)	(0x380 + ADM_EE_OFFS(ee))
+#define ADM_CI_CONF(ci)			(0x390 + ci * ADM_CI_MULTI)
+#define ADM_GP_CTL			0x3d8
+#define ADM_CRCI_CTL(crci, ee)		(0x400 + crci * ADM_CRCI_MULTI + \
+						ADM_EE_OFFS(ee))
+
+/* channel status */
+#define ADM_CH_STATUS_VALID	BIT(1)
+
+/* channel result */
+#define ADM_CH_RSLT_VALID	BIT(31)
+#define ADM_CH_RSLT_ERR		BIT(3)
+#define ADM_CH_RSLT_FLUSH	BIT(2)
+#define ADM_CH_RSLT_TPD		BIT(1)
+
+/* channel conf */
+#define ADM_CH_CONF_SHADOW_EN		BIT(12)
+#define ADM_CH_CONF_MPU_DISABLE		BIT(11)
+#define ADM_CH_CONF_PERM_MPU_CONF	BIT(9)
+#define ADM_CH_CONF_FORCE_RSLT_EN	BIT(7)
+#define ADM_CH_CONF_SEC_DOMAIN(ee)	(((ee & 0x3) << 4) | ((ee & 0x4) << 11))
+
+/* channel result conf */
+#define ADM_CH_RSLT_CONF_FLUSH_EN	BIT(1)
+#define ADM_CH_RSLT_CONF_IRQ_EN		BIT(0)
+
+/* CRCI CTL */
+#define ADM_CRCI_CTL_MUX_SEL	BIT(18)
+#define ADM_CRCI_CTL_RST	BIT(17)
+
+/* CI configuration */
+#define ADM_CI_RANGE_END(x)	(x << 24)
+#define ADM_CI_RANGE_START(x)	(x << 16)
+#define ADM_CI_BURST_4_WORDS	BIT(2)
+#define ADM_CI_BURST_8_WORDS	BIT(3)
+
+/* GP CTL */
+#define ADM_GP_CTL_LP_EN	BIT(12)
+#define ADM_GP_CTL_LP_CNT(x)	(x << 8)
+
+/* Command pointer list entry */
+#define ADM_CPLE_LP		BIT(31)
+#define ADM_CPLE_CMD_PTR_LIST	BIT(29)
+
+/* Command list entry */
+#define ADM_CMD_LC		BIT(31)
+#define ADM_CMD_DST_CRCI(n)	(((n) & 0xf) << 7)
+#define ADM_CMD_SRC_CRCI(n)	(((n) & 0xf) << 3)
+
+#define ADM_CMD_TYPE_SINGLE	0x0
+#define ADM_CMD_TYPE_BOX	0x3
+
+#define ADM_CRCI_MUX_SEL	BIT(4)
+#define ADM_DESC_ALIGN		8
+#define ADM_MAX_XFER		(SZ_64K-1)
+#define ADM_MAX_ROWS		(SZ_64K-1)
+#define ADM_MAX_CHANNELS	16
+
+struct adm_desc_hw_box {
+	u32 cmd;
+	u32 src_addr;
+	u32 dst_addr;
+	u32 row_len;
+	u32 num_rows;
+	u32 row_offset;
+};
+
+struct adm_desc_hw_single {
+	u32 cmd;
+	u32 src_addr;
+	u32 dst_addr;
+	u32 len;
+};
+
+struct adm_async_desc {
+	struct virt_dma_desc vd;
+	struct adm_device *adev;
+
+	size_t length;
+	enum dma_transfer_direction dir;
+	dma_addr_t dma_addr;
+	size_t dma_len;
+
+	void *cpl;
+	dma_addr_t cp_addr;
+	u32 crci;
+	u32 mux;
+	u32 blk_size;
+};
+
+struct adm_chan {
+	struct virt_dma_chan vc;
+	struct adm_device *adev;
+
+	/* parsed from DT */
+	u32 id;			/* channel id */
+
+	struct adm_async_desc *curr_txd;
+	struct dma_slave_config slave;
+	struct list_head node;
+
+	int error;
+	int initialized;
+};
+
+static inline struct adm_chan *to_adm_chan(struct dma_chan *common)
+{
+	return container_of(common, struct adm_chan, vc.chan);
+}
+
+struct adm_device {
+	void __iomem *regs;
+	struct device *dev;
+	struct dma_device common;
+	struct device_dma_parameters dma_parms;
+	struct adm_chan *channels;
+
+	u32 ee;
+
+	struct clk *core_clk;
+	struct clk *iface_clk;
+
+	struct reset_control *clk_reset;
+	struct reset_control *c0_reset;
+	struct reset_control *c1_reset;
+	struct reset_control *c2_reset;
+	int irq;
+};
+
+/**
+ * adm_free_chan - Frees dma resources associated with the specific channel
+ *
+ * Free all allocated descriptors associated with this channel
+ *
+ */
+static void adm_free_chan(struct dma_chan *chan)
+{
+	/* free all queued descriptors */
+	vchan_free_chan_resources(to_virt_chan(chan));
+}
+
+/**
+ * adm_get_blksize - Get block size from burst value
+ *
+ */
+static int adm_get_blksize(unsigned int burst)
+{
+	int ret;
+
+	switch (burst) {
+	case 16:
+	case 32:
+	case 64:
+	case 128:
+		ret = ffs(burst>>4) - 1;
+		break;
+	case 192:
+		ret = 4;
+		break;
+	case 256:
+		ret = 5;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+/**
+ * adm_process_fc_descriptors - Process descriptors for flow controlled xfers
+ *
+ * @achan: ADM channel
+ * @desc: Descriptor memory pointer
+ * @sg: Scatterlist entry
+ * @crci: CRCI value
+ * @burst: Burst size of transaction
+ * @direction: DMA transfer direction
+ */
+static void *adm_process_fc_descriptors(struct adm_chan *achan,
+	void *desc, struct scatterlist *sg, u32 crci, u32 burst,
+	enum dma_transfer_direction direction)
+{
+	struct adm_desc_hw_box *box_desc = NULL;
+	struct adm_desc_hw_single *single_desc;
+	u32 remainder = sg_dma_len(sg);
+	u32 rows, row_offset, crci_cmd;
+	u32 mem_addr = sg_dma_address(sg);
+	u32 *incr_addr = &mem_addr;
+	u32 *src, *dst;
+
+	if (direction == DMA_DEV_TO_MEM) {
+		crci_cmd = ADM_CMD_SRC_CRCI(crci);
+		row_offset = burst;
+		src = &achan->slave.src_addr;
+		dst = &mem_addr;
+	} else {
+		crci_cmd = ADM_CMD_DST_CRCI(crci);
+		row_offset = burst << 16;
+		src = &mem_addr;
+		dst = &achan->slave.dst_addr;
+	}
+
+	while (remainder >= burst) {
+		box_desc = desc;
+		box_desc->cmd = ADM_CMD_TYPE_BOX | crci_cmd;
+		box_desc->row_offset = row_offset;
+		box_desc->src_addr = *src;
+		box_desc->dst_addr = *dst;
+
+		rows = remainder / burst;
+		rows = min_t(u32, rows, ADM_MAX_ROWS);
+		box_desc->num_rows = rows << 16 | rows;
+		box_desc->row_len = burst << 16 | burst;
+
+		*incr_addr += burst * rows;
+		remainder -= burst * rows;
+		desc += sizeof(*box_desc);
+	}
+
+	/* if leftover bytes, do one single descriptor */
+	if (remainder) {
+		single_desc = desc;
+		single_desc->cmd = ADM_CMD_TYPE_SINGLE | crci_cmd;
+		single_desc->len = remainder;
+		single_desc->src_addr = *src;
+		single_desc->dst_addr = *dst;
+		desc += sizeof(*single_desc);
+
+		if (sg_is_last(sg))
+			single_desc->cmd |= ADM_CMD_LC;
+	} else {
+		if (box_desc && sg_is_last(sg))
+			box_desc->cmd |= ADM_CMD_LC;
+	}
+
+	return desc;
+}
+
+/**
+ * adm_process_non_fc_descriptors - Process descriptors for non-fc xfers
+ *
+ * @achan: ADM channel
+ * @desc: Descriptor memory pointer
+ * @sg: Scatterlist entry
+ * @direction: DMA transfer direction
+ */
+static void *adm_process_non_fc_descriptors(struct adm_chan *achan,
+	void *desc, struct scatterlist *sg,
+	enum dma_transfer_direction direction)
+{
+	struct adm_desc_hw_single *single_desc;
+	u32 remainder = sg_dma_len(sg);
+	u32 mem_addr = sg_dma_address(sg);
+	u32 *incr_addr = &mem_addr;
+	u32 *src, *dst;
+
+	if (direction == DMA_DEV_TO_MEM) {
+		src = &achan->slave.src_addr;
+		dst = &mem_addr;
+	} else {
+		src = &mem_addr;
+		dst = &achan->slave.dst_addr;
+	}
+
+	do {
+		single_desc = desc;
+		single_desc->cmd = ADM_CMD_TYPE_SINGLE;
+		single_desc->src_addr = *src;
+		single_desc->dst_addr = *dst;
+		single_desc->len = (remainder > ADM_MAX_XFER) ?
+				ADM_MAX_XFER : remainder;
+
+		remainder -= single_desc->len;
+		*incr_addr += single_desc->len;
+		desc += sizeof(*single_desc);
+	} while (remainder);
+
+	/* set last command if this is the end of the whole transaction */
+	if (sg_is_last(sg))
+		single_desc->cmd |= ADM_CMD_LC;
+
+	return desc;
+}
+
+/**
+ * adm_prep_slave_sg - Prep slave sg transaction
+ *
+ * @chan: dma channel
+ * @sgl: scatter gather list
+ * @sg_len: length of sg
+ * @direction: DMA transfer direction
+ * @flags: DMA flags
+ * @context: transfer context (unused)
+ */
+static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan,
+	struct scatterlist *sgl, unsigned int sg_len,
+	enum dma_transfer_direction direction, unsigned long flags,
+	void *context)
+{
+	struct adm_chan *achan = to_adm_chan(chan);
+	struct adm_device *adev = achan->adev;
+	struct adm_async_desc *async_desc;
+	struct scatterlist *sg;
+	u32 i, burst;
+	u32 single_count = 0, box_count = 0, crci = 0;
+	void *desc;
+	u32 *cple;
+	int blk_size = 0;
+
+	if (!is_slave_direction(direction)) {
+		dev_err(adev->dev, "invalid dma direction\n");
+		return NULL;
+	}
+
+	/*
+	 * get burst value from slave configuration
+	 */
+	burst = (direction == DMA_MEM_TO_DEV) ?
+		achan->slave.dst_maxburst :
+		achan->slave.src_maxburst;
+
+	/* if using flow control, validate burst and crci values */
+	if (achan->slave.device_fc) {
+
+		blk_size = adm_get_blksize(burst);
+		if (blk_size < 0) {
+			dev_err(adev->dev, "invalid burst value: %d\n",
+				burst);
+			return ERR_PTR(-EINVAL);
+		}
+
+		crci = achan->slave.slave_id & 0xf;
+		if (!crci || achan->slave.slave_id > 0x1f) {
+			dev_err(adev->dev, "invalid crci value\n");
+			return ERR_PTR(-EINVAL);
+		}
+	}
+
+	/* iterate through sgs and compute allocation size of structures */
+	for_each_sg(sgl, sg, sg_len, i) {
+		if (achan->slave.device_fc) {
+			box_count += DIV_ROUND_UP(sg_dma_len(sg) / burst,
+						  ADM_MAX_ROWS);
+			if (sg_dma_len(sg) % burst)
+				single_count++;
+		} else {
+			single_count += DIV_ROUND_UP(sg_dma_len(sg),
+						     ADM_MAX_XFER);
+		}
+	}
+
+	async_desc = kzalloc(sizeof(*async_desc), GFP_NOWAIT);
+	if (!async_desc)
+		return ERR_PTR(-ENOMEM);
+
+	if (crci)
+		async_desc->mux = achan->slave.slave_id & ADM_CRCI_MUX_SEL ?
+					ADM_CRCI_CTL_MUX_SEL : 0;
+	async_desc->crci = crci;
+	async_desc->blk_size = blk_size;
+	async_desc->dma_len = single_count * sizeof(struct adm_desc_hw_single) +
+				box_count * sizeof(struct adm_desc_hw_box) +
+				sizeof(*cple) + 2 * ADM_DESC_ALIGN;
+
+	async_desc->cpl = dma_alloc_writecombine(adev->dev, async_desc->dma_len,
+				&async_desc->dma_addr, GFP_NOWAIT);
+
+	if (!async_desc->cpl) {
+		kfree(async_desc);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	async_desc->adev = adev;
+
+	/* both command list entry and descriptors must be 8 byte aligned */
+	cple = PTR_ALIGN(async_desc->cpl, ADM_DESC_ALIGN);
+	desc = PTR_ALIGN(cple + 1, ADM_DESC_ALIGN);
+
+	/* init cmd list */
+	*cple = ADM_CPLE_LP;
+	*cple |= (desc - async_desc->cpl + async_desc->dma_addr) >> 3;
+
+	for_each_sg(sgl, sg, sg_len, i) {
+		async_desc->length += sg_dma_len(sg);
+
+		if (achan->slave.device_fc)
+			desc = adm_process_fc_descriptors(achan, desc, sg, crci,
+							burst, direction);
+		else
+			desc = adm_process_non_fc_descriptors(achan, desc, sg,
+							   direction);
+	}
+
+	return vchan_tx_prep(&achan->vc, &async_desc->vd, flags);
+}
+
+/**
+ * adm_terminate_all - terminate all transactions on a channel
+ * @achan: adm dma channel
+ *
+ * Dequeues and frees all transactions, aborts current transaction
+ * No callbacks are done
+ *
+ */
+static int adm_terminate_all(struct dma_chan *chan)
+{
+	struct adm_chan *achan = to_adm_chan(chan);
+	struct adm_device *adev = achan->adev;
+	unsigned long flags;
+	LIST_HEAD(head);
+
+	spin_lock_irqsave(&achan->vc.lock, flags);
+	vchan_get_all_descriptors(&achan->vc, &head);
+
+	/* send flush command to terminate current transaction */
+	writel_relaxed(0x0,
+		adev->regs + ADM_CH_FLUSH_STATE0(achan->id, adev->ee));
+
+	spin_unlock_irqrestore(&achan->vc.lock, flags);
+
+	vchan_dma_desc_free_list(&achan->vc, &head);
+
+	return 0;
+}
+
+static int adm_slave_config(struct dma_chan *chan, struct dma_slave_config *cfg)
+{
+	struct adm_chan *achan = to_adm_chan(chan);
+	unsigned long flag;
+
+	spin_lock_irqsave(&achan->vc.lock, flag);
+	memcpy(&achan->slave, cfg, sizeof(struct dma_slave_config));
+	spin_unlock_irqrestore(&achan->vc.lock, flag);
+
+	return 0;
+}
+
+/**
+ * adm_start_dma - start next transaction
+ * @achan - ADM dma channel
+ */
+static void adm_start_dma(struct adm_chan *achan)
+{
+	struct virt_dma_desc *vd = vchan_next_desc(&achan->vc);
+	struct adm_device *adev = achan->adev;
+	struct adm_async_desc *async_desc;
+
+	lockdep_assert_held(&achan->vc.lock);
+
+	if (!vd)
+		return;
+
+	list_del(&vd->node);
+
+	/* write next command list out to the CMD FIFO */
+	async_desc = container_of(vd, struct adm_async_desc, vd);
+	achan->curr_txd = async_desc;
+
+	/* reset channel error */
+	achan->error = 0;
+
+	if (!achan->initialized) {
+		/* enable interrupts */
+		writel(ADM_CH_CONF_SHADOW_EN |
+		       ADM_CH_CONF_PERM_MPU_CONF |
+		       ADM_CH_CONF_MPU_DISABLE |
+		       ADM_CH_CONF_SEC_DOMAIN(adev->ee),
+		       adev->regs + ADM_CH_CONF(achan->id));
+
+		writel(ADM_CH_RSLT_CONF_IRQ_EN | ADM_CH_RSLT_CONF_FLUSH_EN,
+			adev->regs + ADM_CH_RSLT_CONF(achan->id, adev->ee));
+
+		achan->initialized = 1;
+	}
+
+	/* set the crci block size if this transaction requires CRCI */
+	if (async_desc->crci) {
+		writel(async_desc->mux | async_desc->blk_size,
+			adev->regs + ADM_CRCI_CTL(async_desc->crci, adev->ee));
+	}
+
+	/* make sure IRQ enable doesn't get reordered */
+	wmb();
+
+	/* write next command list out to the CMD FIFO */
+	writel(ALIGN(async_desc->dma_addr, ADM_DESC_ALIGN) >> 3,
+		adev->regs + ADM_CH_CMD_PTR(achan->id, adev->ee));
+}
+
+/**
+ * adm_dma_irq - irq handler for ADM controller
+ * @irq: IRQ of interrupt
+ * @data: callback data
+ *
+ * IRQ handler for the bam controller
+ */
+static irqreturn_t adm_dma_irq(int irq, void *data)
+{
+	struct adm_device *adev = data;
+	u32 srcs, i;
+	struct adm_async_desc *async_desc;
+	unsigned long flags;
+
+	srcs = readl_relaxed(adev->regs +
+			ADM_SEC_DOMAIN_IRQ_STATUS(adev->ee));
+
+	for (i = 0; i < ADM_MAX_CHANNELS; i++) {
+		struct adm_chan *achan = &adev->channels[i];
+		u32 status, result;
+
+		if (srcs & BIT(i)) {
+			status = readl_relaxed(adev->regs +
+				ADM_CH_STATUS_SD(i, adev->ee));
+
+			/* if no result present, skip */
+			if (!(status & ADM_CH_STATUS_VALID))
+				continue;
+
+			result = readl_relaxed(adev->regs +
+				ADM_CH_RSLT(i, adev->ee));
+
+			/* no valid results, skip */
+			if (!(result & ADM_CH_RSLT_VALID))
+				continue;
+
+			/* flag error if transaction was flushed or failed */
+			if (result & (ADM_CH_RSLT_ERR | ADM_CH_RSLT_FLUSH))
+				achan->error = 1;
+
+			spin_lock_irqsave(&achan->vc.lock, flags);
+			async_desc = achan->curr_txd;
+
+			achan->curr_txd = NULL;
+
+			if (async_desc) {
+				vchan_cookie_complete(&async_desc->vd);
+
+				/* kick off next DMA */
+				adm_start_dma(achan);
+			}
+
+			spin_unlock_irqrestore(&achan->vc.lock, flags);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * adm_tx_status - returns status of transaction
+ * @chan: dma channel
+ * @cookie: transaction cookie
+ * @txstate: DMA transaction state
+ *
+ * Return status of dma transaction
+ */
+static enum dma_status adm_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
+	struct dma_tx_state *txstate)
+{
+	struct adm_chan *achan = to_adm_chan(chan);
+	struct virt_dma_desc *vd;
+	enum dma_status ret;
+	unsigned long flags;
+	size_t residue = 0;
+
+	ret = dma_cookie_status(chan, cookie, txstate);
+	if (ret == DMA_COMPLETE || !txstate)
+		return ret;
+
+	spin_lock_irqsave(&achan->vc.lock, flags);
+
+	vd = vchan_find_desc(&achan->vc, cookie);
+	if (vd)
+		residue = container_of(vd, struct adm_async_desc, vd)->length;
+
+	spin_unlock_irqrestore(&achan->vc.lock, flags);
+
+	/*
+	 * residue is either the full length if it is in the issued list, or 0
+	 * if it is in progress.  We have no reliable way of determining
+	 * anything inbetween
+	*/
+	dma_set_residue(txstate, residue);
+
+	if (achan->error)
+		return DMA_ERROR;
+
+	return ret;
+}
+
+/**
+ * adm_issue_pending - starts pending transactions
+ * @chan: dma channel
+ *
+ * Issues all pending transactions and starts DMA
+ */
+static void adm_issue_pending(struct dma_chan *chan)
+{
+	struct adm_chan *achan = to_adm_chan(chan);
+	unsigned long flags;
+
+	spin_lock_irqsave(&achan->vc.lock, flags);
+
+	if (vchan_issue_pending(&achan->vc) && !achan->curr_txd)
+		adm_start_dma(achan);
+	spin_unlock_irqrestore(&achan->vc.lock, flags);
+}
+
+/**
+ * adm_dma_free_desc - free descriptor memory
+ * @vd: virtual descriptor
+ *
+ */
+static void adm_dma_free_desc(struct virt_dma_desc *vd)
+{
+	struct adm_async_desc *async_desc = container_of(vd,
+			struct adm_async_desc, vd);
+
+	dma_free_writecombine(async_desc->adev->dev, async_desc->dma_len,
+		async_desc->cpl, async_desc->dma_addr);
+	kfree(async_desc);
+}
+
+static void adm_channel_init(struct adm_device *adev, struct adm_chan *achan,
+	u32 index)
+{
+	achan->id = index;
+	achan->adev = adev;
+
+	vchan_init(&achan->vc, &adev->common);
+	achan->vc.desc_free = adm_dma_free_desc;
+}
+
+static int adm_dma_probe(struct platform_device *pdev)
+{
+	struct adm_device *adev;
+	struct resource *iores;
+	int ret;
+	u32 i;
+
+	adev = devm_kzalloc(&pdev->dev, sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->dev = &pdev->dev;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	adev->regs = devm_ioremap_resource(&pdev->dev, iores);
+	if (IS_ERR(adev->regs))
+		return PTR_ERR(adev->regs);
+
+	adev->irq = platform_get_irq(pdev, 0);
+	if (adev->irq < 0)
+		return adev->irq;
+
+	ret = of_property_read_u32(pdev->dev.of_node, "qcom,ee", &adev->ee);
+	if (ret) {
+		dev_err(adev->dev, "Execution environment unspecified\n");
+		return ret;
+	}
+
+	adev->core_clk = devm_clk_get(adev->dev, "core");
+	if (IS_ERR(adev->core_clk))
+		return PTR_ERR(adev->core_clk);
+
+	ret = clk_prepare_enable(adev->core_clk);
+	if (ret) {
+		dev_err(adev->dev, "failed to prepare/enable core clock\n");
+		return ret;
+	}
+
+	adev->iface_clk = devm_clk_get(adev->dev, "iface");
+	if (IS_ERR(adev->iface_clk)) {
+		ret = PTR_ERR(adev->iface_clk);
+		goto err_disable_core_clk;
+	}
+
+	ret = clk_prepare_enable(adev->iface_clk);
+	if (ret) {
+		dev_err(adev->dev, "failed to prepare/enable iface clock\n");
+		goto err_disable_core_clk;
+	}
+
+	adev->clk_reset = devm_reset_control_get(&pdev->dev, "clk");
+	if (IS_ERR(adev->clk_reset)) {
+		dev_err(adev->dev, "failed to get ADM0 reset\n");
+		ret = PTR_ERR(adev->clk_reset);
+		goto err_disable_clks;
+	}
+
+	adev->c0_reset = devm_reset_control_get(&pdev->dev, "c0");
+	if (IS_ERR(adev->c0_reset)) {
+		dev_err(adev->dev, "failed to get ADM0 C0 reset\n");
+		ret = PTR_ERR(adev->c0_reset);
+		goto err_disable_clks;
+	}
+
+	adev->c1_reset = devm_reset_control_get(&pdev->dev, "c1");
+	if (IS_ERR(adev->c1_reset)) {
+		dev_err(adev->dev, "failed to get ADM0 C1 reset\n");
+		ret = PTR_ERR(adev->c1_reset);
+		goto err_disable_clks;
+	}
+
+	adev->c2_reset = devm_reset_control_get(&pdev->dev, "c2");
+	if (IS_ERR(adev->c2_reset)) {
+		dev_err(adev->dev, "failed to get ADM0 C2 reset\n");
+		ret = PTR_ERR(adev->c2_reset);
+		goto err_disable_clks;
+	}
+
+	reset_control_assert(adev->clk_reset);
+	reset_control_assert(adev->c0_reset);
+	reset_control_assert(adev->c1_reset);
+	reset_control_assert(adev->c2_reset);
+
+	reset_control_deassert(adev->clk_reset);
+	reset_control_deassert(adev->c0_reset);
+	reset_control_deassert(adev->c1_reset);
+	reset_control_deassert(adev->c2_reset);
+
+	adev->channels = devm_kcalloc(adev->dev, ADM_MAX_CHANNELS,
+				sizeof(*adev->channels), GFP_KERNEL);
+
+	if (!adev->channels) {
+		ret = -ENOMEM;
+		goto err_disable_clks;
+	}
+
+	/* allocate and initialize channels */
+	INIT_LIST_HEAD(&adev->common.channels);
+
+	for (i = 0; i < ADM_MAX_CHANNELS; i++)
+		adm_channel_init(adev, &adev->channels[i], i);
+
+	/* reset CRCIs */
+	for (i = 0; i < 16; i++)
+		writel(ADM_CRCI_CTL_RST, adev->regs +
+			ADM_CRCI_CTL(i, adev->ee));
+
+	/* configure client interfaces */
+	writel(ADM_CI_RANGE_START(0x40) | ADM_CI_RANGE_END(0xb0) |
+		ADM_CI_BURST_8_WORDS, adev->regs + ADM_CI_CONF(0));
+	writel(ADM_CI_RANGE_START(0x2a) | ADM_CI_RANGE_END(0x2c) |
+		ADM_CI_BURST_8_WORDS, adev->regs + ADM_CI_CONF(1));
+	writel(ADM_CI_RANGE_START(0x12) | ADM_CI_RANGE_END(0x28) |
+		ADM_CI_BURST_8_WORDS, adev->regs + ADM_CI_CONF(2));
+	writel(ADM_GP_CTL_LP_EN | ADM_GP_CTL_LP_CNT(0xf),
+		adev->regs + ADM_GP_CTL);
+
+	ret = devm_request_irq(adev->dev, adev->irq, adm_dma_irq,
+			0, "adm_dma", adev);
+	if (ret)
+		goto err_disable_clks;
+
+	platform_set_drvdata(pdev, adev);
+
+	adev->common.dev = adev->dev;
+	adev->common.dev->dma_parms = &adev->dma_parms;
+
+	/* set capabilities */
+	dma_cap_zero(adev->common.cap_mask);
+	dma_cap_set(DMA_SLAVE, adev->common.cap_mask);
+	dma_cap_set(DMA_PRIVATE, adev->common.cap_mask);
+
+	/* initialize dmaengine apis */
+	adev->common.directions = BIT(DMA_DEV_TO_MEM | DMA_MEM_TO_DEV);
+	adev->common.residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+	adev->common.src_addr_widths = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	adev->common.dst_addr_widths = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	adev->common.device_free_chan_resources = adm_free_chan;
+	adev->common.device_prep_slave_sg = adm_prep_slave_sg;
+	adev->common.device_issue_pending = adm_issue_pending;
+	adev->common.device_tx_status = adm_tx_status;
+	adev->common.device_terminate_all = adm_terminate_all;
+	adev->common.device_config = adm_slave_config;
+
+	ret = dma_async_device_register(&adev->common);
+	if (ret) {
+		dev_err(adev->dev, "failed to register dma async device\n");
+		goto err_disable_clks;
+	}
+
+	ret = of_dma_controller_register(pdev->dev.of_node,
+					 of_dma_xlate_by_chan_id,
+					 &adev->common);
+	if (ret)
+		goto err_unregister_dma;
+
+	return 0;
+
+err_unregister_dma:
+	dma_async_device_unregister(&adev->common);
+err_disable_clks:
+	clk_disable_unprepare(adev->iface_clk);
+err_disable_core_clk:
+	clk_disable_unprepare(adev->core_clk);
+
+	return ret;
+}
+
+static int adm_dma_remove(struct platform_device *pdev)
+{
+	struct adm_device *adev = platform_get_drvdata(pdev);
+	struct adm_chan *achan;
+	u32 i;
+
+	of_dma_controller_free(pdev->dev.of_node);
+	dma_async_device_unregister(&adev->common);
+
+	for (i = 0; i < ADM_MAX_CHANNELS; i++) {
+		achan = &adev->channels[i];
+
+		/* mask IRQs for this channel/EE pair */
+		writel(0, adev->regs + ADM_CH_RSLT_CONF(achan->id, adev->ee));
+
+		adm_terminate_all(&adev->channels[i].vc.chan);
+	}
+
+	devm_free_irq(adev->dev, adev->irq, adev);
+
+	clk_disable_unprepare(adev->core_clk);
+	clk_disable_unprepare(adev->iface_clk);
+
+	return 0;
+}
+
+static const struct of_device_id adm_of_match[] = {
+	{ .compatible = "qcom,adm", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, adm_of_match);
+
+static struct platform_driver adm_dma_driver = {
+	.probe = adm_dma_probe,
+	.remove = adm_dma_remove,
+	.driver = {
+		.name = "adm-dma-engine",
+		.of_match_table = adm_of_match,
+	},
+};
+
+module_platform_driver(adm_dma_driver);
+
+MODULE_AUTHOR("Andy Gross <agross@codeaurora.org>");
+MODULE_DESCRIPTION("QCOM ADM DMA engine driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH 3/5] arm: qcom: dts: ipq8064: Add ADM device node

[Thomas Pedersen]

From: Andy Gross <andy.gross@linaro.org>

Signed-off-by: Andy Gross <andy.gross@linaro.org>
Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 2601a90..1a3549f 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -1,9 +1,11 @@
 /dts-v1/;
 
 #include "skeleton.dtsi"
+#include <dt-bindings/reset/qcom,gcc-ipq806x.h>
 #include <dt-bindings/clock/qcom,gcc-ipq806x.h>
 #include <dt-bindings/clock/qcom,lcc-ipq806x.h>
 #include <dt-bindings/soc/qcom,gsbi.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 
 / {
 	model = "Qualcomm IPQ8064";
@@ -341,5 +343,24 @@
 			#reset-cells = <1>;
 		};
 
+		adm_dma: dma@18300000 {
+			compatible = "qcom,adm";
+			reg = <0x18300000 0x100000>;
+			interrupts = <GIC_SPI 170 IRQ_TYPE_NONE>;
+			#dma-cells = <1>;
+
+			clocks = <&gcc ADM0_CLK>, <&gcc ADM0_PBUS_CLK>;
+			clock-names = "core", "iface";
+
+			resets = <&gcc ADM0_RESET>,
+				 <&gcc ADM0_PBUS_RESET>,
+				 <&gcc ADM0_C0_RESET>,
+				 <&gcc ADM0_C1_RESET>,
+				 <&gcc ADM0_C2_RESET>;
+			reset-names = "clk", "pbus", "c0", "c1", "c2";
+			qcom,ee = <0>;
+
+			status = "disabled";
+		};
 	};
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH 4/5] arm: qcom: dts: Add NAND controller node for ipq806x

[Thomas Pedersen]

From: Archit Taneja <architt@codeaurora.org>

Signed-off-by: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 1a3549f..3183e66 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -362,5 +362,22 @@
 
 			status = "disabled";
 		};
+
+		nand@1ac00000 {
+			compatible = "qcom,ipq806x-nand";
+			reg = <0x1ac00000 0x800>;
+
+			clocks = <&gcc EBI2_CLK>,
+				 <&gcc EBI2_AON_CLK>;
+			clock-names = "core", "aon";
+
+			dmas = <&adm_dma 3>;
+			dma-names = "rxtx";
+			qcom,cmd-crci = <15>;
+			qcom,data-crci = <3>;
+
+			status = "disabled";
+		};
+
 	};
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[RESEND PATCH 5/5] arm: qcom: dts: Enable NAND node on IPQ8064 AP148 platform

[Thomas Pedersen]

From: Archit Taneja <architt@codeaurora.org>

Enable the NAND controller node on the AP148 platform. Provide pinmux
information.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
---
 arch/arm/boot/dts/qcom-ipq8064-ap148.dts | 42 ++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-ipq8064-ap148.dts b/arch/arm/boot/dts/qcom-ipq8064-ap148.dts
index d501382..0a4b5e3 100644
--- a/arch/arm/boot/dts/qcom-ipq8064-ap148.dts
+++ b/arch/arm/boot/dts/qcom-ipq8064-ap148.dts
@@ -38,6 +38,28 @@
 					bias-none;
 				};
 			};
+			nand_pins: nand_pins {
+				mux {
+					pins = "gpio34", "gpio35", "gpio36",
+					       "gpio37", "gpio38", "gpio39",
+					       "gpio40", "gpio41", "gpio42",
+					       "gpio43", "gpio44", "gpio45",
+					       "gpio46", "gpio47";
+					function = "nand";
+					drive-strength = <10>;
+					bias-disable;
+				};
+				pullups {
+					pins = "gpio39";
+					bias-pull-up;
+				};
+				hold {
+					pins = "gpio40", "gpio41", "gpio42",
+					       "gpio43", "gpio44", "gpio45",
+					       "gpio46", "gpio47";
+					bias-bus-hold;
+				};
+			};
 		};
 
 		gsbi@16300000 {
@@ -97,5 +119,25 @@
 		sata@29000000 {
 			status = "ok";
 		};
+
+		nand@1ac00000 {
+			status = "ok";
+
+			pinctrl-0 = <&nand_pins>;
+			pinctrl-names = "default";
+
+			nandcs@0 {
+				compatible = "qcom,nandcs";
+				reg = <0>;
+
+				nand-ecc-strength = <4>;
+				nand-ecc-step-size = <512>;
+				nand-bus-width = <8>;
+			};
+		};
 	};
 };
+
+&adm_dma {
+	status = "ok";
+};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers

[Andy Gross]

On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote:
> From: Andy Gross <andy.gross@linaro.org>
> 
> This patch removes the crci information from the dma
> channel property.  At least one client device requires
> using more than one CRCI value for a channel.  This does
> not match the current binding and the crci information
> needs to be removed.
> 
> Instead, the client device will provide this information
> via other means.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt b/Documentation/devicetree/bindings/dma/qcom_adm.txt
> index 9bcab91..38d45f8 100644
> --- a/Documentation/devicetree/bindings/dma/qcom_adm.txt
> +++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt
> @@ -4,8 +4,7 @@ Required properties:
>  - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960
>  - reg: Address range for DMA registers
>  - interrupts: Should contain one interrupt shared by all channels
> -- #dma-cells: must be <2>.  First cell denotes the channel number.  Second cell
> -  denotes CRCI (client rate control interface) flow control assignment.
> +- #dma-cells: must be <1>.  First cell denotes the channel number.

I've actually been thinking more about this.  The crci being specified in the
slave config is probably the wrong approach.  What we really need is each
physical DMA channel to allow for virtual channels that allow for a CRCI setting
(0 being no flow control).  This would require the properties to continue to be
2 for the dma definition.

This would also require clients to get multiple references to the same DMA
channel if they require some communications with and without flow control.

For NAND, this would mean having two channels for dma.  One for flow controlled
and one without.

Anyone have reservations with this?


Regards,

Andy

Re: [RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers

[Thomas Pedersen]

Hi Andy,

On 2016-06-29 14:06, Andy Gross wrote:
> On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote:
>> From: Andy Gross <andy.gross@linaro.org>
>> 
>> This patch removes the crci information from the dma
>> channel property.  At least one client device requires
>> using more than one CRCI value for a channel.  This does
>> not match the current binding and the crci information
>> needs to be removed.
>> 
>> Instead, the client device will provide this information
>> via other means.
>> 
>> Signed-off-by: Andy Gross <andy.gross@linaro.org>
>> Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 
>> ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt 
>> b/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> index 9bcab91..38d45f8 100644
>> --- a/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> +++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> @@ -4,8 +4,7 @@ Required properties:
>>  - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960
>>  - reg: Address range for DMA registers
>>  - interrupts: Should contain one interrupt shared by all channels
>> -- #dma-cells: must be <2>.  First cell denotes the channel number.  
>> Second cell
>> -  denotes CRCI (client rate control interface) flow control 
>> assignment.
>> +- #dma-cells: must be <1>.  First cell denotes the channel number.
> 
> I've actually been thinking more about this.  The crci being specified 
> in the
> slave config is probably the wrong approach.  What we really need is 
> each
> physical DMA channel to allow for virtual channels that allow for a 
> CRCI setting
> (0 being no flow control).  This would require the properties to 
> continue to be
> 2 for the dma definition.

AFAICT the cmd-crci and data-crci properties in the NAND controller 
client are
really just the slave_id for the DMA engine. Flow control is decided by 
some
other heuristic such as whether it's a read/write/etc. command.

> This would also require clients to get multiple references to the same 
> DMA
> channel if they require some communications with and without flow 
> control.
> 
> For NAND, this would mean having two channels for dma.  One for flow 
> controlled
> and one without.

So the NAND DT would look something like:

	dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>;
	dma-names = "rxtx", "rxtx_fc";
	qcom,cmd-crci = <15>;
	qcom,data-crci = <3>;

? We'd still need the cmd-crci and data-crci properties for the 
slave_id, unless
I'm confusing something?

Thanks,

-- 
thomas

Re: [RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers

[Andy Gross]

On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote:
> Hi Andy,
> 
> On 2016-06-29 14:06, Andy Gross wrote:
> >On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote:
> >>From: Andy Gross <andy.gross@linaro.org>
> >>
> >>This patch removes the crci information from the dma
> >>channel property.  At least one client device requires
> >>using more than one CRCI value for a channel.  This does
> >>not match the current binding and the crci information
> >>needs to be removed.
> >>
> >>Instead, the client device will provide this information
> >>via other means.
> >>
> >>Signed-off-by: Andy Gross <andy.gross@linaro.org>
> >>Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
> >>---
> >> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16
> >>++++++----------
> >> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt
> >>b/Documentation/devicetree/bindings/dma/qcom_adm.txt
> >>index 9bcab91..38d45f8 100644
> >>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt
> >>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt
> >>@@ -4,8 +4,7 @@ Required properties:
> >> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960
> >> - reg: Address range for DMA registers
> >> - interrupts: Should contain one interrupt shared by all channels
> >>-- #dma-cells: must be <2>.  First cell denotes the channel number.
> >>Second cell
> >>-  denotes CRCI (client rate control interface) flow control assignment.
> >>+- #dma-cells: must be <1>.  First cell denotes the channel number.
> >
> >I've actually been thinking more about this.  The crci being specified in
> >the
> >slave config is probably the wrong approach.  What we really need is each
> >physical DMA channel to allow for virtual channels that allow for a CRCI
> >setting
> >(0 being no flow control).  This would require the properties to continue
> >to be
> >2 for the dma definition.
> 
> AFAICT the cmd-crci and data-crci properties in the NAND controller client
> are
> really just the slave_id for the DMA engine. Flow control is decided by some
> other heuristic such as whether it's a read/write/etc. command.
> 
> >This would also require clients to get multiple references to the same DMA
> >channel if they require some communications with and without flow control.
> >
> >For NAND, this would mean having two channels for dma.  One for flow
> >controlled
> >and one without.
> 
> So the NAND DT would look something like:
> 
> 	dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>;
> 	dma-names = "rxtx", "rxtx_fc";
> 	qcom,cmd-crci = <15>;
> 	qcom,data-crci = <3>;
> 
> ? We'd still need the cmd-crci and data-crci properties for the slave_id,
> unless
> I'm confusing something?

No, what we would have are separate channels for cmd and data that would be
virtual channels sharing the same hardware channel.  At least that is what I am
thinking.   So 

dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>;
dma-names = "rxtx", "rxtx-cmd", "rxtx-data";     # insert better names here 

All three would use channel 3, but the virtual channel would know about the
crci.  CRCI = 0 is no flow control.

I need to look at the nand a little harder to see how they formulate the dma
requests.

Regards,

Andy

Re: [RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers

[Thomas Pedersen]

On 2016-07-01 12:08, Andy Gross wrote:
> On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote:
>> Hi Andy,
>> 
>> On 2016-06-29 14:06, Andy Gross wrote:
>> >On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote:
>> >>From: Andy Gross <andy.gross@linaro.org>
>> >>
>> >>This patch removes the crci information from the dma
>> >>channel property.  At least one client device requires
>> >>using more than one CRCI value for a channel.  This does
>> >>not match the current binding and the crci information
>> >>needs to be removed.
>> >>
>> >>Instead, the client device will provide this information
>> >>via other means.
>> >>
>> >>Signed-off-by: Andy Gross <andy.gross@linaro.org>
>> >>Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
>> >>---
>> >> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16
>> >>++++++----------
>> >> 1 file changed, 6 insertions(+), 10 deletions(-)
>> >>
>> >>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> >>b/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> >>index 9bcab91..38d45f8 100644
>> >>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> >>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> >>@@ -4,8 +4,7 @@ Required properties:
>> >> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960
>> >> - reg: Address range for DMA registers
>> >> - interrupts: Should contain one interrupt shared by all channels
>> >>-- #dma-cells: must be <2>.  First cell denotes the channel number.
>> >>Second cell
>> >>-  denotes CRCI (client rate control interface) flow control assignment.
>> >>+- #dma-cells: must be <1>.  First cell denotes the channel number.
>> >
>> >I've actually been thinking more about this.  The crci being specified in
>> >the
>> >slave config is probably the wrong approach.  What we really need is each
>> >physical DMA channel to allow for virtual channels that allow for a CRCI
>> >setting
>> >(0 being no flow control).  This would require the properties to continue
>> >to be
>> >2 for the dma definition.
>> 
>> AFAICT the cmd-crci and data-crci properties in the NAND controller 
>> client
>> are
>> really just the slave_id for the DMA engine. Flow control is decided 
>> by some
>> other heuristic such as whether it's a read/write/etc. command.
>> 
>> >This would also require clients to get multiple references to the same DMA
>> >channel if they require some communications with and without flow control.
>> >
>> >For NAND, this would mean having two channels for dma.  One for flow
>> >controlled
>> >and one without.
>> 
>> So the NAND DT would look something like:
>> 
>> 	dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>;
>> 	dma-names = "rxtx", "rxtx_fc";
>> 	qcom,cmd-crci = <15>;
>> 	qcom,data-crci = <3>;
>> 
>> ? We'd still need the cmd-crci and data-crci properties for the 
>> slave_id,
>> unless
>> I'm confusing something?
> 
> No, what we would have are separate channels for cmd and data that 
> would be
> virtual channels sharing the same hardware channel.  At least that is 
> what I am
> thinking.   So
> 
> dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>;
> dma-names = "rxtx", "rxtx-cmd", "rxtx-data";     # insert better names 
> here
> 
> All three would use channel 3, but the virtual channel would know about 
> the
> crci.  CRCI = 0 is no flow control.
> 
> I need to look at the nand a little harder to see how they formulate 
> the dma
> requests.

Did you have a chance to look into this? If not, can you provide some 
more
specific pointers (like would these virtual channels be in the NAND 
controller
or ADM driver?), and I can give it a try?

-- 
thomas

Re: [RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers

[Andy Gross]

On Thu, Jul 28, 2016 at 03:16:07PM -0700, Thomas Pedersen wrote:
> On 2016-07-01 12:08, Andy Gross wrote:
> >On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote:
> >>Hi Andy,
> >>
> >>On 2016-06-29 14:06, Andy Gross wrote:
> >>>On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote:
> >>>>From: Andy Gross <andy.gross@linaro.org>
> >>>>
> >>>>This patch removes the crci information from the dma
> >>>>channel property.  At least one client device requires
> >>>>using more than one CRCI value for a channel.  This does
> >>>>not match the current binding and the crci information
> >>>>needs to be removed.
> >>>>
> >>>>Instead, the client device will provide this information
> >>>>via other means.
> >>>>
> >>>>Signed-off-by: Andy Gross <andy.gross@linaro.org>
> >>>>Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
> >>>>---
> >>>> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16
> >>>>++++++----------
> >>>> 1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>
> >>>>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt
> >>>>b/Documentation/devicetree/bindings/dma/qcom_adm.txt
> >>>>index 9bcab91..38d45f8 100644
> >>>>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt
> >>>>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt
> >>>>@@ -4,8 +4,7 @@ Required properties:
> >>>> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960
> >>>> - reg: Address range for DMA registers
> >>>> - interrupts: Should contain one interrupt shared by all channels
> >>>>-- #dma-cells: must be <2>.  First cell denotes the channel number.
> >>>>Second cell
> >>>>-  denotes CRCI (client rate control interface) flow control assignment.
> >>>>+- #dma-cells: must be <1>.  First cell denotes the channel number.
> >>>
> >>>I've actually been thinking more about this.  The crci being specified in
> >>>the
> >>>slave config is probably the wrong approach.  What we really need is each
> >>>physical DMA channel to allow for virtual channels that allow for a CRCI
> >>>setting
> >>>(0 being no flow control).  This would require the properties to continue
> >>>to be
> >>>2 for the dma definition.
> >>
> >>AFAICT the cmd-crci and data-crci properties in the NAND controller
> >>client
> >>are
> >>really just the slave_id for the DMA engine. Flow control is decided by
> >>some
> >>other heuristic such as whether it's a read/write/etc. command.
> >>
> >>>This would also require clients to get multiple references to the same DMA
> >>>channel if they require some communications with and without flow control.
> >>>
> >>>For NAND, this would mean having two channels for dma.  One for flow
> >>>controlled
> >>>and one without.
> >>
> >>So the NAND DT would look something like:
> >>
> >>	dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>;
> >>	dma-names = "rxtx", "rxtx_fc";
> >>	qcom,cmd-crci = <15>;
> >>	qcom,data-crci = <3>;
> >>
> >>? We'd still need the cmd-crci and data-crci properties for the
> >>slave_id,
> >>unless
> >>I'm confusing something?
> >
> >No, what we would have are separate channels for cmd and data that would
> >be
> >virtual channels sharing the same hardware channel.  At least that is what
> >I am
> >thinking.   So
> >
> >dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>;
> >dma-names = "rxtx", "rxtx-cmd", "rxtx-data";     # insert better names
> >here
> >
> >All three would use channel 3, but the virtual channel would know about
> >the
> >crci.  CRCI = 0 is no flow control.
> >
> >I need to look at the nand a little harder to see how they formulate the
> >dma
> >requests.
> 
> Did you have a chance to look into this? If not, can you provide some more
> specific pointers (like would these virtual channels be in the NAND
> controller
> or ADM driver?), and I can give it a try?

The virtual channels would be in the ADM driver.  So when you specify the dmas
in the DT, you'd specify adm, channel X, crci Y.  This in turn would return a
dma channel that encapsulates the channel + crci.  This takes away the
requirement of having to do the slave_config.

As it is right now with the current ADM driver, you grab one channel and then
slave_config it before you add descriptors.  This simplifies the number of dma
channels, but complicates things by requiring you to constantly slave_config if
you are going from crci, to no crci.

Separating the channel+crci into a virtual channel means that the ADM driver
itself will collate the requests as they come in, and apply them to the actual
hardware channel.

That make sense?  If so, this requires modifying the DT doc to specify the crci
in the binding, modifying the ADM driver to identify/add channels based on the
channel + crci, and modifying the clients which use both flow control and
non-flow control.

Re: [RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers

[Thomas Pedersen]

On 2016-08-01 13:37, Andy Gross wrote:
> On Thu, Jul 28, 2016 at 03:16:07PM -0700, Thomas Pedersen wrote:
>> On 2016-07-01 12:08, Andy Gross wrote:
>> >On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote:
>> >>Hi Andy,
>> >>
>> >>On 2016-06-29 14:06, Andy Gross wrote:
>> >>>On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote:
>> >>>>From: Andy Gross <andy.gross@linaro.org>
>> >>>>
>> >>>>This patch removes the crci information from the dma
>> >>>>channel property.  At least one client device requires
>> >>>>using more than one CRCI value for a channel.  This does
>> >>>>not match the current binding and the crci information
>> >>>>needs to be removed.
>> >>>>
>> >>>>Instead, the client device will provide this information
>> >>>>via other means.
>> >>>>
>> >>>>Signed-off-by: Andy Gross <andy.gross@linaro.org>
>> >>>>Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
>> >>>>---
>> >>>> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16
>> >>>>++++++----------
>> >>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>> >>>>
>> >>>>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> >>>>b/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> >>>>index 9bcab91..38d45f8 100644
>> >>>>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> >>>>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt
>> >>>>@@ -4,8 +4,7 @@ Required properties:
>> >>>> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960
>> >>>> - reg: Address range for DMA registers
>> >>>> - interrupts: Should contain one interrupt shared by all channels
>> >>>>-- #dma-cells: must be <2>.  First cell denotes the channel number.
>> >>>>Second cell
>> >>>>-  denotes CRCI (client rate control interface) flow control assignment.
>> >>>>+- #dma-cells: must be <1>.  First cell denotes the channel number.
>> >>>
>> >>>I've actually been thinking more about this.  The crci being specified in
>> >>>the
>> >>>slave config is probably the wrong approach.  What we really need is each
>> >>>physical DMA channel to allow for virtual channels that allow for a CRCI
>> >>>setting
>> >>>(0 being no flow control).  This would require the properties to continue
>> >>>to be
>> >>>2 for the dma definition.
>> >>
>> >>AFAICT the cmd-crci and data-crci properties in the NAND controller
>> >>client
>> >>are
>> >>really just the slave_id for the DMA engine. Flow control is decided by
>> >>some
>> >>other heuristic such as whether it's a read/write/etc. command.
>> >>
>> >>>This would also require clients to get multiple references to the same DMA
>> >>>channel if they require some communications with and without flow control.
>> >>>
>> >>>For NAND, this would mean having two channels for dma.  One for flow
>> >>>controlled
>> >>>and one without.
>> >>
>> >>So the NAND DT would look something like:
>> >>
>> >>	dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>;
>> >>	dma-names = "rxtx", "rxtx_fc";
>> >>	qcom,cmd-crci = <15>;
>> >>	qcom,data-crci = <3>;
>> >>
>> >>? We'd still need the cmd-crci and data-crci properties for the
>> >>slave_id,
>> >>unless
>> >>I'm confusing something?
>> >
>> >No, what we would have are separate channels for cmd and data that would
>> >be
>> >virtual channels sharing the same hardware channel.  At least that is what
>> >I am
>> >thinking.   So
>> >
>> >dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>;
>> >dma-names = "rxtx", "rxtx-cmd", "rxtx-data";     # insert better names
>> >here
>> >
>> >All three would use channel 3, but the virtual channel would know about
>> >the
>> >crci.  CRCI = 0 is no flow control.
>> >
>> >I need to look at the nand a little harder to see how they formulate the
>> >dma
>> >requests.
>> 
>> Did you have a chance to look into this? If not, can you provide some 
>> more
>> specific pointers (like would these virtual channels be in the NAND
>> controller
>> or ADM driver?), and I can give it a try?
> 
> The virtual channels would be in the ADM driver.  So when you specify 
> the dmas
> in the DT, you'd specify adm, channel X, crci Y.  This in turn would 
> return a
> dma channel that encapsulates the channel + crci.  This takes away the
> requirement of having to do the slave_config.
> 
> As it is right now with the current ADM driver, you grab one channel 
> and then
> slave_config it before you add descriptors.  This simplifies the number 
> of dma
> channels, but complicates things by requiring you to constantly 
> slave_config if
> you are going from crci, to no crci.
> 
> Separating the channel+crci into a virtual channel means that the ADM 
> driver
> itself will collate the requests as they come in, and apply them to the 
> actual
> hardware channel.
> 
> That make sense?  If so, this requires modifying the DT doc to specify 
> the crci
> in the binding, modifying the ADM driver to identify/add channels based 
> on the
> channel + crci, and modifying the clients which use both flow control 
> and
> non-flow control.

OK thanks for clearing that up. Would be nice to avoid calling 
slave_config
for each command. I'll give this a shot.

-- 
thomas

RE: [RESEND,2/5] dmaengine: Add ADM driver

[Zoran Markovic]

> Add the DMA engine driver for the QCOM Application Data Mover (ADM) DMA
> controller found in the MSM8x60 and IPQ/APQ8064 platforms.

With minor changes I got this working on MDM9615, using qcom_nand. Below are the changes I had to make, please consider them. Patches for MDM9615 NAND are pending.

> +static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan,
> +	struct scatterlist *sgl, unsigned int sg_len,
> +	enum dma_transfer_direction direction, unsigned long flags,
> +	void *context)
> +{
...

> +	/* if using flow control, validate burst and crci values */
> +	if (achan->slave.device_fc) {
> +
> +		blk_size = adm_get_blksize(burst);
> +		if (blk_size < 0) {
> +			dev_err(adev->dev, "invalid burst value: %d\n",
> +				burst);
> +			return ERR_PTR(-EINVAL);
Return NULL here, most DMA clients (including qcom_nand) expect NULL if prep_slave_sg() fails.
> +		}
> +
> +		crci = achan->slave.slave_id & 0xf;
> +		if (!crci || achan->slave.slave_id > 0x1f) {
> +			dev_err(adev->dev, "invalid crci value\n");
> +			return ERR_PTR(-EINVAL);
Ditto above.
> +		}
> +	}
> +
> +	/* iterate through sgs and compute allocation size of structures */
> +	for_each_sg(sgl, sg, sg_len, i) {
> +		if (achan->slave.device_fc) {
> +			box_count += DIV_ROUND_UP(sg_dma_len(sg) / burst,
> +						  ADM_MAX_ROWS);
> +			if (sg_dma_len(sg) % burst)
> +				single_count++;
> +		} else {
> +			single_count += DIV_ROUND_UP(sg_dma_len(sg),
> +						     ADM_MAX_XFER);
> +		}
> +	}
> +
> +	async_desc = kzalloc(sizeof(*async_desc), GFP_NOWAIT);
> +	if (!async_desc)
> +		return ERR_PTR(-ENOMEM);
Ditto above.

> +
> +	if (crci)
> +		async_desc->mux = achan->slave.slave_id & ADM_CRCI_MUX_SEL ?
> +					ADM_CRCI_CTL_MUX_SEL : 0;
> +	async_desc->crci = crci;
> +	async_desc->blk_size = blk_size;
> +	async_desc->dma_len = single_count * sizeof(struct adm_desc_hw_single) +
> +				box_count * sizeof(struct adm_desc_hw_box) +
> +				sizeof(*cple) + 2 * ADM_DESC_ALIGN;
> +
> +	async_desc->cpl = dma_alloc_writecombine(adev->dev, async_desc->dma_len,
> +				&async_desc->dma_addr, GFP_NOWAIT);
Under pressure this allocation might fail, resulting in NAND errors. I handled it using wait_event_timeout() to wait until buffers are available. Either that, or clients such as qcom_nand need to handle this failure.

> +
> +	if (!async_desc->cpl) {
> +		kfree(async_desc);
> +		return ERR_PTR(-ENOMEM);
Return NULL.
> +	}
...
> +}
...

> +static void adm_dma_free_desc(struct virt_dma_desc *vd) {
> +	struct adm_async_desc *async_desc = container_of(vd,
> +			struct adm_async_desc, vd);
> +
> +	dma_free_writecombine(async_desc->adev->dev, async_desc->dma_len,
> +		async_desc->cpl, async_desc->dma_addr);
> +	kfree(async_desc);
Do wake_up() here to signal buffer availability.
> +}

Regards, Zoran

Re: [RESEND,2/5] dmaengine: Add ADM driver

[Neil Armstrong]

Le 22/12/2016 18:55, Zoran Markovic a écrit :
>> Add the DMA engine driver for the QCOM Application Data Mover (ADM) DMA
>> controller found in the MSM8x60 and IPQ/APQ8064 platforms.
> 
> With minor changes I got this working on MDM9615, using qcom_nand. Below are the changes I had to make, please consider them. Patches for MDM9615 NAND are pending.
> 
>> +static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan,
>> +	struct scatterlist *sgl, unsigned int sg_len,
>> +	enum dma_transfer_direction direction, unsigned long flags,
>> +	void *context)
>> +{
> ...
> 
>> +	/* if using flow control, validate burst and crci values */
>> +	if (achan->slave.device_fc) {
>> +
>> +		blk_size = adm_get_blksize(burst);
>> +		if (blk_size < 0) {
>> +			dev_err(adev->dev, "invalid burst value: %d\n",
>> +				burst);
>> +			return ERR_PTR(-EINVAL);
> Return NULL here, most DMA clients (including qcom_nand) expect NULL if prep_slave_sg() fails.
>> +		}
>> +
>> +		crci = achan->slave.slave_id & 0xf;
>> +		if (!crci || achan->slave.slave_id > 0x1f) {
>> +			dev_err(adev->dev, "invalid crci value\n");
>> +			return ERR_PTR(-EINVAL);
> Ditto above.
>> +		}
>> +	}
>> +
>> +	/* iterate through sgs and compute allocation size of structures */
>> +	for_each_sg(sgl, sg, sg_len, i) {
>> +		if (achan->slave.device_fc) {
>> +			box_count += DIV_ROUND_UP(sg_dma_len(sg) / burst,
>> +						  ADM_MAX_ROWS);
>> +			if (sg_dma_len(sg) % burst)
>> +				single_count++;
>> +		} else {
>> +			single_count += DIV_ROUND_UP(sg_dma_len(sg),
>> +						     ADM_MAX_XFER);
>> +		}
>> +	}
>> +
>> +	async_desc = kzalloc(sizeof(*async_desc), GFP_NOWAIT);
>> +	if (!async_desc)
>> +		return ERR_PTR(-ENOMEM);
> Ditto above.
> 
>> +
>> +	if (crci)
>> +		async_desc->mux = achan->slave.slave_id & ADM_CRCI_MUX_SEL ?
>> +					ADM_CRCI_CTL_MUX_SEL : 0;
>> +	async_desc->crci = crci;
>> +	async_desc->blk_size = blk_size;
>> +	async_desc->dma_len = single_count * sizeof(struct adm_desc_hw_single) +
>> +				box_count * sizeof(struct adm_desc_hw_box) +
>> +				sizeof(*cple) + 2 * ADM_DESC_ALIGN;
>> +
>> +	async_desc->cpl = dma_alloc_writecombine(adev->dev, async_desc->dma_len,
>> +				&async_desc->dma_addr, GFP_NOWAIT);
> Under pressure this allocation might fail, resulting in NAND errors. I handled it using wait_event_timeout() to wait until buffers are available. Either that, or clients such as qcom_nand need to handle this failure.
> 
>> +
>> +	if (!async_desc->cpl) {
>> +		kfree(async_desc);
>> +		return ERR_PTR(-ENOMEM);
> Return NULL.
>> +	}
> ...
>> +}
> ...
> 
>> +static void adm_dma_free_desc(struct virt_dma_desc *vd) {
>> +	struct adm_async_desc *async_desc = container_of(vd,
>> +			struct adm_async_desc, vd);
>> +
>> +	dma_free_writecombine(async_desc->adev->dev, async_desc->dma_len,
>> +		async_desc->cpl, async_desc->dma_addr);
>> +	kfree(async_desc);
> Do wake_up() here to signal buffer availability.
>> +}
> 
> Regards, Zoran
> 

Hi Zoran,

These are also the fixes we needed to make the ADM work on MDM9615.

Andy, do you plan to resend this driver ?

Neil

Re: [RESEND,2/5] dmaengine: Add ADM driver

[Andy Gross]

On Fri, Dec 23, 2016 at 08:53:09PM +0100, Neil Armstrong wrote:
> 
> 
> Le 22/12/2016 18:55, Zoran Markovic a écrit :
> >> Add the DMA engine driver for the QCOM Application Data Mover (ADM) DMA
> >> controller found in the MSM8x60 and IPQ/APQ8064 platforms.
> > 
> > With minor changes I got this working on MDM9615, using qcom_nand. Below are the changes I had to make, please consider them. Patches for MDM9615 NAND are pending.
> > 
> >> +static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan,
> >> +	struct scatterlist *sgl, unsigned int sg_len,
> >> +	enum dma_transfer_direction direction, unsigned long flags,
> >> +	void *context)
> >> +{
> > ...
> > 
> >> +	/* if using flow control, validate burst and crci values */
> >> +	if (achan->slave.device_fc) {
> >> +
> >> +		blk_size = adm_get_blksize(burst);
> >> +		if (blk_size < 0) {
> >> +			dev_err(adev->dev, "invalid burst value: %d\n",
> >> +				burst);
> >> +			return ERR_PTR(-EINVAL);
> > Return NULL here, most DMA clients (including qcom_nand) expect NULL if prep_slave_sg() fails.
> >> +		}
> >> +
> >> +		crci = achan->slave.slave_id & 0xf;
> >> +		if (!crci || achan->slave.slave_id > 0x1f) {
> >> +			dev_err(adev->dev, "invalid crci value\n");
> >> +			return ERR_PTR(-EINVAL);
> > Ditto above.
> >> +		}
> >> +	}
> >> +
> >> +	/* iterate through sgs and compute allocation size of structures */
> >> +	for_each_sg(sgl, sg, sg_len, i) {
> >> +		if (achan->slave.device_fc) {
> >> +			box_count += DIV_ROUND_UP(sg_dma_len(sg) / burst,
> >> +						  ADM_MAX_ROWS);
> >> +			if (sg_dma_len(sg) % burst)
> >> +				single_count++;
> >> +		} else {
> >> +			single_count += DIV_ROUND_UP(sg_dma_len(sg),
> >> +						     ADM_MAX_XFER);
> >> +		}
> >> +	}
> >> +
> >> +	async_desc = kzalloc(sizeof(*async_desc), GFP_NOWAIT);
> >> +	if (!async_desc)
> >> +		return ERR_PTR(-ENOMEM);
> > Ditto above.
> > 
> >> +
> >> +	if (crci)
> >> +		async_desc->mux = achan->slave.slave_id & ADM_CRCI_MUX_SEL ?
> >> +					ADM_CRCI_CTL_MUX_SEL : 0;
> >> +	async_desc->crci = crci;
> >> +	async_desc->blk_size = blk_size;
> >> +	async_desc->dma_len = single_count * sizeof(struct adm_desc_hw_single) +
> >> +				box_count * sizeof(struct adm_desc_hw_box) +
> >> +				sizeof(*cple) + 2 * ADM_DESC_ALIGN;
> >> +
> >> +	async_desc->cpl = dma_alloc_writecombine(adev->dev, async_desc->dma_len,
> >> +				&async_desc->dma_addr, GFP_NOWAIT);
> > Under pressure this allocation might fail, resulting in NAND errors. I handled it using wait_event_timeout() to wait until buffers are available. Either that, or clients such as qcom_nand need to handle this failure.

prep_slave_sg calls can be made from atomic context.  Which is why we do the
GFP_NOWAIT.  You can't sleep in that function.  Instead, the client needs to see
that it got ENOMEM and deal with the memory pressure itself.  Either that or we
need to relax the dma_alloc to a dma_alloc_coherent (I suspect you'll still have
the same problem).  Or you need to increase the static dma pool.

I've seen this issue with people using ADM w/ SPI.  They had other dma pool
pressure and the ADM transfers would fail to get buffers.

> > 
> >> +
> >> +	if (!async_desc->cpl) {
> >> +		kfree(async_desc);
> >> +		return ERR_PTR(-ENOMEM);
> > Return NULL.
> >> +	}
> > ...
> >> +}
> > ...
> > 
> >> +static void adm_dma_free_desc(struct virt_dma_desc *vd) {
> >> +	struct adm_async_desc *async_desc = container_of(vd,
> >> +			struct adm_async_desc, vd);
> >> +
> >> +	dma_free_writecombine(async_desc->adev->dev, async_desc->dma_len,
> >> +		async_desc->cpl, async_desc->dma_addr);
> >> +	kfree(async_desc);
> > Do wake_up() here to signal buffer availability.
> >> +}
> > 
> > Regards, Zoran
> > 
> 
> Hi Zoran,
> 
> These are also the fixes we needed to make the ADM work on MDM9615.
> 
> Andy, do you plan to resend this driver ?

The main outstanding comments I had were specific to the crci encoding and the
use of a virtual channel to encapsulate the crci information instead of using
the slave_config.  I don't see that being done.

I haven't had time to address that and I was hoping that Thomas would do it.
But it seems like he moved on to other things, so I need to put this on my list.


Andy