[PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[Sinan Kaya]

This patch implements the hardware hooks for the HIDMA channel driver.

The main functions of interest are:
- hidma_ll_init
- hidma_ll_request
- hidma_ll_queue_request
- hidma_ll_hw_start

OS layer calls the hidma_ll_init function during probe to set up the
hardware. At this moment, the number of supported descriptors are also
given. On each request, a descriptor is allocated from the free pool and
filled in with the transfer parameters. Multiple requests can be queued
into the hardware via the OS interface. When client is ready for requests
to be executed, start method is called.

Completions are delivered via callbacks via tasklet.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/dma/qcom/Makefile   |   2 +
 drivers/dma/qcom/hidma.h    |  20 +-
 drivers/dma/qcom/hidma_ll.c | 900 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 912 insertions(+), 10 deletions(-)
 create mode 100644 drivers/dma/qcom/hidma_ll.c

diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile
index bfea699..6bf9267 100644
--- a/drivers/dma/qcom/Makefile
+++ b/drivers/dma/qcom/Makefile
@@ -1,3 +1,5 @@
 obj-$(CONFIG_QCOM_BAM_DMA) += bam_dma.o
 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
diff --git a/drivers/dma/qcom/hidma.h b/drivers/dma/qcom/hidma.h
index 231e306..c5eea65 100644
--- a/drivers/dma/qcom/hidma.h
+++ b/drivers/dma/qcom/hidma.h
@@ -1,7 +1,7 @@
 /*
  * Qualcomm Technologies HIDMA data structures
  *
- * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014-2016, 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
@@ -20,13 +20,13 @@
 #include <linux/interrupt.h>
 #include <linux/dmaengine.h>
 
-#define TRE_SIZE			32 /* each TRE is 32 bytes  */
-#define TRE_CFG_IDX			0
-#define TRE_LEN_IDX			1
-#define TRE_SRC_LOW_IDX		2
-#define TRE_SRC_HI_IDX			3
-#define TRE_DEST_LOW_IDX		4
-#define TRE_DEST_HI_IDX		5
+#define HIDMA_TRE_SIZE			32 /* each TRE is 32 bytes  */
+#define HIDMA_TRE_CFG_IDX		0
+#define HIDMA_TRE_LEN_IDX		1
+#define HIDMA_TRE_SRC_LOW_IDX		2
+#define HIDMA_TRE_SRC_HI_IDX		3
+#define HIDMA_TRE_DEST_LOW_IDX		4
+#define HIDMA_TRE_DEST_HI_IDX		5
 
 struct hidma_tx_status {
 	u8 err_info;			/* error record in this transfer    */
@@ -37,13 +37,13 @@ struct hidma_tre {
 	atomic_t allocated;		/* if this channel is allocated	    */
 	bool queued;			/* flag whether this is pending     */
 	u16 status;			/* status			    */
-	u32 chidx;			/* index of the tre		    */
+	u32 idx;			/* index of the tre		    */
 	u32 dma_sig;			/* signature of the tre		    */
 	const char *dev_name;		/* name of the device		    */
 	void (*callback)(void *data);	/* requester callback		    */
 	void *data;			/* Data associated with this channel*/
 	struct hidma_lldev *lldev;	/* lldma device pointer		    */
-	u32 tre_local[TRE_SIZE / sizeof(u32) + 1]; /* TRE local copy        */
+	u32 tre_local[HIDMA_TRE_SIZE / sizeof(u32) + 1]; /* TRE local copy  */
 	u32 tre_index;			/* the offset where this was written*/
 	u32 int_flags;			/* interrupt flags		    */
 };
diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
new file mode 100644
index 0000000..443a5a2
--- /dev/null
+++ b/drivers/dma/qcom/hidma_ll.c
@@ -0,0 +1,900 @@
+/*
+ * Qualcomm Technologies HIDMA DMA engine low level code
+ *
+ * Copyright (c) 2015-2016, 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/dmaengine.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/mm.h>
+#include <linux/highmem.h>
+#include <linux/dma-mapping.h>
+#include <linux/delay.h>
+#include <linux/atomic.h>
+#include <linux/iopoll.h>
+#include <linux/kfifo.h>
+#include <linux/bitops.h>
+
+#include "hidma.h"
+
+#define HIDMA_EVRE_SIZE			16	/* each EVRE is 16 bytes */
+
+#define HIDMA_TRCA_CTRLSTS_REG			0x000
+#define HIDMA_TRCA_RING_LOW_REG		0x008
+#define HIDMA_TRCA_RING_HIGH_REG		0x00C
+#define HIDMA_TRCA_RING_LEN_REG		0x010
+#define HIDMA_TRCA_DOORBELL_REG		0x400
+
+#define HIDMA_EVCA_CTRLSTS_REG			0x000
+#define HIDMA_EVCA_INTCTRL_REG			0x004
+#define HIDMA_EVCA_RING_LOW_REG		0x008
+#define HIDMA_EVCA_RING_HIGH_REG		0x00C
+#define HIDMA_EVCA_RING_LEN_REG		0x010
+#define HIDMA_EVCA_WRITE_PTR_REG		0x020
+#define HIDMA_EVCA_DOORBELL_REG		0x400
+
+#define HIDMA_EVCA_IRQ_STAT_REG		0x100
+#define HIDMA_EVCA_IRQ_CLR_REG			0x108
+#define HIDMA_EVCA_IRQ_EN_REG			0x110
+
+#define HIDMA_EVRE_CFG_IDX			0
+
+#define HIDMA_EVRE_ERRINFO_BIT_POS		24
+#define HIDMA_EVRE_CODE_BIT_POS		28
+
+#define HIDMA_EVRE_ERRINFO_MASK		GENMASK(3, 0)
+#define HIDMA_EVRE_CODE_MASK			GENMASK(3, 0)
+
+#define HIDMA_CH_CONTROL_MASK			GENMASK(7, 0)
+#define HIDMA_CH_STATE_MASK			GENMASK(7, 0)
+#define HIDMA_CH_STATE_BIT_POS			0x8
+
+#define HIDMA_IRQ_EV_CH_EOB_IRQ_BIT_POS	0
+#define HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS	1
+#define HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS	9
+#define HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS	10
+#define HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS	11
+#define HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS	14
+
+#define ENABLE_IRQS (BIT(HIDMA_IRQ_EV_CH_EOB_IRQ_BIT_POS)	| \
+		BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)		| \
+		BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)	| \
+		BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)		| \
+		BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS)		| \
+		BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))
+
+#define HIDMA_INCREMENT_ITERATOR(iter, size, ring_size)	\
+do {								\
+	iter += size;						\
+	if (iter >= ring_size)					\
+		iter -= ring_size;				\
+} while (0)
+
+#define HIDMA_CH_STATE(val)	\
+	((val >> HIDMA_CH_STATE_BIT_POS) & HIDMA_CH_STATE_MASK)
+
+enum ch_command {
+	HIDMA_CH_DISABLE = 0,
+	HIDMA_CH_ENABLE = 1,
+	HIDMA_CH_SUSPEND = 2,
+	HIDMA_CH_RESET = 9,
+};
+
+enum ch_state {
+	HIDMA_CH_DISABLED = 0,
+	HIDMA_CH_ENABLED = 1,
+	HIDMA_CH_RUNNING = 2,
+	HIDMA_CH_SUSPENDED = 3,
+	HIDMA_CH_STOPPED = 4,
+};
+
+enum tre_type {
+	HIDMA_TRE_MEMCPY = 3,
+};
+
+enum err_code {
+	HIDMA_EVRE_STATUS_COMPLETE = 1,
+	HIDMA_EVRE_STATUS_ERROR = 4,
+};
+
+void hidma_ll_free(struct hidma_lldev *lldev, u32 tre_ch)
+{
+	struct hidma_tre *tre;
+
+	if (tre_ch >= lldev->nr_tres) {
+		dev_err(lldev->dev, "invalid TRE number in free:%d", tre_ch);
+		return;
+	}
+
+	tre = &lldev->trepool[tre_ch];
+	if (atomic_read(&tre->allocated) != true) {
+		dev_err(lldev->dev, "trying to free an unused TRE:%d", tre_ch);
+		return;
+	}
+
+	atomic_set(&tre->allocated, 0);
+}
+
+int hidma_ll_request(struct hidma_lldev *lldev, u32 dma_sig,
+		     const char *dev_name,
+		     void (*callback)(void *data), void *data, u32 *tre_ch)
+{
+	unsigned int i;
+	struct hidma_tre *tre;
+	u32 *tre_local;
+
+	if (!tre_ch || !lldev)
+		return -EINVAL;
+
+	/* need to have at least one empty spot in the queue */
+	for (i = 0; i < lldev->nr_tres - 1; i++) {
+		if (atomic_add_unless(&lldev->trepool[i].allocated, 1, 1))
+			break;
+	}
+
+	if (i == (lldev->nr_tres - 1))
+		return -ENOMEM;
+
+	tre = &lldev->trepool[i];
+	tre->dma_sig = dma_sig;
+	tre->dev_name = dev_name;
+	tre->callback = callback;
+	tre->data = data;
+	tre->idx = i;
+	tre->status = 0;
+	tre->queued = 0;
+	lldev->tx_status_list[i].err_code = 0;
+	tre->lldev = lldev;
+	tre_local = &tre->tre_local[0];
+	tre_local[HIDMA_TRE_CFG_IDX] = HIDMA_TRE_MEMCPY;
+	tre_local[HIDMA_TRE_CFG_IDX] |= (lldev->chidx & 0xFF) << 8;
+	tre_local[HIDMA_TRE_CFG_IDX] |= BIT(16);	/* set IEOB */
+	*tre_ch = i;
+	if (callback)
+		callback(data);
+	return 0;
+}
+
+/*
+ * Multiple TREs may be queued and waiting in the
+ * pending queue.
+ */
+static void hidma_ll_tre_complete(unsigned long arg)
+{
+	struct hidma_lldev *lldev = (struct hidma_lldev *)arg;
+	struct hidma_tre *tre;
+
+	while (kfifo_out(&lldev->handoff_fifo, &tre, 1)) {
+		/* call the user if it has been read by the hardware */
+		if (tre->callback)
+			tre->callback(tre->data);
+	}
+}
+
+/*
+ * Called to handle the interrupt for the channel.
+ * Return a positive number if TRE or EVRE were consumed on this run.
+ * Return a positive number if there are pending TREs or EVREs.
+ * Return 0 if there is nothing to consume or no pending TREs/EVREs found.
+ */
+static int hidma_handle_tre_completion(struct hidma_lldev *lldev)
+{
+	struct hidma_tre *tre;
+	u32 evre_write_off;
+	u32 evre_ring_size = lldev->evre_ring_size;
+	u32 tre_ring_size = lldev->tre_ring_size;
+	u32 num_completed = 0, tre_iterator, evre_iterator;
+	unsigned long flags;
+
+	evre_write_off = readl_relaxed(lldev->evca + HIDMA_EVCA_WRITE_PTR_REG);
+	tre_iterator = lldev->tre_processed_off;
+	evre_iterator = lldev->evre_processed_off;
+
+	if ((evre_write_off > evre_ring_size) ||
+	    ((evre_write_off % HIDMA_EVRE_SIZE) != 0)) {
+		dev_err(lldev->dev, "HW reports invalid EVRE write offset\n");
+		return 0;
+	}
+
+	/*
+	 * By the time control reaches here the number of EVREs and TREs
+	 * may not match. Only consume the ones that hardware told us.
+	 */
+	while ((evre_iterator != evre_write_off)) {
+		u32 *current_evre = lldev->evre_ring + evre_iterator;
+		u32 cfg;
+		u8 err_info;
+
+		spin_lock_irqsave(&lldev->lock, flags);
+		tre = lldev->pending_tre_list[tre_iterator / HIDMA_TRE_SIZE];
+		if (!tre) {
+			spin_unlock_irqrestore(&lldev->lock, flags);
+			dev_warn(lldev->dev,
+				 "tre_index [%d] and tre out of sync\n",
+				 tre_iterator / HIDMA_TRE_SIZE);
+			HIDMA_INCREMENT_ITERATOR(tre_iterator, HIDMA_TRE_SIZE,
+						 tre_ring_size);
+			HIDMA_INCREMENT_ITERATOR(evre_iterator,
+						 HIDMA_EVRE_SIZE,
+						 evre_ring_size);
+			continue;
+		}
+		lldev->pending_tre_list[tre->tre_index] = NULL;
+
+		/*
+		 * Keep track of pending TREs that SW is expecting to receive
+		 * from HW. We got one now. Decrement our counter.
+		 */
+		lldev->pending_tre_count--;
+		if (lldev->pending_tre_count < 0) {
+			dev_warn(lldev->dev,
+				 "tre count mismatch on completion");
+			lldev->pending_tre_count = 0;
+		}
+
+		spin_unlock_irqrestore(&lldev->lock, flags);
+
+		cfg = current_evre[HIDMA_EVRE_CFG_IDX];
+		err_info = cfg >> HIDMA_EVRE_ERRINFO_BIT_POS;
+		err_info &= HIDMA_EVRE_ERRINFO_MASK;
+		lldev->tx_status_list[tre->idx].err_info = err_info;
+		lldev->tx_status_list[tre->idx].err_code =
+		    (cfg >> HIDMA_EVRE_CODE_BIT_POS) & HIDMA_EVRE_CODE_MASK;
+		tre->queued = 0;
+
+		kfifo_put(&lldev->handoff_fifo, tre);
+		tasklet_schedule(&lldev->task);
+
+		HIDMA_INCREMENT_ITERATOR(tre_iterator, HIDMA_TRE_SIZE,
+					 tre_ring_size);
+		HIDMA_INCREMENT_ITERATOR(evre_iterator, HIDMA_EVRE_SIZE,
+					 evre_ring_size);
+
+		/*
+		 * Read the new event descriptor written by the HW.
+		 * As we are processing the delivered events, other events
+		 * get queued to the SW for processing.
+		 */
+		evre_write_off =
+		    readl_relaxed(lldev->evca + HIDMA_EVCA_WRITE_PTR_REG);
+		num_completed++;
+	}
+
+	if (num_completed) {
+		u32 evre_read_off = (lldev->evre_processed_off +
+				     HIDMA_EVRE_SIZE * num_completed);
+		u32 tre_read_off = (lldev->tre_processed_off +
+				    HIDMA_TRE_SIZE * num_completed);
+
+		evre_read_off = evre_read_off % evre_ring_size;
+		tre_read_off = tre_read_off % tre_ring_size;
+
+		writel(evre_read_off, lldev->evca + HIDMA_EVCA_DOORBELL_REG);
+
+		/* record the last processed tre offset */
+		lldev->tre_processed_off = tre_read_off;
+		lldev->evre_processed_off = evre_read_off;
+	}
+
+	return num_completed;
+}
+
+void hidma_cleanup_pending_tre(struct hidma_lldev *lldev, u8 err_info,
+			       u8 err_code)
+{
+	u32 tre_iterator;
+	struct hidma_tre *tre;
+	u32 tre_ring_size = lldev->tre_ring_size;
+	int num_completed = 0;
+	u32 tre_read_off;
+	unsigned long flags;
+
+	tre_iterator = lldev->tre_processed_off;
+	while (lldev->pending_tre_count) {
+		int tre_index = tre_iterator / HIDMA_TRE_SIZE;
+
+		spin_lock_irqsave(&lldev->lock, flags);
+		tre = lldev->pending_tre_list[tre_index];
+		if (!tre) {
+			spin_unlock_irqrestore(&lldev->lock, flags);
+			HIDMA_INCREMENT_ITERATOR(tre_iterator, HIDMA_TRE_SIZE,
+						 tre_ring_size);
+			continue;
+		}
+		lldev->pending_tre_list[tre_index] = NULL;
+		lldev->pending_tre_count--;
+		if (lldev->pending_tre_count < 0) {
+			dev_warn(lldev->dev,
+				 "tre count mismatch on completion");
+			lldev->pending_tre_count = 0;
+		}
+		spin_unlock_irqrestore(&lldev->lock, flags);
+
+		lldev->tx_status_list[tre->idx].err_info = err_info;
+		lldev->tx_status_list[tre->idx].err_code = err_code;
+		tre->queued = 0;
+
+		kfifo_put(&lldev->handoff_fifo, tre);
+		tasklet_schedule(&lldev->task);
+
+		HIDMA_INCREMENT_ITERATOR(tre_iterator, HIDMA_TRE_SIZE,
+					 tre_ring_size);
+		num_completed++;
+	}
+	tre_read_off = (lldev->tre_processed_off +
+			HIDMA_TRE_SIZE * num_completed);
+
+	tre_read_off = tre_read_off % tre_ring_size;
+
+	/* record the last processed tre offset */
+	lldev->tre_processed_off = tre_read_off;
+}
+
+static int hidma_ll_reset(struct hidma_lldev *lldev)
+{
+	u32 val;
+	int ret;
+
+	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
+	val &= ~(HIDMA_CH_CONTROL_MASK << 16);
+	val |= HIDMA_CH_RESET << 16;
+	writel(val, lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
+
+	/*
+	 * Delay 10ms after reset to allow DMA logic to quiesce.
+	 * Do a polled read up to 1ms and 10ms maximum.
+	 */
+	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
+				 HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
+				 10000);
+	if (ret) {
+		dev_err(lldev->dev, "transfer channel did not reset\n");
+		return ret;
+	}
+
+	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
+	val &= ~(HIDMA_CH_CONTROL_MASK << 16);
+	val |= HIDMA_CH_RESET << 16;
+	writel(val, lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
+
+	/*
+	 * Delay 10ms after reset to allow DMA logic to quiesce.
+	 * Do a polled read up to 1ms and 10ms maximum.
+	 */
+	ret = readl_poll_timeout(lldev->evca + HIDMA_EVCA_CTRLSTS_REG, val,
+				 HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
+				 10000);
+	if (ret)
+		return ret;
+
+	lldev->trch_state = HIDMA_CH_DISABLED;
+	lldev->evch_state = HIDMA_CH_DISABLED;
+	return 0;
+}
+
+static void hidma_ll_enable_irq(struct hidma_lldev *lldev, u32 irq_bits)
+{
+	writel(irq_bits, lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
+}
+
+/*
+ * The interrupt handler for HIDMA will try to consume as many pending
+ * EVRE from the event queue as possible. Each EVRE has an associated
+ * TRE that holds the user interface parameters. EVRE reports the
+ * result of the transaction. Hardware guarantees ordering between EVREs
+ * and TREs. We use last processed offset to figure out which TRE is
+ * associated with which EVRE. If two TREs are consumed by HW, the EVREs
+ * are in order in the event ring.
+ *
+ * This handler will do a one pass for consuming EVREs. Other EVREs may
+ * be delivered while we are working. It will try to consume incoming
+ * EVREs one more time and return.
+ *
+ * For unprocessed EVREs, hardware will trigger another interrupt until
+ * all the interrupt bits are cleared.
+ *
+ * Hardware guarantees that by the time interrupt is observed, all data
+ * transactions in flight are delivered to their respective places and
+ * are visible to the CPU.
+ *
+ * On demand paging for IOMMU is only supported for PCIe via PRI
+ * (Page Request Interface) not for HIDMA. All other hardware instances
+ * including HIDMA work on pinned DMA addresses.
+ *
+ * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
+ * IOMMU latency will be built into the data movement time. By the time
+ * interrupt happens, IOMMU lookups + data movement has already taken place.
+ *
+ * While the first read in a typical PCI endpoint ISR flushes all outstanding
+ * requests traditionally to the destination, this concept does not apply
+ * here for this HW.
+ */
+static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
+{
+	u32 status;
+	u32 enable;
+	u32 cause;
+
+	/*
+	 * Fine tuned for this HW...
+	 *
+	 * This ISR has been designed for this particular hardware. Relaxed
+	 * read and write accessors are used for performance reasons due to
+	 * interrupt delivery guarantees. Do not copy this code blindly and
+	 * expect that to work.
+	 */
+	status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
+	enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
+	cause = status & enable;
+
+	while (cause) {
+		if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
+		    (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
+		    (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
+		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
+		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
+			u8 err_code = HIDMA_EVRE_STATUS_ERROR;
+			u8 err_info = 0xFF;
+
+			/* Clear out pending interrupts */
+			writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
+
+			dev_err(lldev->dev, "error 0x%x, resetting...\n",
+				cause);
+
+			hidma_cleanup_pending_tre(lldev, err_info, err_code);
+
+			/* reset the channel for recovery */
+			if (hidma_ll_setup(lldev)) {
+				dev_err(lldev->dev,
+					"channel reinitialize failed after error\n");
+				return;
+			}
+			hidma_ll_enable_irq(lldev, ENABLE_IRQS);
+			return;
+		}
+
+		/*
+		 * Try to consume as many EVREs as possible.
+		 */
+		hidma_handle_tre_completion(lldev);
+
+		/* We consumed TREs or there are pending TREs or EVREs. */
+		writel_relaxed(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
+
+		/*
+		 * Another interrupt might have arrived while we are
+		 * processing this one. Read the new cause.
+		 */
+		status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
+		enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
+		cause = status & enable;
+	}
+}
+
+static int hidma_ll_enable(struct hidma_lldev *lldev)
+{
+	u32 val;
+	int ret;
+
+	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
+	val &= ~(HIDMA_CH_CONTROL_MASK << 16);
+	val |= HIDMA_CH_ENABLE << 16;
+	writel(val, lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
+
+	ret = readl_poll_timeout(lldev->evca + HIDMA_EVCA_CTRLSTS_REG, val,
+				 (HIDMA_CH_STATE(val) == HIDMA_CH_ENABLED) ||
+				 (HIDMA_CH_STATE(val) == HIDMA_CH_RUNNING),
+				 1000, 10000);
+	if (ret) {
+		dev_err(lldev->dev, "event channel did not get enabled\n");
+		return ret;
+	}
+
+	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
+	val &= ~(HIDMA_CH_CONTROL_MASK << 16);
+	val |= HIDMA_CH_ENABLE << 16;
+	writel(val, lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
+
+	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
+				 (HIDMA_CH_STATE(val) == HIDMA_CH_ENABLED) ||
+				 (HIDMA_CH_STATE(val) == HIDMA_CH_RUNNING),
+				 1000, 10000);
+	if (ret) {
+		dev_err(lldev->dev, "transfer channel did not get enabled\n");
+		return ret;
+	}
+
+	lldev->trch_state = HIDMA_CH_ENABLED;
+	lldev->evch_state = HIDMA_CH_ENABLED;
+
+	return 0;
+}
+
+int hidma_ll_resume(struct hidma_lldev *lldev)
+{
+	return hidma_ll_enable(lldev);
+}
+
+static void hidma_ll_hw_start(struct hidma_lldev *lldev)
+{
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&lldev->lock, irqflags);
+	writel(lldev->tre_write_offset, lldev->trca + HIDMA_TRCA_DOORBELL_REG);
+	spin_unlock_irqrestore(&lldev->lock, irqflags);
+}
+
+bool hidma_ll_isenabled(struct hidma_lldev *lldev)
+{
+	u32 val;
+
+	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
+	lldev->trch_state = HIDMA_CH_STATE(val);
+	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
+	lldev->evch_state = HIDMA_CH_STATE(val);
+
+	/* both channels have to be enabled before calling this function */
+	if (((lldev->trch_state == HIDMA_CH_ENABLED) ||
+	     (lldev->trch_state == HIDMA_CH_RUNNING)) &&
+	    ((lldev->evch_state == HIDMA_CH_ENABLED) ||
+	     (lldev->evch_state == HIDMA_CH_RUNNING)))
+		return true;
+
+	return false;
+}
+
+void hidma_ll_queue_request(struct hidma_lldev *lldev, u32 tre_ch)
+{
+	struct hidma_tre *tre;
+	unsigned long flags;
+
+	tre = &lldev->trepool[tre_ch];
+
+	/* copy the TRE into its location in the TRE ring */
+	spin_lock_irqsave(&lldev->lock, flags);
+	tre->tre_index = lldev->tre_write_offset / HIDMA_TRE_SIZE;
+	lldev->pending_tre_list[tre->tre_index] = tre;
+	memcpy(lldev->tre_ring + lldev->tre_write_offset,
+	       &tre->tre_local[0], HIDMA_TRE_SIZE);
+	lldev->tx_status_list[tre->idx].err_code = 0;
+	lldev->tx_status_list[tre->idx].err_info = 0;
+	tre->queued = 1;
+	lldev->pending_tre_count++;
+	lldev->tre_write_offset = (lldev->tre_write_offset + HIDMA_TRE_SIZE)
+					% lldev->tre_ring_size;
+	spin_unlock_irqrestore(&lldev->lock, flags);
+}
+
+void hidma_ll_start(struct hidma_lldev *lldev)
+{
+	hidma_ll_hw_start(lldev);
+}
+
+/*
+ * Note that even though we stop this channel
+ * if there is a pending transaction in flight
+ * it will complete and follow the callback.
+ * This request will prevent further requests
+ * to be made.
+ */
+int hidma_ll_pause(struct hidma_lldev *lldev)
+{
+	u32 val;
+	int ret;
+
+	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
+	lldev->evch_state = HIDMA_CH_STATE(val);
+	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
+	lldev->trch_state = HIDMA_CH_STATE(val);
+
+	/* already suspended by this OS */
+	if ((lldev->trch_state == HIDMA_CH_SUSPENDED) ||
+	    (lldev->evch_state == HIDMA_CH_SUSPENDED))
+		return 0;
+
+	/* already stopped by the manager */
+	if ((lldev->trch_state == HIDMA_CH_STOPPED) ||
+	    (lldev->evch_state == HIDMA_CH_STOPPED))
+		return 0;
+
+	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
+	val &= ~(HIDMA_CH_CONTROL_MASK << 16);
+	val |= HIDMA_CH_SUSPEND << 16;
+	writel(val, lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
+
+	/*
+	 * Start the wait right after the suspend is confirmed.
+	 * Do a polled read up to 1ms and 10ms maximum.
+	 */
+	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
+				 HIDMA_CH_STATE(val) == HIDMA_CH_SUSPENDED,
+				 1000, 10000);
+	if (ret)
+		return ret;
+
+	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
+	val &= ~(HIDMA_CH_CONTROL_MASK << 16);
+	val |= HIDMA_CH_SUSPEND << 16;
+	writel(val, lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
+
+	/*
+	 * Start the wait right after the suspend is confirmed
+	 * Delay up to 10ms after reset to allow DMA logic to quiesce.
+	 */
+	ret = readl_poll_timeout(lldev->evca + HIDMA_EVCA_CTRLSTS_REG, val,
+				 HIDMA_CH_STATE(val) == HIDMA_CH_SUSPENDED,
+				 1000, 10000);
+	if (ret)
+		return ret;
+
+	lldev->trch_state = HIDMA_CH_SUSPENDED;
+	lldev->evch_state = HIDMA_CH_SUSPENDED;
+	return 0;
+}
+
+void hidma_ll_set_transfer_params(struct hidma_lldev *lldev, u32 tre_ch,
+				  dma_addr_t src, dma_addr_t dest, u32 len,
+				  u32 flags)
+{
+	struct hidma_tre *tre;
+	u32 *tre_local;
+
+	if (tre_ch >= lldev->nr_tres) {
+		dev_err(lldev->dev,
+			"invalid TRE number in transfer params:%d", tre_ch);
+		return;
+	}
+
+	tre = &lldev->trepool[tre_ch];
+	if (atomic_read(&tre->allocated) != true) {
+		dev_err(lldev->dev,
+			"trying to set params on an unused TRE:%d", tre_ch);
+		return;
+	}
+
+	tre_local = &tre->tre_local[0];
+	tre_local[HIDMA_TRE_LEN_IDX] = len;
+	tre_local[HIDMA_TRE_SRC_LOW_IDX] = lower_32_bits(src);
+	tre_local[HIDMA_TRE_SRC_HI_IDX] = upper_32_bits(src);
+	tre_local[HIDMA_TRE_DEST_LOW_IDX] = lower_32_bits(dest);
+	tre_local[HIDMA_TRE_DEST_HI_IDX] = upper_32_bits(dest);
+	tre->int_flags = flags;
+}
+
+/*
+ * Called during initialization and after an error condition
+ * to restore hardware state.
+ */
+int hidma_ll_setup(struct hidma_lldev *lldev)
+{
+	int rc;
+	u64 addr;
+	u32 val;
+	u32 nr_tres = lldev->nr_tres;
+
+	lldev->pending_tre_count = 0;
+	lldev->tre_processed_off = 0;
+	lldev->evre_processed_off = 0;
+	lldev->tre_write_offset = 0;
+
+	/* disable interrupts */
+	hidma_ll_enable_irq(lldev, 0);
+
+	/* clear all pending interrupts */
+	val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
+	writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
+
+	rc = hidma_ll_reset(lldev);
+	if (rc)
+		return rc;
+
+	/*
+	 * Clear all pending interrupts again.
+	 * Otherwise, we observe reset complete interrupts.
+	 */
+	val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
+	writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
+
+	/* disable interrupts again after reset */
+	hidma_ll_enable_irq(lldev, 0);
+
+	addr = lldev->tre_ring_handle;
+	writel(lower_32_bits(addr), lldev->trca + HIDMA_TRCA_RING_LOW_REG);
+	writel(upper_32_bits(addr), lldev->trca + HIDMA_TRCA_RING_HIGH_REG);
+	writel(lldev->tre_ring_size, lldev->trca + HIDMA_TRCA_RING_LEN_REG);
+
+	addr = lldev->evre_ring_handle;
+	writel(lower_32_bits(addr), lldev->evca + HIDMA_EVCA_RING_LOW_REG);
+	writel(upper_32_bits(addr), lldev->evca + HIDMA_EVCA_RING_HIGH_REG);
+	writel(HIDMA_EVRE_SIZE * nr_tres,
+	       lldev->evca + HIDMA_EVCA_RING_LEN_REG);
+
+	/* support IRQ only for now */
+	val = readl(lldev->evca + HIDMA_EVCA_INTCTRL_REG);
+	val &= ~0xF;
+	val |= 0x1;
+	writel(val, lldev->evca + HIDMA_EVCA_INTCTRL_REG);
+
+	/* clear all pending interrupts and enable them */
+	writel(ENABLE_IRQS, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
+	hidma_ll_enable_irq(lldev, ENABLE_IRQS);
+
+	rc = hidma_ll_enable(lldev);
+	if (rc)
+		return rc;
+
+	return rc;
+}
+
+struct hidma_lldev *hidma_ll_init(struct device *dev, u32 nr_tres,
+				  void __iomem *trca, void __iomem *evca,
+				  u8 chidx)
+{
+	u32 required_bytes;
+	struct hidma_lldev *lldev;
+	int rc;
+
+	if (!trca || !evca || !dev || !nr_tres)
+		return NULL;
+
+	/* need at least four TREs */
+	if (nr_tres < 4)
+		return NULL;
+
+	/* need an extra space */
+	nr_tres += 1;
+
+	lldev = devm_kzalloc(dev, sizeof(struct hidma_lldev), GFP_KERNEL);
+	if (!lldev)
+		return NULL;
+
+	lldev->evca = evca;
+	lldev->trca = trca;
+	lldev->dev = dev;
+	lldev->trepool = devm_kcalloc(lldev->dev, nr_tres,
+				      sizeof(struct hidma_tre), GFP_KERNEL);
+	if (!lldev->trepool)
+		return NULL;
+
+	required_bytes = sizeof(lldev->pending_tre_list[0]);
+	lldev->pending_tre_list = devm_kcalloc(dev, nr_tres, required_bytes,
+					       GFP_KERNEL);
+	if (!lldev->pending_tre_list)
+		return NULL;
+
+	lldev->tx_status_list = devm_kcalloc(dev, nr_tres,
+					     sizeof(lldev->tx_status_list[0]),
+					     GFP_KERNEL);
+	if (!lldev->tx_status_list)
+		return NULL;
+
+	lldev->tre_ring = dmam_alloc_coherent(dev,
+					      (HIDMA_TRE_SIZE + 1) * nr_tres,
+					      &lldev->tre_ring_handle,
+					      GFP_KERNEL);
+	if (!lldev->tre_ring)
+		return NULL;
+
+	memset(lldev->tre_ring, 0, (HIDMA_TRE_SIZE + 1) * nr_tres);
+	lldev->tre_ring_size = HIDMA_TRE_SIZE * nr_tres;
+	lldev->nr_tres = nr_tres;
+
+	/* the TRE ring has to be TRE_SIZE aligned */
+	if (!IS_ALIGNED(lldev->tre_ring_handle, HIDMA_TRE_SIZE)) {
+		u8 tre_ring_shift;
+
+		tre_ring_shift = lldev->tre_ring_handle % HIDMA_TRE_SIZE;
+		tre_ring_shift = HIDMA_TRE_SIZE - tre_ring_shift;
+		lldev->tre_ring_handle += tre_ring_shift;
+		lldev->tre_ring += tre_ring_shift;
+	}
+
+	lldev->evre_ring = dmam_alloc_coherent(dev,
+					       (HIDMA_EVRE_SIZE + 1) * nr_tres,
+					       &lldev->evre_ring_handle,
+					       GFP_KERNEL);
+	if (!lldev->evre_ring)
+		return NULL;
+
+	memset(lldev->evre_ring, 0, (HIDMA_EVRE_SIZE + 1) * nr_tres);
+	lldev->evre_ring_size = HIDMA_EVRE_SIZE * nr_tres;
+
+	/* the EVRE ring has to be EVRE_SIZE aligned */
+	if (!IS_ALIGNED(lldev->evre_ring_handle, HIDMA_EVRE_SIZE)) {
+		u8 evre_ring_shift;
+
+		evre_ring_shift = lldev->evre_ring_handle % HIDMA_EVRE_SIZE;
+		evre_ring_shift = HIDMA_EVRE_SIZE - evre_ring_shift;
+		lldev->evre_ring_handle += evre_ring_shift;
+		lldev->evre_ring += evre_ring_shift;
+	}
+	lldev->nr_tres = nr_tres;
+	lldev->chidx = chidx;
+
+	rc = kfifo_alloc(&lldev->handoff_fifo,
+			 nr_tres * sizeof(struct hidma_tre *), GFP_KERNEL);
+	if (rc)
+		return NULL;
+
+	rc = hidma_ll_setup(lldev);
+	if (rc)
+		return NULL;
+
+	spin_lock_init(&lldev->lock);
+	tasklet_init(&lldev->task, hidma_ll_tre_complete, (unsigned long)lldev);
+	lldev->initialized = 1;
+	hidma_ll_enable_irq(lldev, ENABLE_IRQS);
+	return lldev;
+}
+
+int hidma_ll_uninit(struct hidma_lldev *lldev)
+{
+	int rc = 0;
+	u32 val;
+
+	if (!lldev)
+		return -ENODEV;
+
+	if (lldev->initialized) {
+		u32 required_bytes;
+
+		lldev->initialized = 0;
+
+		required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
+		tasklet_kill(&lldev->task);
+		memset(lldev->trepool, 0, required_bytes);
+		lldev->trepool = NULL;
+		lldev->pending_tre_count = 0;
+		lldev->tre_write_offset = 0;
+
+		rc = hidma_ll_reset(lldev);
+
+		/*
+		 * Clear all pending interrupts again.
+		 * Otherwise, we observe reset complete interrupts.
+		 */
+		val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
+		writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
+		hidma_ll_enable_irq(lldev, 0);
+	}
+	return rc;
+}
+
+irqreturn_t hidma_ll_inthandler(int chirq, void *arg)
+{
+	struct hidma_lldev *lldev = arg;
+
+	hidma_ll_int_handler_internal(lldev);
+	return IRQ_HANDLED;
+}
+
+enum dma_status hidma_ll_status(struct hidma_lldev *lldev, u32 tre_ch)
+{
+	enum dma_status ret = DMA_ERROR;
+	unsigned long flags;
+	u8 err_code;
+
+	spin_lock_irqsave(&lldev->lock, flags);
+	err_code = lldev->tx_status_list[tre_ch].err_code;
+
+	if (err_code & HIDMA_EVRE_STATUS_COMPLETE)
+		ret = DMA_COMPLETE;
+	else if (err_code & HIDMA_EVRE_STATUS_ERROR)
+		ret = DMA_ERROR;
+	else
+		ret = DMA_IN_PROGRESS;
+	spin_unlock_irqrestore(&lldev->lock, flags);
+
+	return ret;
+}
-- 
1.8.2.1

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[Vinod Koul]

On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:

> + * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
> + * IOMMU latency will be built into the data movement time. By the time
> + * interrupt happens, IOMMU lookups + data movement has already taken place.

Do you mean dmaengine API or dma mapping API here? Where is you IOMMU
located wrt to dma controller?

> + *
> + * While the first read in a typical PCI endpoint ISR flushes all outstanding
> + * requests traditionally to the destination, this concept does not apply
> + * here for this HW.
> + */
> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
> +{
> +	u32 status;
> +	u32 enable;
> +	u32 cause;
> +
> +	/*
> +	 * Fine tuned for this HW...
> +	 *
> +	 * This ISR has been designed for this particular hardware. Relaxed
> +	 * read and write accessors are used for performance reasons due to
> +	 * interrupt delivery guarantees. Do not copy this code blindly and
> +	 * expect that to work.
> +	 */
> +	status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
> +	enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
> +	cause = status & enable;
> +
> +	while (cause) {
> +		if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
> +		    (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
> +		    (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
> +		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
> +		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {

Switch please

> +			u8 err_code = HIDMA_EVRE_STATUS_ERROR;
> +			u8 err_info = 0xFF;
> +
> +			/* Clear out pending interrupts */
> +			writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
> +
> +			dev_err(lldev->dev, "error 0x%x, resetting...\n",
> +				cause);
> +
> +			hidma_cleanup_pending_tre(lldev, err_info, err_code);
> +
> +			/* reset the channel for recovery */
> +			if (hidma_ll_setup(lldev)) {

should this be done in ISR?

> +int hidma_ll_resume(struct hidma_lldev *lldev)
> +{
> +	return hidma_ll_enable(lldev);
> +}

why do we need this empty function, use hidma_ll_enable.

> +bool hidma_ll_isenabled(struct hidma_lldev *lldev)
> +{
> +	u32 val;
> +
> +	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
> +	lldev->trch_state = HIDMA_CH_STATE(val);
> +	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
> +	lldev->evch_state = HIDMA_CH_STATE(val);
> +
> +	/* both channels have to be enabled before calling this function */
> +	if (((lldev->trch_state == HIDMA_CH_ENABLED) ||
> +	     (lldev->trch_state == HIDMA_CH_RUNNING)) &&
> +	    ((lldev->evch_state == HIDMA_CH_ENABLED) ||
> +	     (lldev->evch_state == HIDMA_CH_RUNNING)))
> +		return true;

hmmm this looks hard to read, why not do:

is_chan_enabled(state)
{
	switch (state) {
	case HIDMA_CH_ENABLED:
	case HIDMA_CH_RUNNING:
		return true;
	default:
		return false;
}

and then :

	if (is_chan_enabled(lldev->trch_state) &&
			is_chan_enabled(lldev->evch_state))


> +void hidma_ll_start(struct hidma_lldev *lldev)
> +{
> +	hidma_ll_hw_start(lldev);
> +}

Another dummy :(

> +/*
> + * Note that even though we stop this channel
> + * if there is a pending transaction in flight
> + * it will complete and follow the callback.
> + * This request will prevent further requests
> + * to be made.

Why the odd formating?

> +int hidma_ll_uninit(struct hidma_lldev *lldev)
> +{
> +	int rc = 0;
> +	u32 val;
> +
> +	if (!lldev)
> +		return -ENODEV;
> +
> +	if (lldev->initialized) {
> +		u32 required_bytes;
> +
> +		lldev->initialized = 0;
> +
> +		required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
> +		tasklet_kill(&lldev->task);
> +		memset(lldev->trepool, 0, required_bytes);
> +		lldev->trepool = NULL;
> +		lldev->pending_tre_count = 0;
> +		lldev->tre_write_offset = 0;
> +
> +		rc = hidma_ll_reset(lldev);
> +
> +		/*
> +		 * Clear all pending interrupts again.
> +		 * Otherwise, we observe reset complete interrupts.
> +		 */
> +		val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
> +		writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
> +		hidma_ll_enable_irq(lldev, 0);

uninit enables irq?

-- 
~Vinod

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[Sinan Kaya]

On 4/25/2016 11:28 PM, Vinod Koul wrote:
> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:
> 
>> + * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
>> + * IOMMU latency will be built into the data movement time. By the time
>> + * interrupt happens, IOMMU lookups + data movement has already taken place.
> 
> Do you mean dmaengine API or dma mapping API here? Where is you IOMMU
> located wrt to dma controller?

I mean DMA API in this context. The code works on dma_addr_t. That means the caller
takes care of DMA mapping before coming to this code. This code is not involved in
any of the DMA mapping calls or any other IOMMU related operations. All of this 
is abstracted from this code.

The IOMMU is right in front of this device. That's why, all IOMMU operations are 
inclusive into the data transfers. 

> 
>> + *
>> + * While the first read in a typical PCI endpoint ISR flushes all outstanding
>> + * requests traditionally to the destination, this concept does not apply
>> + * here for this HW.
>> + */
>> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
>> +{
>> +	u32 status;
>> +	u32 enable;
>> +	u32 cause;
>> +
>> +	/*
>> +	 * Fine tuned for this HW...
>> +	 *
>> +	 * This ISR has been designed for this particular hardware. Relaxed
>> +	 * read and write accessors are used for performance reasons due to
>> +	 * interrupt delivery guarantees. Do not copy this code blindly and
>> +	 * expect that to work.
>> +	 */
>> +	status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
>> +	enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
>> +	cause = status & enable;
>> +
>> +	while (cause) {
>> +		if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>> +		    (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>> +		    (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>> +		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>> +		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
> 
> Switch please

Cause is a combined status register. Let's say it contains 0x41. I need to check
if bit 0 or bit 6 is set in this value for each case condition. The value is not 0x40 
or 0x1. 

I created macro like this instead.

+#define HIDMA_IS_ERR_INTERRUPT(cause)                          \
+       (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
+       (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
+       (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))       || \
+       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))    || \
+       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))

and replaced the if statement as follows

if (HIDMA_IS_ERR_INTERRUPT(cause)) {

> 
>> +			u8 err_code = HIDMA_EVRE_STATUS_ERROR;
>> +			u8 err_info = 0xFF;
>> +
>> +			/* Clear out pending interrupts */
>> +			writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
>> +
>> +			dev_err(lldev->dev, "error 0x%x, resetting...\n",
>> +				cause);
>> +
>> +			hidma_cleanup_pending_tre(lldev, err_info, err_code);
>> +
>> +			/* reset the channel for recovery */
>> +			if (hidma_ll_setup(lldev)) {
> 
> should this be done in ISR?

I created a new tasklet called rst_task and posted the code there. 

 /*
+ * Abort all transactions and perform a reset.
+ */
+static void hidma_ll_abort(unsigned long arg)
+{
+       u8 err_code = HIDMA_EVRE_STATUS_ERROR;
+       u8 err_info = 0xFF;
+
+       dev_err(lldev->dev, "error 0x%x, resetting...\n",
+               cause);
+
+       hidma_cleanup_pending_tre(lldev, err_info, err_code);
+
+       /* reset the channel for recovery */
+       if (hidma_ll_setup(lldev)) {
+               dev_err(lldev->dev,
+                       "channel reinitialize failed after error\n");
+               return;
+       }
+       hidma_ll_control_irq(lldev, ENABLE_IRQS);
+}

+               if (HIDMA_IS_ERR_INTERRUPT(cause)) {
                        /* Clear out pending interrupts */
                        writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);

-                       dev_err(lldev->dev, "error 0x%x, resetting...\n",
-                               cause);
-
-                       hidma_cleanup_pending_tre(lldev, err_info, err_code);
-
-                       /* reset the channel for recovery */
-                       if (hidma_ll_setup(lldev)) {
-                               dev_err(lldev->dev,
-                                       "channel reinitialize failed after error\n");
-                               return;
-                       }
-                       hidma_ll_enable_irq(lldev, ENABLE_IRQS);
+                       tasklet_schedule(&lldev->rst_task);
 



> 
>> +int hidma_ll_resume(struct hidma_lldev *lldev)
>> +{
>> +	return hidma_ll_enable(lldev);
>> +}
> 
> why do we need this empty function, use hidma_ll_enable.

hidma_ll_enable is a common function that gets called from multiple places. 
hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
and resuming the DMA channel. 

> 
>> +bool hidma_ll_isenabled(struct hidma_lldev *lldev)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
>> +	lldev->trch_state = HIDMA_CH_STATE(val);
>> +	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
>> +	lldev->evch_state = HIDMA_CH_STATE(val);
>> +
>> +	/* both channels have to be enabled before calling this function */
>> +	if (((lldev->trch_state == HIDMA_CH_ENABLED) ||
>> +	     (lldev->trch_state == HIDMA_CH_RUNNING)) &&
>> +	    ((lldev->evch_state == HIDMA_CH_ENABLED) ||
>> +	     (lldev->evch_state == HIDMA_CH_RUNNING)))
>> +		return true;
> 
> hmmm this looks hard to read, why not do:
> 
> is_chan_enabled(state)
> {
> 	switch (state) {
> 	case HIDMA_CH_ENABLED:
> 	case HIDMA_CH_RUNNING:
> 		return true;
> 	default:
> 		return false;
> }
> 
> and then :
> 
> 	if (is_chan_enabled(lldev->trch_state) &&
> 			is_chan_enabled(lldev->evch_state))
> 
Sure, makes sense.

> 
>> +void hidma_ll_start(struct hidma_lldev *lldev)
>> +{
>> +	hidma_ll_hw_start(lldev);
>> +}
> 
> Another dummy :(

renamed hidma_ll_hw_start to hidma_ll_start and removed this function.

> 
>> +/*
>> + * Note that even though we stop this channel
>> + * if there is a pending transaction in flight
>> + * it will complete and follow the callback.
>> + * This request will prevent further requests
>> + * to be made.
> 
> Why the odd formating?

aligned to 75 characters.

> 
>> +int hidma_ll_uninit(struct hidma_lldev *lldev)
>> +{
>> +	int rc = 0;
>> +	u32 val;
>> +
>> +	if (!lldev)
>> +		return -ENODEV;
>> +
>> +	if (lldev->initialized) {
>> +		u32 required_bytes;
>> +
>> +		lldev->initialized = 0;
>> +
>> +		required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
>> +		tasklet_kill(&lldev->task);
>> +		memset(lldev->trepool, 0, required_bytes);
>> +		lldev->trepool = NULL;
>> +		lldev->pending_tre_count = 0;
>> +		lldev->tre_write_offset = 0;
>> +
>> +		rc = hidma_ll_reset(lldev);
>> +
>> +		/*
>> +		 * Clear all pending interrupts again.
>> +		 * Otherwise, we observe reset complete interrupts.
>> +		 */
>> +		val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
>> +		writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
>> +		hidma_ll_enable_irq(lldev, 0);
> 
> uninit enables irq?
> 
renamed to hidma_ll_control_irq. Second argument is the bits to enable. If 0, 
interrupts are disabled.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[Andy Shevchenko]

On Tue, Apr 26, 2016 at 6:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 4/25/2016 11:28 PM, Vinod Koul wrote:
>> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:

>>> +    while (cause) {
>>> +            if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>>> +                (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
>>
>> Switch please
>
> Cause is a combined status register. Let's say it contains 0x41. I need to check
> if bit 0 or bit 6 is set in this value for each case condition. The value is not 0x40
> or 0x1.
>
> I created macro like this instead.
>
> +#define HIDMA_IS_ERR_INTERRUPT(cause)                          \
> +       (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
> +       (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
> +       (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))       || \
> +       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))    || \
> +       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))

This looks overheaded.

#define HIDMA_XXX (BIT(a) | BIT (b) ... BIT(n))

>
> and replaced the if statement as follows
>
> if (HIDMA_IS_ERR_INTERRUPT(cause)) {

if (cause & HIDMA_XXX) {

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[Sinan Kaya]

On 4/26/2016 11:10 AM, Andy Shevchenko wrote:
> On Tue, Apr 26, 2016 at 6:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 4/25/2016 11:28 PM, Vinod Koul wrote:
>>> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:
> 
>>>> +    while (cause) {
>>>> +            if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>>>> +                (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
>>>
>>> Switch please
>>
>> Cause is a combined status register. Let's say it contains 0x41. I need to check
>> if bit 0 or bit 6 is set in this value for each case condition. The value is not 0x40
>> or 0x1.
>>
>> I created macro like this instead.
>>
>> +#define HIDMA_IS_ERR_INTERRUPT(cause)                          \
>> +       (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
>> +       (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
>> +       (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))       || \
>> +       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))    || \
>> +       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))
> 
> This looks overheaded.
> 
> #define HIDMA_XXX (BIT(a) | BIT (b) ... BIT(n))
> 
>>
>> and replaced the if statement as follows
>>
>> if (HIDMA_IS_ERR_INTERRUPT(cause)) {
> 
> if (cause & HIDMA_XXX) {
> 

This is even better.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[Vinod Koul]

On Tue, Apr 26, 2016 at 11:04:55AM -0400, Sinan Kaya wrote:
> On 4/25/2016 11:28 PM, Vinod Koul wrote:
> >> +
> >> +			/* reset the channel for recovery */
> >> +			if (hidma_ll_setup(lldev)) {
> > 
> > should this be done in ISR?
> 
> I created a new tasklet called rst_task and posted the code there. 

sounds better

> 
>  /*
> + * Abort all transactions and perform a reset.
> + */
> +static void hidma_ll_abort(unsigned long arg)
> +{
> +       u8 err_code = HIDMA_EVRE_STATUS_ERROR;
> +       u8 err_info = 0xFF;
> +
> +       dev_err(lldev->dev, "error 0x%x, resetting...\n",
> +               cause);

right justify this and others as well please

> >> +int hidma_ll_resume(struct hidma_lldev *lldev)
> >> +{
> >> +	return hidma_ll_enable(lldev);
> >> +}
> > 
> > why do we need this empty function, use hidma_ll_enable.
> 
> hidma_ll_enable is a common function that gets called from multiple places. 
> hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
> and resuming the DMA channel. 

is there a reason why we can't have the code in resume and that being called
internally as well as externally?

> >> +/*
> >> + * Note that even though we stop this channel
> >> + * if there is a pending transaction in flight
> >> + * it will complete and follow the callback.
> >> + * This request will prevent further requests
> >> + * to be made.
> > 
> > Why the odd formating?
> 
> aligned to 75 characters.

This seems to be 50 chars!

-- 
~Vinod

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[Sinan Kaya]

On 4/26/2016 12:24 PM, Vinod Koul wrote:
>> +
>> > +       dev_err(lldev->dev, "error 0x%x, resetting...\n",
>> > +               cause);
> right justify this and others as well please
> 

Can you please point me to other lines that need to be fixed please? It looks good
to me though it doesn't to you. I want to make sure that I'm touching the right ones.
You seem to prefer a tab following an open parenthesis in your code. Do you want me to
add a tab for every single multi-line such that it comes this for instance?


	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
				 	HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 
					1000, 10000); 

instead of

	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
				 HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
				 10000); 


>>>> > >> +int hidma_ll_resume(struct hidma_lldev *lldev)
>>>> > >> +{
>>>> > >> +	return hidma_ll_enable(lldev);
>>>> > >> +}
>>> > > 
>>> > > why do we need this empty function, use hidma_ll_enable.
>> > 
>> > hidma_ll_enable is a common function that gets called from multiple places. 
>> > hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
>> > and resuming the DMA channel. 
> is there a reason why we can't have the code in resume and that being called
> internally as well as externally?
> 

I'll have the pause code in OS layer call hidma_ll_enable and get rid of pause
function here.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

[Sinan Kaya]

On 4/28/2016 3:30 PM, Sinan Kaya wrote:
> On 4/26/2016 12:24 PM, Vinod Koul wrote:
>>> +
>>>> +       dev_err(lldev->dev, "error 0x%x, resetting...\n",
>>>> +               cause);
>> right justify this and others as well please
>>
> 
> Can you please point me to other lines that need to be fixed please? It looks good
> to me though it doesn't to you. I want to make sure that I'm touching the right ones.
> You seem to prefer a tab following an open parenthesis in your code. Do you want me to
> add a tab for every single multi-line such that it comes this for instance?
> 
> 
> 	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
> 				 	HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 
> 					1000, 10000); 
> 
> instead of
> 
> 	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
> 				 HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
> 				 10000); 
> 
> 
>>>>>>>> +int hidma_ll_resume(struct hidma_lldev *lldev)
>>>>>>>> +{
>>>>>>>> +	return hidma_ll_enable(lldev);
>>>>>>>> +}
>>>>>>
>>>>>> why do we need this empty function, use hidma_ll_enable.
>>>>
>>>> hidma_ll_enable is a common function that gets called from multiple places. 
>>>> hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
>>>> and resuming the DMA channel. 
>> is there a reason why we can't have the code in resume and that being called
>> internally as well as externally?
>>
> 
> I'll have the pause code in OS layer call hidma_ll_enable and get rid of pause
> function here.
> 


I posted v18 along with all the style corrections and other things you asked me.
Please let me know if there is anything outstanding.



-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Sinan Kaya]

Add debugfs hooks for debugging the execution behavior of the DMA
channel. The debugfs hooks get initialized by the probe function and
uninitialized by the remove function.

A stats file is created in debugfs. The stats file will show the
information about each HIDMA channel as well as each asynchronous job
queued and completed at a given time.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/dma/qcom/Makefile    |   2 +-
 drivers/dma/qcom/hidma.c     |   5 +-
 drivers/dma/qcom/hidma.h     |   2 +
 drivers/dma/qcom/hidma_dbg.c | 219 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 226 insertions(+), 2 deletions(-)
 create mode 100644 drivers/dma/qcom/hidma_dbg.c

diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile
index 6bf9267..4bfc38b 100644
--- a/drivers/dma/qcom/Makefile
+++ b/drivers/dma/qcom/Makefile
@@ -2,4 +2,4 @@ obj-$(CONFIG_QCOM_BAM_DMA) += bam_dma.o
 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
+hdma-objs        := hidma_ll.o hidma.o hidma_dbg.o
diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index f8960f1..8972508 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -1,7 +1,7 @@
 /*
  * Qualcomm Technologies HIDMA DMA engine interface
  *
- * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015-2016, 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
@@ -681,6 +681,7 @@ static int hidma_probe(struct platform_device *pdev)
 
 	dmadev->irq = chirq;
 	tasklet_init(&dmadev->task, hidma_issue_task, (unsigned long)dmadev);
+	hidma_debug_init(dmadev);
 	dev_info(&pdev->dev, "HI-DMA engine driver registration complete\n");
 	platform_set_drvdata(pdev, dmadev);
 	pm_runtime_mark_last_busy(dmadev->ddev.dev);
@@ -689,6 +690,7 @@ static int hidma_probe(struct platform_device *pdev)
 	return 0;
 
 uninit:
+	hidma_debug_uninit(dmadev);
 	hidma_ll_uninit(dmadev->lldev);
 dmafree:
 	if (dmadev)
@@ -706,6 +708,7 @@ static int hidma_remove(struct platform_device *pdev)
 	pm_runtime_get_sync(dmadev->ddev.dev);
 	dma_async_device_unregister(&dmadev->ddev);
 	devm_free_irq(dmadev->ddev.dev, dmadev->irq, dmadev->lldev);
+	hidma_debug_uninit(dmadev);
 	hidma_ll_uninit(dmadev->lldev);
 	hidma_free(dmadev);
 
diff --git a/drivers/dma/qcom/hidma.h b/drivers/dma/qcom/hidma.h
index c5eea65..22806a2 100644
--- a/drivers/dma/qcom/hidma.h
+++ b/drivers/dma/qcom/hidma.h
@@ -157,4 +157,6 @@ int hidma_ll_uninit(struct hidma_lldev *llhndl);
 irqreturn_t hidma_ll_inthandler(int irq, void *arg);
 void hidma_cleanup_pending_tre(struct hidma_lldev *llhndl, u8 err_info,
 				u8 err_code);
+int hidma_debug_init(struct hidma_dev *dmadev);
+void hidma_debug_uninit(struct hidma_dev *dmadev);
 #endif
diff --git a/drivers/dma/qcom/hidma_dbg.c b/drivers/dma/qcom/hidma_dbg.c
new file mode 100644
index 0000000..68e779e
--- /dev/null
+++ b/drivers/dma/qcom/hidma_dbg.c
@@ -0,0 +1,219 @@
+/*
+ * Qualcomm Technologies HIDMA debug file
+ *
+ * Copyright (c) 2015-2016, 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/debugfs.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/pm_runtime.h>
+
+#include "hidma.h"
+
+static void hidma_ll_chstats(struct seq_file *s, void *llhndl, u32 tre_ch)
+{
+	struct hidma_lldev *lldev = llhndl;
+	struct hidma_tre *tre;
+	u32 length;
+	dma_addr_t src_start;
+	dma_addr_t dest_start;
+	u32 *tre_local;
+
+	if (tre_ch >= lldev->nr_tres) {
+		dev_err(lldev->dev, "invalid TRE number in chstats:%d", tre_ch);
+		return;
+	}
+	tre = &lldev->trepool[tre_ch];
+	seq_printf(s, "------Channel %d -----\n", tre_ch);
+	seq_printf(s, "allocated=%d\n", atomic_read(&tre->allocated));
+	seq_printf(s, "queued = 0x%x\n", tre->queued);
+	seq_printf(s, "err_info = 0x%x\n",
+		   lldev->tx_status_list[tre->idx].err_info);
+	seq_printf(s, "err_code = 0x%x\n",
+		   lldev->tx_status_list[tre->idx].err_code);
+	seq_printf(s, "status = 0x%x\n", tre->status);
+	seq_printf(s, "idx = 0x%x\n", tre->idx);
+	seq_printf(s, "dma_sig = 0x%x\n", tre->dma_sig);
+	seq_printf(s, "dev_name=%s\n", tre->dev_name);
+	seq_printf(s, "callback=%p\n", tre->callback);
+	seq_printf(s, "data=%p\n", tre->data);
+	seq_printf(s, "tre_index = 0x%x\n", tre->tre_index);
+
+	tre_local = &tre->tre_local[0];
+	src_start = tre_local[HIDMA_TRE_SRC_LOW_IDX];
+	src_start = ((u64) (tre_local[HIDMA_TRE_SRC_HI_IDX]) << 32) + src_start;
+	dest_start = tre_local[HIDMA_TRE_DEST_LOW_IDX];
+	dest_start += ((u64) (tre_local[HIDMA_TRE_DEST_HI_IDX]) << 32);
+	length = tre_local[HIDMA_TRE_LEN_IDX];
+
+	seq_printf(s, "src=%pap\n", &src_start);
+	seq_printf(s, "dest=%pap\n", &dest_start);
+	seq_printf(s, "length = 0x%x\n", length);
+}
+
+static void hidma_ll_devstats(struct seq_file *s, void *llhndl)
+{
+	struct hidma_lldev *lldev = llhndl;
+
+	seq_puts(s, "------Device -----\n");
+	seq_printf(s, "lldev init = 0x%x\n", lldev->initialized);
+	seq_printf(s, "trch_state = 0x%x\n", lldev->trch_state);
+	seq_printf(s, "evch_state = 0x%x\n", lldev->evch_state);
+	seq_printf(s, "chidx = 0x%x\n", lldev->chidx);
+	seq_printf(s, "nr_tres = 0x%x\n", lldev->nr_tres);
+	seq_printf(s, "trca=%p\n", lldev->trca);
+	seq_printf(s, "tre_ring=%p\n", lldev->tre_ring);
+	seq_printf(s, "tre_ring_handle=%pap\n", &lldev->tre_ring_handle);
+	seq_printf(s, "tre_ring_size = 0x%x\n", lldev->tre_ring_size);
+	seq_printf(s, "tre_processed_off = 0x%x\n", lldev->tre_processed_off);
+	seq_printf(s, "pending_tre_count=%d\n", lldev->pending_tre_count);
+	seq_printf(s, "evca=%p\n", lldev->evca);
+	seq_printf(s, "evre_ring=%p\n", lldev->evre_ring);
+	seq_printf(s, "evre_ring_handle=%pap\n", &lldev->evre_ring_handle);
+	seq_printf(s, "evre_ring_size = 0x%x\n", lldev->evre_ring_size);
+	seq_printf(s, "evre_processed_off = 0x%x\n", lldev->evre_processed_off);
+	seq_printf(s, "tre_write_offset = 0x%x\n", lldev->tre_write_offset);
+}
+
+/*
+ * hidma_chan_stats: display HIDMA channel statistics
+ *
+ * Display the statistics for the current HIDMA virtual channel device.
+ */
+static int hidma_chan_stats(struct seq_file *s, void *unused)
+{
+	struct hidma_chan *mchan = s->private;
+	struct hidma_desc *mdesc;
+	struct hidma_dev *dmadev = mchan->dmadev;
+
+	pm_runtime_get_sync(dmadev->ddev.dev);
+	seq_printf(s, "paused=%u\n", mchan->paused);
+	seq_printf(s, "dma_sig=%u\n", mchan->dma_sig);
+	seq_puts(s, "prepared\n");
+	list_for_each_entry(mdesc, &mchan->prepared, node)
+		hidma_ll_chstats(s, mchan->dmadev->lldev, mdesc->tre_ch);
+
+	seq_puts(s, "active\n");
+	list_for_each_entry(mdesc, &mchan->active, node)
+		hidma_ll_chstats(s, mchan->dmadev->lldev, mdesc->tre_ch);
+
+	seq_puts(s, "completed\n");
+	list_for_each_entry(mdesc, &mchan->completed, node)
+		hidma_ll_chstats(s, mchan->dmadev->lldev, mdesc->tre_ch);
+
+	hidma_ll_devstats(s, mchan->dmadev->lldev);
+	pm_runtime_mark_last_busy(dmadev->ddev.dev);
+	pm_runtime_put_autosuspend(dmadev->ddev.dev);
+	return 0;
+}
+
+/*
+ * hidma_dma_info: display HIDMA device info
+ *
+ * Display the info for the current HIDMA device.
+ */
+static int hidma_dma_info(struct seq_file *s, void *unused)
+{
+	struct hidma_dev *dmadev = s->private;
+	resource_size_t sz;
+
+	seq_printf(s, "nr_descriptors=%d\n", dmadev->nr_descriptors);
+	seq_printf(s, "dev_trca=%p\n", &dmadev->dev_trca);
+	seq_printf(s, "dev_trca_phys=%pa\n", &dmadev->trca_resource->start);
+	sz = resource_size(dmadev->trca_resource);
+	seq_printf(s, "dev_trca_size=%pa\n", &sz);
+	seq_printf(s, "dev_evca=%p\n", &dmadev->dev_evca);
+	seq_printf(s, "dev_evca_phys=%pa\n", &dmadev->evca_resource->start);
+	sz = resource_size(dmadev->evca_resource);
+	seq_printf(s, "dev_evca_size=%pa\n", &sz);
+	return 0;
+}
+
+static int hidma_chan_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, hidma_chan_stats, inode->i_private);
+}
+
+static int hidma_dma_info_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, hidma_dma_info, inode->i_private);
+}
+
+static const struct file_operations hidma_chan_fops = {
+	.open = hidma_chan_stats_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static const struct file_operations hidma_dma_fops = {
+	.open = hidma_dma_info_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+void hidma_debug_uninit(struct hidma_dev *dmadev)
+{
+	debugfs_remove_recursive(dmadev->debugfs);
+	debugfs_remove_recursive(dmadev->stats);
+}
+
+int hidma_debug_init(struct hidma_dev *dmadev)
+{
+	int rc = 0;
+	int chidx = 0;
+	struct list_head *position = NULL;
+
+	dmadev->debugfs = debugfs_create_dir(dev_name(dmadev->ddev.dev), NULL);
+	if (!dmadev->debugfs) {
+		rc = -ENODEV;
+		return rc;
+	}
+
+	/* walk through the virtual channel list */
+	list_for_each(position, &dmadev->ddev.channels) {
+		struct hidma_chan *chan;
+
+		chan = list_entry(position, struct hidma_chan,
+				  chan.device_node);
+		sprintf(chan->dbg_name, "chan%d", chidx);
+		chan->debugfs = debugfs_create_dir(chan->dbg_name,
+						   dmadev->debugfs);
+		if (!chan->debugfs) {
+			rc = -ENOMEM;
+			goto cleanup;
+		}
+		chan->stats = debugfs_create_file("stats", S_IRUGO,
+						  chan->debugfs, chan,
+						  &hidma_chan_fops);
+		if (!chan->stats) {
+			rc = -ENOMEM;
+			goto cleanup;
+		}
+		chidx++;
+	}
+
+	dmadev->stats = debugfs_create_file("stats", S_IRUGO,
+					    dmadev->debugfs, dmadev,
+					    &hidma_dma_fops);
+	if (!dmadev->stats) {
+		rc = -ENOMEM;
+		goto cleanup;
+	}
+
+	return 0;
+cleanup:
+	hidma_debug_uninit(dmadev);
+	return rc;
+}
-- 
1.8.2.1

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Vinod Koul]

On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:

> +static int hidma_chan_stats(struct seq_file *s, void *unused)
> +{
> +	struct hidma_chan *mchan = s->private;
> +	struct hidma_desc *mdesc;
> +	struct hidma_dev *dmadev = mchan->dmadev;
> +
> +	pm_runtime_get_sync(dmadev->ddev.dev);

debug shouldn't power up device, why do you want to do that

-- 
~Vinod

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[okaya]

On 2016-04-25 23:30, Vinod Koul wrote:
> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
> 
>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>> +{
>> +	struct hidma_chan *mchan = s->private;
>> +	struct hidma_desc *mdesc;
>> +	struct hidma_dev *dmadev = mchan->dmadev;
>> +
>> +	pm_runtime_get_sync(dmadev->ddev.dev);
> 
> debug shouldn't power up device, why do you want to do that


Clocks are turned off while the hw is idle. I can’t reach hw registers 
without restoring power.

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Vinod Koul]

On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
> On 2016-04-25 23:30, Vinod Koul wrote:
> >On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
> >
> >>+static int hidma_chan_stats(struct seq_file *s, void *unused)
> >>+{
> >>+	struct hidma_chan *mchan = s->private;
> >>+	struct hidma_desc *mdesc;
> >>+	struct hidma_dev *dmadev = mchan->dmadev;
> >>+
> >>+	pm_runtime_get_sync(dmadev->ddev.dev);
> >
> >debug shouldn't power up device, why do you want to do that
> 
> 
> Clocks are turned off while the hw is idle. I can’t reach hw
> registers without restoring power.

Hmm, have you thought about using regmap?

-- 
~Vinod

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Sinan Kaya]

On 4/26/2016 12:25 PM, Vinod Koul wrote:
> On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
>> On 2016-04-25 23:30, Vinod Koul wrote:
>>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>>>
>>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>>>> +{
>>>> +	struct hidma_chan *mchan = s->private;
>>>> +	struct hidma_desc *mdesc;
>>>> +	struct hidma_dev *dmadev = mchan->dmadev;
>>>> +
>>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
>>>
>>> debug shouldn't power up device, why do you want to do that
>>
>>
>> Clocks are turned off while the hw is idle. I can’t reach hw
>> registers without restoring power.
> 
> Hmm, have you thought about using regmap?
> 

To be honest, I didn't know what regmap is but I just read some code
and looked at how it is used. Feel free to correct me if I got it 
wrong. 

Regmap seems to be designed for *slow* speed peripherals to improve frequent
accesses by the SW. It looks like it is used by MFD, SPI and I2C drivers.

It seems to cache the register contents and flush/invalidate them only when
needed.

The MMIO version seems to be assuming the presence of device-tree like CLK
API which doesn't exist on ACPI systems and is not portable.

My reaction is that it is a lot of code with no added functionality to what
HIDMA driver is trying to achieve. 

Given that the use case here is only for debug purposes; I think it is OK 
to keep this runtime call here. I don't want to add any overhead into the
existing code just to support the debug use case.  

None of my register read/writes are slow. This file will only be used to 
troubleshoot customer issues.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Vinod Koul]

On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
> On 4/26/2016 12:25 PM, Vinod Koul wrote:
> > On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
> >> On 2016-04-25 23:30, Vinod Koul wrote:
> >>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
> >>>
> >>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
> >>>> +{
> >>>> +	struct hidma_chan *mchan = s->private;
> >>>> +	struct hidma_desc *mdesc;
> >>>> +	struct hidma_dev *dmadev = mchan->dmadev;
> >>>> +
> >>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
> >>>
> >>> debug shouldn't power up device, why do you want to do that
> >>
> >>
> >> Clocks are turned off while the hw is idle. I can’t reach hw
> >> registers without restoring power.
> > 
> > Hmm, have you thought about using regmap?
> > 
> 
> To be honest, I didn't know what regmap is but I just read some code
> and looked at how it is used. Feel free to correct me if I got it 
> wrong. 
> 
> Regmap seems to be designed for *slow* speed peripherals to improve frequent
> accesses by the SW. It looks like it is used by MFD, SPI and I2C drivers.
> 
> It seems to cache the register contents and flush/invalidate them only when
> needed.
> 
> The MMIO version seems to be assuming the presence of device-tree like CLK
> API which doesn't exist on ACPI systems and is not portable.
> 
> My reaction is that it is a lot of code with no added functionality to what
> HIDMA driver is trying to achieve. 
> 
> Given that the use case here is only for debug purposes; I think it is OK 
> to keep this runtime call here. I don't want to add any overhead into the
> existing code just to support the debug use case.  
> 
> None of my register read/writes are slow. This file will only be used to 
> troubleshoot customer issues.

$ is always faster than MMIO. This way you can give reg contents to users
without waking up hw.

Also we at Intel use regmap on ACPI systems without CLK API

-- 
~Vinod

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Marc Zyngier]

On 27/04/16 09:15, Vinod Koul wrote:
> On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
>> On 4/26/2016 12:25 PM, Vinod Koul wrote:
>>> On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
>>>> On 2016-04-25 23:30, Vinod Koul wrote:
>>>>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>>>>>
>>>>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>>>>>> +{
>>>>>> +	struct hidma_chan *mchan = s->private;
>>>>>> +	struct hidma_desc *mdesc;
>>>>>> +	struct hidma_dev *dmadev = mchan->dmadev;
>>>>>> +
>>>>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
>>>>>
>>>>> debug shouldn't power up device, why do you want to do that
>>>>
>>>>
>>>> Clocks are turned off while the hw is idle. I can’t reach hw
>>>> registers without restoring power.
>>>
>>> Hmm, have you thought about using regmap?
>>>
>>
>> To be honest, I didn't know what regmap is but I just read some code
>> and looked at how it is used. Feel free to correct me if I got it 
>> wrong. 
>>
>> Regmap seems to be designed for *slow* speed peripherals to improve frequent
>> accesses by the SW. It looks like it is used by MFD, SPI and I2C drivers.
>>
>> It seems to cache the register contents and flush/invalidate them only when
>> needed.
>>
>> The MMIO version seems to be assuming the presence of device-tree like CLK
>> API which doesn't exist on ACPI systems and is not portable.
>>
>> My reaction is that it is a lot of code with no added functionality to what
>> HIDMA driver is trying to achieve. 
>>
>> Given that the use case here is only for debug purposes; I think it is OK 
>> to keep this runtime call here. I don't want to add any overhead into the
>> existing code just to support the debug use case.  
>>
>> None of my register read/writes are slow. This file will only be used to 
>> troubleshoot customer issues.

I'd recommend you actually run perf on a a few of your MMIO accesses. I
believe the result will be eye opening. On the KVM side, we've trimmed
our MMIO access as much as possible, using a memory-based cache (similar
to regmap in concept). This has made some code paths about 40% faster.

> $ is always faster than MMIO. This way you can give reg contents to users
> without waking up hw.

Indeed. MMIO access sucks rocks, even on a very fast box. Actually, the
faster the box is, the slower MMIO feels (compared to memory).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[okaya]

On 2016-04-27 04:47, Marc Zyngier wrote:
> On 27/04/16 09:15, Vinod Koul wrote:
>> On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
>>> On 4/26/2016 12:25 PM, Vinod Koul wrote:
>>>> On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org 
>>>> wrote:
>>>>> On 2016-04-25 23:30, Vinod Koul wrote:
>>>>>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>>>>>> 
>>>>>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>>>>>>> +{
>>>>>>> +	struct hidma_chan *mchan = s->private;
>>>>>>> +	struct hidma_desc *mdesc;
>>>>>>> +	struct hidma_dev *dmadev = mchan->dmadev;
>>>>>>> +
>>>>>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
>>>>>> 
>>>>>> debug shouldn't power up device, why do you want to do that
>>>>> 
>>>>> 
>>>>> Clocks are turned off while the hw is idle. I can’t reach hw
>>>>> registers without restoring power.
>>>> 
>>>> Hmm, have you thought about using regmap?
>>>> 
>>> 
>>> To be honest, I didn't know what regmap is but I just read some code
>>> and looked at how it is used. Feel free to correct me if I got it
>>> wrong.
>>> 
>>> Regmap seems to be designed for *slow* speed peripherals to improve 
>>> frequent
>>> accesses by the SW. It looks like it is used by MFD, SPI and I2C 
>>> drivers.
>>> 
>>> It seems to cache the register contents and flush/invalidate them 
>>> only when
>>> needed.
>>> 
>>> The MMIO version seems to be assuming the presence of device-tree 
>>> like CLK
>>> API which doesn't exist on ACPI systems and is not portable.
>>> 
>>> My reaction is that it is a lot of code with no added functionality 
>>> to what
>>> HIDMA driver is trying to achieve.
>>> 
>>> Given that the use case here is only for debug purposes; I think it 
>>> is OK
>>> to keep this runtime call here. I don't want to add any overhead into 
>>> the
>>> existing code just to support the debug use case.
>>> 
>>> None of my register read/writes are slow. This file will only be used 
>>> to
>>> troubleshoot customer issues.
> 
> I'd recommend you actually run perf on a a few of your MMIO accesses. I
> believe the result will be eye opening. On the KVM side, we've trimmed
> our MMIO access as much as possible, using a memory-based cache 
> (similar
> to regmap in concept). This has made some code paths about 40% faster.
> 
>> $ is always faster than MMIO. This way you can give reg contents to 
>> users
>> without waking up hw.
> 
> Indeed. MMIO access sucks rocks, even on a very fast box. Actually, the
> faster the box is, the slower MMIO feels (compared to memory).
> 
> Thanks,
> 
> 	M.


Agreed. However, I need to understand how regmap really works under the 
covers and whether it is compatible with the hardware.

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[okaya]

On 2016-04-27 04:15, Vinod Koul wrote:
> On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
>> On 4/26/2016 12:25 PM, Vinod Koul wrote:
>> > On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
>> >> On 2016-04-25 23:30, Vinod Koul wrote:
>> >>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>> >>>
>> >>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>> >>>> +{
>> >>>> +	struct hidma_chan *mchan = s->private;
>> >>>> +	struct hidma_desc *mdesc;
>> >>>> +	struct hidma_dev *dmadev = mchan->dmadev;
>> >>>> +
>> >>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
>> >>>
>> >>> debug shouldn't power up device, why do you want to do that
>> >>
>> >>
>> >> Clocks are turned off while the hw is idle. I can’t reach hw
>> >> registers without restoring power.
>> >
>> > Hmm, have you thought about using regmap?
>> >
>> 
>> To be honest, I didn't know what regmap is but I just read some code
>> and looked at how it is used. Feel free to correct me if I got it
>> wrong.
>> 
>> Regmap seems to be designed for *slow* speed peripherals to improve 
>> frequent
>> accesses by the SW. It looks like it is used by MFD, SPI and I2C 
>> drivers.
>> 
>> It seems to cache the register contents and flush/invalidate them only 
>> when
>> needed.
>> 
>> The MMIO version seems to be assuming the presence of device-tree like 
>> CLK
>> API which doesn't exist on ACPI systems and is not portable.
>> 
>> My reaction is that it is a lot of code with no added functionality to 
>> what
>> HIDMA driver is trying to achieve.
>> 
>> Given that the use case here is only for debug purposes; I think it is 
>> OK
>> to keep this runtime call here. I don't want to add any overhead into 
>> the
>> existing code just to support the debug use case.
>> 
>> None of my register read/writes are slow. This file will only be used 
>> to
>> troubleshoot customer issues.
> 
> $ is always faster than MMIO. This way you can give reg contents to 
> users
> without waking up hw.
> 
> Also we at Intel use regmap on ACPI systems without CLK API

I can try and see the performance impact is. What happens to registers 
that hw updates like status registers. Those will be most interesting 
during debug. How does remap get updated for those? Is there a way to 
tell it not to cache certain registers

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Sinan Kaya]

On 4/27/2016 8:51 AM, okaya@codeaurora.org wrote:
> On 2016-04-27 04:15, Vinod Koul wrote:
>> On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
>>> On 4/26/2016 12:25 PM, Vinod Koul wrote:
>>> > On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
>>> >> On 2016-04-25 23:30, Vinod Koul wrote:
>>> >>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>>> >>>
>>> >>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>>> >>>> +{
>>> >>>> +    struct hidma_chan *mchan = s->private;
>>> >>>> +    struct hidma_desc *mdesc;
>>> >>>> +    struct hidma_dev *dmadev = mchan->dmadev;
>>> >>>> +
>>> >>>> +    pm_runtime_get_sync(dmadev->ddev.dev);
>>> >>>
>>> >>> debug shouldn't power up device, why do you want to do that
>>> >>
>>> >>
>>> >> Clocks are turned off while the hw is idle. I can’t reach hw
>>> >> registers without restoring power.
>>> >
>>> > Hmm, have you thought about using regmap?
>>> >
>>>
>>> To be honest, I didn't know what regmap is but I just read some code
>>> and looked at how it is used. Feel free to correct me if I got it
>>> wrong.
>>>
>>> Regmap seems to be designed for *slow* speed peripherals to improve frequent
>>> accesses by the SW. It looks like it is used by MFD, SPI and I2C drivers.
>>>
>>> It seems to cache the register contents and flush/invalidate them only when
>>> needed.
>>>
>>> The MMIO version seems to be assuming the presence of device-tree like CLK
>>> API which doesn't exist on ACPI systems and is not portable.
>>>
>>> My reaction is that it is a lot of code with no added functionality to what
>>> HIDMA driver is trying to achieve.
>>>
>>> Given that the use case here is only for debug purposes; I think it is OK
>>> to keep this runtime call here. I don't want to add any overhead into the
>>> existing code just to support the debug use case.
>>>
>>> None of my register read/writes are slow. This file will only be used to
>>> troubleshoot customer issues.
>>
>> $ is always faster than MMIO. This way you can give reg contents to users
>> without waking up hw.
>>
>> Also we at Intel use regmap on ACPI systems without CLK API
> 
> I can try and see the performance impact is. What happens to registers that hw updates like status registers. Those will be most interesting during debug. How does remap get updated for those? Is there a way to tell it not to cache certain registers

My evaluation turned out negative. The regmap code is nice for bus like peripherals
like I2C and SPI where everything is bitwise accessed. This is not the case
in this code. 

Regmap is a nice tool if used properly but it doesn't mean that it will work
in every single case. It doesn't match with the goal of this driver. 

As soon as I abstract register accesses, the regmap code writes all MMIO registers
with the readl variant functions.

Barriers are really expensive on ARM. I paid very special attention in the 
code to decide when to use relaxed version vs. the readl version. I lose 
all of this optimization. 

Since the clocks are restored only during the debug case, I don't see any
problems here. It is not worth the effort to do redo the whole thing and 
introduce errors as I see a lot of tripping points like regmap_sync variants.


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Vinod Koul]

On Sun, May 01, 2016 at 12:35:37AM -0400, Sinan Kaya wrote:
> >>> >
> >>> > Hmm, have you thought about using regmap?
> >>>
> >>> To be honest, I didn't know what regmap is but I just read some code
> >>> and looked at how it is used. Feel free to correct me if I got it
> >>> wrong.
> >>>
> >>> Regmap seems to be designed for *slow* speed peripherals to improve frequent
> >>> accesses by the SW. It looks like it is used by MFD, SPI and I2C drivers.
> >>>
> >>> It seems to cache the register contents and flush/invalidate them only when
> >>> needed.
> >>>
> >>> The MMIO version seems to be assuming the presence of device-tree like CLK
> >>> API which doesn't exist on ACPI systems and is not portable.
> >>>
> >>> My reaction is that it is a lot of code with no added functionality to what
> >>> HIDMA driver is trying to achieve.
> >>>
> >>> Given that the use case here is only for debug purposes; I think it is OK
> >>> to keep this runtime call here. I don't want to add any overhead into the
> >>> existing code just to support the debug use case.
> >>>
> >>> None of my register read/writes are slow. This file will only be used to
> >>> troubleshoot customer issues.
> >>
> >> $ is always faster than MMIO. This way you can give reg contents to users
> >> without waking up hw.
> >>
> >> Also we at Intel use regmap on ACPI systems without CLK API
> > 
> > I can try and see the performance impact is. What happens to registers that hw updates like status registers. Those will be most interesting during debug. How does remap get updated for those? Is there a way to tell it not to cache certain registers
> 
> My evaluation turned out negative. The regmap code is nice for bus like peripherals
> like I2C and SPI where everything is bitwise accessed. This is not the case
> in this code. 

I do not entirely agree with the statements here, it does give big benefit
on our systems with MMIO. I am going to ask Mark to comment on this, he
know better and understands ARM.

I am probably going to be okay with this not using regmap and it is debug
but you should give that a try in future for better performance and ofcourse
you can add to regmap to get a better model for your device

> Regmap is a nice tool if used properly but it doesn't mean that it will work
> in every single case. It doesn't match with the goal of this driver. 
> 
> As soon as I abstract register accesses, the regmap code writes all MMIO registers
> with the readl variant functions.
> 
> Barriers are really expensive on ARM. I paid very special attention in the 
> code to decide when to use relaxed version vs. the readl version. I lose 
> all of this optimization. 
> 
> Since the clocks are restored only during the debug case, I don't see any
> problems here. It is not worth the effort to do redo the whole thing and 
> introduce errors as I see a lot of tripping points like regmap_sync variants.

-- 
~Vinod

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On Mon, May 02, 2016 at 02:55:52PM +0530, Vinod Koul wrote:
> On Sun, May 01, 2016 at 12:35:37AM -0400, Sinan Kaya wrote:

> > My evaluation turned out negative. The regmap code is nice for bus like peripherals
> > like I2C and SPI where everything is bitwise accessed. This is not the case
> > in this code. 

> I do not entirely agree with the statements here, it does give big benefit
> on our systems with MMIO. I am going to ask Mark to comment on this, he
> know better and understands ARM.

> I am probably going to be okay with this not using regmap and it is debug
> but you should give that a try in future for better performance and ofcourse
> you can add to regmap to get a better model for your device

I've no idea what "this" is, sorry.  All I've got here is an enormous
backtrace with a bunch of the messages not even word wrapped.

Please be aware that I get CCed on so much irrelevant crap that copying
me into the middle of a thread about some other subsystem is likely to
get missed - almost always it's review of some patch I've got no
interest in.

> > Barriers are really expensive on ARM. I paid very special attention in the 
> > code to decide when to use relaxed version vs. the readl version. I lose 
> > all of this optimization. 

Drivers should not be using the relaxed accessors, they are there for
the generic code to build on not for drivers and they really need the
cache flush operations.  Getting the cache flush operations right,
especially on ARM, isn't easy and needs detailed review.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH V17 3/3] dmaengine: qcom_hidma: add support for object hierarchy

[Sinan Kaya]

In order to create a relationship model between the channels and the
management object, we are adding support for object hierarchy to the
drivers. This patch simplifies the userspace application development.
We will not have to traverse different firmware paths based on device
tree or ACPI based kernels.

No matter what flavor of kernel is used, objects will be represented as
platform devices.

The new layout is as follows:

hidmam_10: hidma-mgmt@0x5A000000 {
	compatible = "qcom,hidma-mgmt-1.0";
	...

	hidma_10: hidma@0x5a010000 {
			compatible = "qcom,hidma-1.0";
			...
	}
}

The hidma_mgmt_init detects each instance of the hidma-mgmt-1.0 objects
in device tree and calls into the channel driver to create platform devices
for each child of the management object.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 Documentation/ABI/testing/sysfs-platform-hidma |   9 ++
 drivers/dma/qcom/hidma.c                       |  39 ++++++++-
 drivers/dma/qcom/hidma_mgmt.c                  | 113 ++++++++++++++++++++++++-
 3 files changed, 156 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-hidma

diff --git a/Documentation/ABI/testing/sysfs-platform-hidma b/Documentation/ABI/testing/sysfs-platform-hidma
new file mode 100644
index 0000000..d364415
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-hidma
@@ -0,0 +1,9 @@
+What:		/sys/devices/platform/hidma-*/chid
+		/sys/devices/platform/QCOM8061:*/chid
+Date:		Dec 2015
+KernelVersion:	4.4
+Contact:	"Sinan Kaya <okaya@cudeaurora.org>"
+Description:
+		Contains the ID of the channel within the HIDMA instance.
+		It is used to associate a given HIDMA channel with the
+		priority and weight calls in the management interface.
diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index 8972508..1f48f6d 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -549,6 +549,43 @@ static irqreturn_t hidma_chirq_handler(int chirq, void *arg)
 	return hidma_ll_inthandler(chirq, lldev);
 }
 
+static ssize_t hidma_show_values(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct hidma_dev *mdev = platform_get_drvdata(pdev);
+
+	buf[0] = 0;
+
+	if (strcmp(attr->attr.name, "chid") == 0)
+		sprintf(buf, "%d\n", mdev->chidx);
+
+	return strlen(buf);
+}
+
+static int hidma_create_sysfs_entry(struct hidma_dev *dev, char *name,
+				    int mode)
+{
+	struct device_attribute *attrs;
+	char *name_copy;
+
+	attrs = devm_kmalloc(dev->ddev.dev, sizeof(struct device_attribute),
+			     GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
+
+	name_copy = devm_kstrdup(dev->ddev.dev, name, GFP_KERNEL);
+	if (!name_copy)
+		return -ENOMEM;
+
+	attrs->attr.name = name_copy;
+	attrs->attr.mode = mode;
+	attrs->show = hidma_show_values;
+	sysfs_attr_init(&attrs->attr);
+
+	return device_create_file(dev->ddev.dev, attrs);
+}
+
 static int hidma_probe(struct platform_device *pdev)
 {
 	struct hidma_dev *dmadev;
@@ -682,6 +719,7 @@ static int hidma_probe(struct platform_device *pdev)
 	dmadev->irq = chirq;
 	tasklet_init(&dmadev->task, hidma_issue_task, (unsigned long)dmadev);
 	hidma_debug_init(dmadev);
+	hidma_create_sysfs_entry(dmadev, "chid", S_IRUGO);
 	dev_info(&pdev->dev, "HI-DMA engine driver registration complete\n");
 	platform_set_drvdata(pdev, dmadev);
 	pm_runtime_mark_last_busy(dmadev->ddev.dev);
@@ -730,7 +768,6 @@ static const struct of_device_id hidma_match[] = {
 	{.compatible = "qcom,hidma-1.0",},
 	{},
 };
-
 MODULE_DEVICE_TABLE(of, hidma_match);
 
 static struct platform_driver hidma_driver = {
diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
index ef491b8..c0e3653 100644
--- a/drivers/dma/qcom/hidma_mgmt.c
+++ b/drivers/dma/qcom/hidma_mgmt.c
@@ -1,7 +1,7 @@
 /*
  * Qualcomm Technologies HIDMA DMA engine Management interface
  *
- * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015-2016, 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
@@ -17,13 +17,14 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/property.h>
-#include <linux/interrupt.h>
-#include <linux/platform_device.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
 #include <linux/bitops.h>
+#include <linux/dma-mapping.h>
 
 #include "hidma_mgmt.h"
 
@@ -298,5 +299,109 @@ static struct platform_driver hidma_mgmt_driver = {
 	},
 };
 
-module_platform_driver(hidma_mgmt_driver);
+#if defined(CONFIG_OF) && defined(CONFIG_OF_IRQ)
+static int object_counter;
+
+static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
+{
+	struct platform_device *pdev_parent = of_find_device_by_node(np);
+	struct platform_device_info pdevinfo;
+	struct of_phandle_args out_irq;
+	struct device_node *child;
+	struct resource *res;
+	const __be32 *cell;
+	int ret = 0, size, i, num;
+	u64 addr, addr_size;
+
+	for_each_available_child_of_node(np, child) {
+		struct resource *res_iter;
+		struct platform_device *new_pdev;
+
+		cell = of_get_property(child, "reg", &size);
+		if (!cell) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		size /= sizeof(*cell);
+		num = size /
+			(of_n_addr_cells(child) + of_n_size_cells(child)) + 1;
+
+		/* allocate a resource array */
+		res = kcalloc(num, sizeof(*res), GFP_KERNEL);
+		if (!res) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		/* read each reg value */
+		i = 0;
+		res_iter = res;
+		while (i < size) {
+			addr = of_read_number(&cell[i],
+					      of_n_addr_cells(child));
+			i += of_n_addr_cells(child);
+
+			addr_size = of_read_number(&cell[i],
+						   of_n_size_cells(child));
+			i += of_n_size_cells(child);
+
+			res_iter->start = addr;
+			res_iter->end = res_iter->start + addr_size - 1;
+			res_iter->flags = IORESOURCE_MEM;
+			res_iter++;
+		}
+
+		ret = of_irq_parse_one(child, 0, &out_irq);
+		if (ret)
+			goto out;
+
+		res_iter->start = irq_create_of_mapping(&out_irq);
+		res_iter->name = "hidma event irq";
+		res_iter->flags = IORESOURCE_IRQ;
+
+		memset(&pdevinfo, 0, sizeof(pdevinfo));
+		pdevinfo.fwnode = &child->fwnode;
+		pdevinfo.parent = pdev_parent ? &pdev_parent->dev : NULL;
+		pdevinfo.name = child->name;
+		pdevinfo.id = object_counter++;
+		pdevinfo.res = res;
+		pdevinfo.num_res = num;
+		pdevinfo.data = NULL;
+		pdevinfo.size_data = 0;
+		pdevinfo.dma_mask = DMA_BIT_MASK(64);
+		new_pdev = platform_device_register_full(&pdevinfo);
+		if (!new_pdev) {
+			ret = -ENODEV;
+			goto out;
+		}
+		of_dma_configure(&new_pdev->dev, child);
+
+		kfree(res);
+		res = NULL;
+	}
+out:
+	kfree(res);
+
+	return ret;
+}
+#endif
+
+static int __init hidma_mgmt_init(void)
+{
+#if defined(CONFIG_OF) && defined(CONFIG_OF_IRQ)
+	struct device_node *child;
+
+	for (child = of_find_matching_node(NULL, hidma_mgmt_match); child;
+	     child = of_find_matching_node(child, hidma_mgmt_match)) {
+		/* device tree based firmware here */
+		hidma_mgmt_of_populate_channels(child);
+		of_node_put(child);
+	}
+#endif
+	platform_driver_register(&hidma_mgmt_driver);
+
+	return 0;
+}
+module_init(hidma_mgmt_init);
 MODULE_LICENSE("GPL v2");
-- 
1.8.2.1