[PATCH v5 0/3] add support for Mediatek High-Speed DMA controller on MT7622 and MT7623 SoC

[PATCH v5 0/3] add support for Mediatek High-Speed DMA controller on MT7622 and MT7623 SoC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Changes since v4:
- rebase to 4.16-rc1
- fold back patch 4 and 5 in v4 into the original commit
- add the missing header included
- fix typo and add more explanation in existing comments
- fix the comments with kernel-doc style which uses /** as the begin
- begin the multi-line comments with /*

Changes since v3:
- enhance dt-binding documents based on Rob's comments
- make consistent among all comments
- perfer ordering declarations longest to shortest
- fix warning reported from kbuild test robot
- add patch 4 and 5 to keep Fengguang and Julia's effort

Changes since v2:
- remove extra tasklets and refactor driver for letting descriptors being processed ASAP.
- add more comments stating the relevant logic
- remove unused macro and variables
- change to use the disclaimer with SPDX identifier
- add mt7622 support
- refine hardware initialization sequence
- add tx_status support for DMA_RESIDUE_GRANULARITY_SEGMENT
- refine remove handler with killing vc's tasklet

Changes since v1:
- fix typo in the commit message.
- delete status shown in the dt-binding example.

This patchset introduces support for MediaTek High-Speed DMA controller
(MTK-HSDMA) Currently, the driver is already tested successfully with
dmatest module on MT7622 and MT7623 SoC. 

MTK-HSDMA on MT7622/23 SoC has a single ring which is dedicated for
doing memory-to-memory transfer through ring-based descriptor management.
Even though there is only a single ring available inside HSDMA, the
driver is being extended to the support for multiple virtual channels
processing simultaneously by means of DMA_VIRTUAL_CHANNELS.

Sean Wang (3):
  dt-bindings: dmaengine: Add MediaTek High-Speed DMA controller
    bindings
  dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622
    and MT7623 SoC
  dmaengine: mediatek: update MAINTAINERS entry with MediaTek DMA driver

 .../devicetree/bindings/dma/mtk-hsdma.txt          |   33 +
 MAINTAINERS                                        |    9 +
 drivers/dma/Kconfig                                |    2 +
 drivers/dma/Makefile                               |    1 +
 drivers/dma/mediatek/Kconfig                       |   13 +
 drivers/dma/mediatek/Makefile                      |    1 +
 drivers/dma/mediatek/mtk-hsdma.c                   | 1054 ++++++++++++++++++++
 7 files changed, 1113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/mtk-hsdma.txt
 create mode 100644 drivers/dma/mediatek/Kconfig
 create mode 100644 drivers/dma/mediatek/Makefile
 create mode 100644 drivers/dma/mediatek/mtk-hsdma.c

-- 
2.7.4

[PATCH v5 1/3] dt-bindings: dmaengine: Add MediaTek High-Speed DMA controller bindings

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Document the devicetree bindings for MediaTek High-Speed DMA controller
which could be found on MT7623 SoC or other similar Mediatek SoCs.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../devicetree/bindings/dma/mtk-hsdma.txt          | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/mtk-hsdma.txt

diff --git a/Documentation/devicetree/bindings/dma/mtk-hsdma.txt b/Documentation/devicetree/bindings/dma/mtk-hsdma.txt
new file mode 100644
index 0000000..4bb31735
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/mtk-hsdma.txt
@@ -0,0 +1,33 @@
+MediaTek High-Speed DMA Controller
+==================================
+
+This device follows the generic DMA bindings defined in dma/dma.txt.
+
+Required properties:
+
+- compatible:	Must be one of
+		  "mediatek,mt7622-hsdma": for MT7622 SoC
+		  "mediatek,mt7623-hsdma": for MT7623 SoC
+- reg:		Should contain the register's base address and length.
+- interrupts:	Should contain a reference to the interrupt used by this
+		device.
+- clocks:	Should be the clock specifiers corresponding to the entry in
+		clock-names property.
+- clock-names:	Should contain "hsdma" entries.
+- power-domains: Phandle to the power domain that the device is part of
+- #dma-cells: 	The length of the DMA specifier, must be <1>. This one cell
+		in dmas property of a client device represents the channel
+		number.
+Example:
+
+        hsdma: dma-controller@1b007000 {
+		compatible = "mediatek,mt7623-hsdma";
+		reg = <0 0x1b007000 0 0x1000>;
+		interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_LOW>;
+		clocks = <&ethsys CLK_ETHSYS_HSDMA>;
+		clock-names = "hsdma";
+		power-domains = <&scpsys MT2701_POWER_DOMAIN_ETH>;
+		#dma-cells = <1>;
+	};
+
+DMA clients must use the format described in dma/dma.txt file.
-- 
2.7.4

[PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

MediaTek High-Speed DMA controller (HSDMA) on MT7622 and MT7623 SoC has
a single ring is dedicated to memory-to-memory transfer through ring based
descriptor management.

Even though there is only one physical ring available inside HSDMA, the
driver can be easily extended to the support of multiple virtual channels
processing simultaneously by means of DMA_VIRTUAL_CHANNELS effort.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Fengguang Wu <fengguang.wu@intel.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
---
 drivers/dma/Kconfig              |    2 +
 drivers/dma/Makefile             |    1 +
 drivers/dma/mediatek/Kconfig     |   13 +
 drivers/dma/mediatek/Makefile    |    1 +
 drivers/dma/mediatek/mtk-hsdma.c | 1054 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 1071 insertions(+)
 create mode 100644 drivers/dma/mediatek/Kconfig
 create mode 100644 drivers/dma/mediatek/Makefile
 create mode 100644 drivers/dma/mediatek/mtk-hsdma.c

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 27df3e2..86ace76 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -633,6 +633,8 @@ config ZX_DMA
 # driver files
 source "drivers/dma/bestcomm/Kconfig"
 
+source "drivers/dma/mediatek/Kconfig"
+
 source "drivers/dma/qcom/Kconfig"
 
 source "drivers/dma/dw/Kconfig"
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index b9dca8a..7a57b97 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -75,5 +75,6 @@ obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
 obj-$(CONFIG_ZX_DMA) += zx_dma.o
 obj-$(CONFIG_ST_FDMA) += st_fdma.o
 
+obj-y += mediatek/
 obj-y += qcom/
 obj-y += xilinx/
diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
new file mode 100644
index 0000000..27bac0b
--- /dev/null
+++ b/drivers/dma/mediatek/Kconfig
@@ -0,0 +1,13 @@
+
+config MTK_HSDMA
+	tristate "MediaTek High-Speed DMA controller support"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	select DMA_ENGINE
+	select DMA_VIRTUAL_CHANNELS
+	---help---
+	  Enable support for High-Speed DMA controller on MediaTek
+	  SoCs.
+
+	  This controller provides the channels which is dedicated to
+	  memory-to-memory transfer to offload from CPU through ring-
+	  based descriptor management.
diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
new file mode 100644
index 0000000..6e778f8
--- /dev/null
+++ b/drivers/dma/mediatek/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
diff --git a/drivers/dma/mediatek/mtk-hsdma.c b/drivers/dma/mediatek/mtk-hsdma.c
new file mode 100644
index 0000000..a0ae2cc
--- /dev/null
+++ b/drivers/dma/mediatek/mtk-hsdma.c
@@ -0,0 +1,1054 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for MediaTek High-Speed DMA Controller
+ *
+ * Copyright (c) 2017-2018 MediaTek Inc.
+ * Author: Sean Wang <sean.wang@mediatek.com>
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/jiffies.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_dma.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/refcount.h>
+#include <linux/slab.h>
+
+#include "../virt-dma.h"
+
+#define MTK_DMA_DEV KBUILD_MODNAME
+
+#define MTK_HSDMA_USEC_POLL		20
+#define MTK_HSDMA_TIMEOUT_POLL		200000
+#define MTK_HSDMA_DMA_BUSWIDTHS		BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED)
+
+/* The default number of virtual channel */
+#define MTK_HSDMA_NR_VCHANS		3
+
+/* Only one physical channel supported */
+#define MTK_HSDMA_NR_MAX_PCHANS		1
+
+/* Macro for physical descriptor (PD) manipulation */
+/* The number of PD which must be 2 of power */
+#define MTK_DMA_SIZE			64
+#define MTK_HSDMA_NEXT_DESP_IDX(x, y)	(((x) + 1) & ((y) - 1))
+#define MTK_HSDMA_LAST_DESP_IDX(x, y)	(((x) - 1) & ((y) - 1))
+#define MTK_HSDMA_MAX_LEN		0x3f80
+#define MTK_HSDMA_ALIGN_SIZE		4
+#define MTK_HSDMA_PLEN_MASK		0x3fff
+#define MTK_HSDMA_DESC_PLEN(x)		(((x) & MTK_HSDMA_PLEN_MASK) << 16)
+#define MTK_HSDMA_DESC_PLEN_GET(x)	(((x) >> 16) & MTK_HSDMA_PLEN_MASK)
+
+/* Registers for underlying ring manipulation */
+#define MTK_HSDMA_TX_BASE		0x0
+#define MTK_HSDMA_TX_CNT		0x4
+#define MTK_HSDMA_TX_CPU		0x8
+#define MTK_HSDMA_TX_DMA		0xc
+#define MTK_HSDMA_RX_BASE		0x100
+#define MTK_HSDMA_RX_CNT		0x104
+#define MTK_HSDMA_RX_CPU		0x108
+#define MTK_HSDMA_RX_DMA		0x10c
+
+/* Registers for global setup */
+#define MTK_HSDMA_GLO			0x204
+#define MTK_HSDMA_GLO_MULTI_DMA		BIT(10)
+#define MTK_HSDMA_TX_WB_DDONE		BIT(6)
+#define MTK_HSDMA_BURST_64BYTES		(0x2 << 4)
+#define MTK_HSDMA_GLO_RX_BUSY		BIT(3)
+#define MTK_HSDMA_GLO_RX_DMA		BIT(2)
+#define MTK_HSDMA_GLO_TX_BUSY		BIT(1)
+#define MTK_HSDMA_GLO_TX_DMA		BIT(0)
+#define MTK_HSDMA_GLO_DMA		(MTK_HSDMA_GLO_TX_DMA |	\
+					 MTK_HSDMA_GLO_RX_DMA)
+#define MTK_HSDMA_GLO_BUSY		(MTK_HSDMA_GLO_RX_BUSY | \
+					 MTK_HSDMA_GLO_TX_BUSY)
+#define MTK_HSDMA_GLO_DEFAULT		(MTK_HSDMA_GLO_TX_DMA | \
+					 MTK_HSDMA_GLO_RX_DMA | \
+					 MTK_HSDMA_TX_WB_DDONE | \
+					 MTK_HSDMA_BURST_64BYTES | \
+					 MTK_HSDMA_GLO_MULTI_DMA)
+
+/* Registers for reset */
+#define MTK_HSDMA_RESET			0x208
+#define MTK_HSDMA_RST_TX		BIT(0)
+#define MTK_HSDMA_RST_RX		BIT(16)
+
+/* Registers for interrupt control */
+#define MTK_HSDMA_DLYINT		0x20c
+#define MTK_HSDMA_RXDLY_INT_EN		BIT(15)
+
+/* Interrupt fires when the pending number's more than the specified */
+#define MTK_HSDMA_RXMAX_PINT(x)		(((x) & 0x7f) << 8)
+
+/* Interrupt fires when the pending time's more than the specified in 20 us */
+#define MTK_HSDMA_RXMAX_PTIME(x)	((x) & 0x7f)
+#define MTK_HSDMA_DLYINT_DEFAULT	(MTK_HSDMA_RXDLY_INT_EN | \
+					 MTK_HSDMA_RXMAX_PINT(20) | \
+					 MTK_HSDMA_RXMAX_PTIME(20))
+#define MTK_HSDMA_INT_STATUS		0x220
+#define MTK_HSDMA_INT_ENABLE		0x228
+#define MTK_HSDMA_INT_RXDONE		BIT(16)
+
+enum mtk_hsdma_vdesc_flag {
+	MTK_HSDMA_VDESC_FINISHED	= 0x01,
+};
+
+#define IS_MTK_HSDMA_VDESC_FINISHED(x) ((x) == MTK_HSDMA_VDESC_FINISHED)
+
+/**
+ * struct mtk_hsdma_pdesc - This is the struct holding info describing physical
+ *			    descriptor (PD) and its placement must be kept at
+ *			    4-bytes alignment in little endian order.
+ * @desc[1-4]:		    The control pad used to indicate hardware how to
+ *			    deal with the descriptor such as source and
+ *			    destination address and data length. The maximum
+ *			    data length each pdesc can handle is 0x3f80 bytes
+ */
+struct mtk_hsdma_pdesc {
+	__le32 desc1;
+	__le32 desc2;
+	__le32 desc3;
+	__le32 desc4;
+} __packed __aligned(4);
+
+/**
+ * struct mtk_hsdma_vdesc - This is the struct holding info describing virtual
+ *			    descriptor (VD)
+ * @vd:			    An instance for struct virt_dma_desc
+ * @len:		    The total data size device wants to move
+ * @residue:		    The remaining data size device will move
+ * @dest:		    The destination address device wants to move to
+ * @src:		    The source address device wants to move from
+ */
+struct mtk_hsdma_vdesc {
+	struct virt_dma_desc vd;
+	size_t len;
+	size_t residue;
+	dma_addr_t dest;
+	dma_addr_t src;
+};
+
+/**
+ * struct mtk_hsdma_cb - This is the struct holding extra info required for RX
+ *			 ring to know what relevant VD the the PD is being
+ *			 mapped to.
+ * @vd:			 Pointer to the relevant VD.
+ * @flag:		 Flag indicating what action should be taken when VD
+ *			 is completed.
+ */
+struct mtk_hsdma_cb {
+	struct virt_dma_desc *vd;
+	enum mtk_hsdma_vdesc_flag flag;
+};
+
+/**
+ * struct mtk_hsdma_ring - This struct holds info describing underlying ring
+ *			   space
+ * @txd:		   The descriptor TX ring which describes DMA source
+ *			   information
+ * @rxd:		   The descriptor RX ring which describes DMA
+ *			   destination information
+ * @cb:			   The extra information pointed at by RX ring
+ * @tphys:		   The physical addr of TX ring
+ * @rphys:		   The physical addr of RX ring
+ * @cur_tptr:		   Pointer to the next free descriptor used by the host
+ * @cur_rptr:		   Pointer to the last done descriptor by the device
+ */
+struct mtk_hsdma_ring {
+	struct mtk_hsdma_pdesc *txd;
+	struct mtk_hsdma_pdesc *rxd;
+	struct mtk_hsdma_cb *cb;
+	dma_addr_t tphys;
+	dma_addr_t rphys;
+	u16 cur_tptr;
+	u16 cur_rptr;
+};
+
+/**
+ * struct mtk_hsdma_pchan - This is the struct holding info describing physical
+ *			   channel (PC)
+ * @ring:		   An instance for the underlying ring
+ * @sz_ring:		   Total size allocated for the ring
+ * @nr_free:		   Total number of free rooms in the ring. It would
+ *			   be accessed and updated frequently between IRQ
+ *			   context and user context to reflect whether ring
+ *			   can accept requests from VD.
+ */
+struct mtk_hsdma_pchan {
+	struct mtk_hsdma_ring ring;
+	size_t sz_ring;
+	atomic_t nr_free;
+};
+
+/**
+ * struct mtk_hsdma_vchan - This is the struct holding info describing virtual
+ *			   channel (VC)
+ * @vc:			   An instance for struct virt_dma_chan
+ * @issue_completion:	   The wait for all issued descriptors completited
+ * @issue_synchronize:	   Bool indicating channel synchronization starts
+ * @desc_hw_processing:	   List those descriptors the hardware is processing
+ */
+struct mtk_hsdma_vchan {
+	struct virt_dma_chan vc;
+	struct completion issue_completion;
+	bool issue_synchronize;
+	/* protected by vc.lock */
+	struct list_head desc_hw_processing;
+};
+
+/**
+ * struct mtk_hsdma_soc - This is the struct holding differences among SoCs
+ * @ddone:		  Bit mask for DDONE
+ * @ls0:		  Bit mask for LS0
+ */
+struct mtk_hsdma_soc {
+	__le32 ddone;
+	__le32 ls0;
+};
+
+/**
+ * struct mtk_hsdma_device - This is the struct holding info describing HSDMA
+ *			     device
+ * @ddev:		     An instance for struct dma_device
+ * @base:		     The mapped register I/O base
+ * @clk:		     The clock that device internal is using
+ * @irq:		     The IRQ that device are using
+ * @dma_requests:	     The number of VCs the device supports to
+ * @vc:			     The pointer to all available VCs
+ * @pc:			     The pointer to the underlying PC
+ * @pc_refcnt:		     Track how many VCs are using the PC
+ * @lock:		     Lock protect agaisting multiple VCs access PC
+ * @soc:		     The pointer to area holding differences among
+ *			     vaious platform
+ */
+struct mtk_hsdma_device {
+	struct dma_device ddev;
+	void __iomem *base;
+	struct clk *clk;
+	u32 irq;
+
+	u32 dma_requests;
+	struct mtk_hsdma_vchan *vc;
+	struct mtk_hsdma_pchan *pc;
+	refcount_t pc_refcnt;
+
+	/* Lock used to protect against multiple VCs access PC */
+	spinlock_t lock;
+
+	const struct mtk_hsdma_soc *soc;
+};
+
+static struct mtk_hsdma_device *to_hsdma_dev(struct dma_chan *chan)
+{
+	return container_of(chan->device, struct mtk_hsdma_device,
+			    ddev);
+}
+
+static inline struct mtk_hsdma_vchan *to_hsdma_vchan(struct dma_chan *chan)
+{
+	return container_of(chan, struct mtk_hsdma_vchan, vc.chan);
+}
+
+static struct mtk_hsdma_vdesc *to_hsdma_vdesc(struct virt_dma_desc *vd)
+{
+	return container_of(vd, struct mtk_hsdma_vdesc, vd);
+}
+
+static struct device *hsdma2dev(struct mtk_hsdma_device *hsdma)
+{
+	return hsdma->ddev.dev;
+}
+
+static u32 mtk_dma_read(struct mtk_hsdma_device *hsdma, u32 reg)
+{
+	return readl(hsdma->base + reg);
+}
+
+static void mtk_dma_write(struct mtk_hsdma_device *hsdma, u32 reg, u32 val)
+{
+	writel(val, hsdma->base + reg);
+}
+
+static void mtk_dma_rmw(struct mtk_hsdma_device *hsdma, u32 reg,
+			u32 mask, u32 set)
+{
+	u32 val;
+
+	val = mtk_dma_read(hsdma, reg);
+	val &= ~mask;
+	val |= set;
+	mtk_dma_write(hsdma, reg, val);
+}
+
+static void mtk_dma_set(struct mtk_hsdma_device *hsdma, u32 reg, u32 val)
+{
+	mtk_dma_rmw(hsdma, reg, 0, val);
+}
+
+static void mtk_dma_clr(struct mtk_hsdma_device *hsdma, u32 reg, u32 val)
+{
+	mtk_dma_rmw(hsdma, reg, val, 0);
+}
+
+static void mtk_hsdma_vdesc_free(struct virt_dma_desc *vd)
+{
+	kfree(container_of(vd, struct mtk_hsdma_vdesc, vd));
+}
+
+static int mtk_hsdma_busy_wait(struct mtk_hsdma_device *hsdma)
+{
+	u32 status = 0;
+
+	return readl_poll_timeout(hsdma->base + MTK_HSDMA_GLO, status,
+				  !(status & MTK_HSDMA_GLO_BUSY),
+				  MTK_HSDMA_USEC_POLL,
+				  MTK_HSDMA_TIMEOUT_POLL);
+}
+
+static int mtk_hsdma_alloc_pchan(struct mtk_hsdma_device *hsdma,
+				 struct mtk_hsdma_pchan *pc)
+{
+	struct mtk_hsdma_ring *ring = &pc->ring;
+	int err;
+
+	memset(pc, 0, sizeof(*pc));
+
+	/*
+	 * Allocate ring space where [0 ... MTK_DMA_SIZE - 1] is for TX ring
+	 * and [MTK_DMA_SIZE ... 2 * MTK_DMA_SIZE - 1] is for RX ring.
+	 */
+	pc->sz_ring = 2 * MTK_DMA_SIZE * sizeof(*ring->txd);
+	ring->txd = dma_zalloc_coherent(hsdma2dev(hsdma), pc->sz_ring,
+					&ring->tphys, GFP_ATOMIC);
+	if (!ring->txd)
+		return -ENOMEM;
+
+	ring->rxd = &ring->txd[MTK_DMA_SIZE];
+	ring->rphys = ring->tphys + MTK_DMA_SIZE * sizeof(*ring->txd);
+	ring->cur_tptr = 0;
+	ring->cur_rptr = MTK_DMA_SIZE - 1;
+
+	ring->cb = kcalloc(MTK_DMA_SIZE, sizeof(*ring->cb), GFP_KERNEL);
+	if (!ring->cb) {
+		err = -ENOMEM;
+		goto err_free_dma;
+	}
+
+	atomic_set(&pc->nr_free, MTK_DMA_SIZE - 1);
+
+	/* Disable HSDMA and wait for the completion */
+	mtk_dma_clr(hsdma, MTK_HSDMA_GLO, MTK_HSDMA_GLO_DMA);
+	err = mtk_hsdma_busy_wait(hsdma);
+	if (err)
+		goto err_free_cb;
+
+	/* Reset */
+	mtk_dma_set(hsdma, MTK_HSDMA_RESET,
+		    MTK_HSDMA_RST_TX | MTK_HSDMA_RST_RX);
+	mtk_dma_clr(hsdma, MTK_HSDMA_RESET,
+		    MTK_HSDMA_RST_TX | MTK_HSDMA_RST_RX);
+
+	/* Setup HSDMA initial pointer in the ring */
+	mtk_dma_write(hsdma, MTK_HSDMA_TX_BASE, ring->tphys);
+	mtk_dma_write(hsdma, MTK_HSDMA_TX_CNT, MTK_DMA_SIZE);
+	mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr);
+	mtk_dma_write(hsdma, MTK_HSDMA_TX_DMA, 0);
+	mtk_dma_write(hsdma, MTK_HSDMA_RX_BASE, ring->rphys);
+	mtk_dma_write(hsdma, MTK_HSDMA_RX_CNT, MTK_DMA_SIZE);
+	mtk_dma_write(hsdma, MTK_HSDMA_RX_CPU, ring->cur_rptr);
+	mtk_dma_write(hsdma, MTK_HSDMA_RX_DMA, 0);
+
+	/* Enable HSDMA */
+	mtk_dma_set(hsdma, MTK_HSDMA_GLO, MTK_HSDMA_GLO_DMA);
+
+	/* Setup delayed interrupt */
+	mtk_dma_write(hsdma, MTK_HSDMA_DLYINT, MTK_HSDMA_DLYINT_DEFAULT);
+
+	/* Enable interrupt */
+	mtk_dma_set(hsdma, MTK_HSDMA_INT_ENABLE, MTK_HSDMA_INT_RXDONE);
+
+	return 0;
+
+err_free_cb:
+	kfree(ring->cb);
+
+err_free_dma:
+	dma_free_coherent(hsdma2dev(hsdma),
+			  pc->sz_ring, ring->txd, ring->tphys);
+	return err;
+}
+
+static void mtk_hsdma_free_pchan(struct mtk_hsdma_device *hsdma,
+				 struct mtk_hsdma_pchan *pc)
+{
+	struct mtk_hsdma_ring *ring = &pc->ring;
+
+	/* Disable HSDMA and then wait for the completion */
+	mtk_dma_clr(hsdma, MTK_HSDMA_GLO, MTK_HSDMA_GLO_DMA);
+	mtk_hsdma_busy_wait(hsdma);
+
+	/* Reset pointer in the ring */
+	mtk_dma_clr(hsdma, MTK_HSDMA_INT_ENABLE, MTK_HSDMA_INT_RXDONE);
+	mtk_dma_write(hsdma, MTK_HSDMA_TX_BASE, 0);
+	mtk_dma_write(hsdma, MTK_HSDMA_TX_CNT, 0);
+	mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, 0);
+	mtk_dma_write(hsdma, MTK_HSDMA_RX_BASE, 0);
+	mtk_dma_write(hsdma, MTK_HSDMA_RX_CNT, 0);
+	mtk_dma_write(hsdma, MTK_HSDMA_RX_CPU, MTK_DMA_SIZE - 1);
+
+	kfree(ring->cb);
+
+	dma_free_coherent(hsdma2dev(hsdma),
+			  pc->sz_ring, ring->txd, ring->tphys);
+}
+
+static int mtk_hsdma_issue_pending_vdesc(struct mtk_hsdma_device *hsdma,
+					 struct mtk_hsdma_pchan *pc,
+					 struct mtk_hsdma_vdesc *hvd)
+{
+	struct mtk_hsdma_ring *ring = &pc->ring;
+	struct mtk_hsdma_pdesc *txd, *rxd;
+	u16 reserved, prev, tlen, num_sgs;
+	unsigned long flags;
+
+	/* Protect against PC is accessed by multiple VCs simultaneously */
+	spin_lock_irqsave(&hsdma->lock, flags);
+
+	/*
+	 * Reserve rooms, where pc->nr_free is used to track how many free
+	 * rooms in the ring being updated in user and IRQ context.
+	 */
+	num_sgs = DIV_ROUND_UP(hvd->len, MTK_HSDMA_MAX_LEN);
+	reserved = min_t(u16, num_sgs, atomic_read(&pc->nr_free));
+
+	if (!reserved) {
+		spin_unlock_irqrestore(&hsdma->lock, flags);
+		return -ENOSPC;
+	}
+
+	atomic_sub(reserved, &pc->nr_free);
+
+	while (reserved--) {
+		/* Limit size by PD capability for valid data moving */
+		tlen = (hvd->len > MTK_HSDMA_MAX_LEN) ?
+		       MTK_HSDMA_MAX_LEN : hvd->len;
+
+		/*
+		 * Setup PDs using the remaining VD info mapped on those
+		 * reserved rooms. And since RXD is shared memory between the
+		 * host and the device allocated by dma_alloc_coherent call,
+		 * the helper macro WRITE_ONCE can ensure the data written to
+		 * RAM would really happens.
+		 */
+		txd = &ring->txd[ring->cur_tptr];
+		WRITE_ONCE(txd->desc1, hvd->src);
+		WRITE_ONCE(txd->desc2,
+			   hsdma->soc->ls0 | MTK_HSDMA_DESC_PLEN(tlen));
+
+		rxd = &ring->rxd[ring->cur_tptr];
+		WRITE_ONCE(rxd->desc1, hvd->dest);
+		WRITE_ONCE(rxd->desc2, MTK_HSDMA_DESC_PLEN(tlen));
+
+		/* Associate VD, the PD belonged to */
+		ring->cb[ring->cur_tptr].vd = &hvd->vd;
+
+		/* Move forward the pointer of TX ring */
+		ring->cur_tptr = MTK_HSDMA_NEXT_DESP_IDX(ring->cur_tptr,
+							 MTK_DMA_SIZE);
+
+		/* Update VD with remaining data */
+		hvd->src  += tlen;
+		hvd->dest += tlen;
+		hvd->len  -= tlen;
+	}
+
+	/*
+	 * Tagging flag for the last PD for VD will be responsible for
+	 * completing VD.
+	 */
+	if (!hvd->len) {
+		prev = MTK_HSDMA_LAST_DESP_IDX(ring->cur_tptr, MTK_DMA_SIZE);
+		ring->cb[prev].flag = MTK_HSDMA_VDESC_FINISHED;
+	}
+
+	/* Ensure all changes indeed done before we're going on */
+	wmb();
+
+	/*
+	 * Updating into hardware the pointer of TX ring lets HSDMA to take
+	 * action for those pending PDs.
+	 */
+	mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr);
+
+	spin_unlock_irqrestore(&hsdma->lock, flags);
+
+	return !hvd->len ? 0 : -ENOSPC;
+}
+
+static void mtk_hsdma_issue_vchan_pending(struct mtk_hsdma_device *hsdma,
+					  struct mtk_hsdma_vchan *hvc)
+{
+	struct virt_dma_desc *vd, *vd2;
+	int err;
+
+	lockdep_assert_held(&hvc->vc.lock);
+
+	list_for_each_entry_safe(vd, vd2, &hvc->vc.desc_issued, node) {
+		struct mtk_hsdma_vdesc *hvd;
+
+		hvd = to_hsdma_vdesc(vd);
+
+		/* Map VD into PC and all VCs shares a single PC */
+		err = mtk_hsdma_issue_pending_vdesc(hsdma, hsdma->pc, hvd);
+
+		/*
+		 * Move VD from desc_issued to desc_hw_processing when entire
+		 * VD is fit into available PDs. Otherwise, the uncompleted
+		 * VDs would stay in list desc_issued and then restart the
+		 * processing as soon as possible once underlying ring space
+		 * got freed.
+		 */
+		if (err == -ENOSPC)
+			break;
+
+		/*
+		 * The extra list desc_hw_processing is used because
+		 * hardware can't provide sufficient information allowing us
+		 * to know what VDs are still working on the underlying ring.
+		 * Through the additional list, it can help us to implement
+		 * terminate_all, residue calculation and such thing needed
+		 * to know detail descriptor status on the hardware.
+		 */
+		list_move_tail(&vd->node, &hvc->desc_hw_processing);
+	}
+}
+
+static void mtk_hsdma_free_rooms_in_ring(struct mtk_hsdma_device *hsdma)
+{
+	struct mtk_hsdma_vchan *hvc;
+	struct mtk_hsdma_pdesc *rxd;
+	struct mtk_hsdma_vdesc *hvd;
+	struct mtk_hsdma_pchan *pc;
+	struct mtk_hsdma_cb *cb;
+	__le32 desc2;
+	u32 status;
+	u16 next;
+	int i;
+
+	pc = hsdma->pc;
+
+	/* Read IRQ status */
+	status = mtk_dma_read(hsdma, MTK_HSDMA_INT_STATUS);
+
+	/*
+	 * Ack the pending IRQ all to let hardware know software is handling
+	 * those finished physical descriptors. Otherwise, the hardware would
+	 * keep the used IRQ line in certain trigger state.
+	 */
+	mtk_dma_write(hsdma, MTK_HSDMA_INT_STATUS, status);
+
+	while (1) {
+		next = MTK_HSDMA_NEXT_DESP_IDX(pc->ring.cur_rptr,
+					       MTK_DMA_SIZE);
+		rxd = &pc->ring.rxd[next];
+
+		/*
+		 * If MTK_HSDMA_DESC_DDONE is no specified, that means data
+		 * moving for the PD is still under going.
+		 */
+		desc2 = READ_ONCE(rxd->desc2);
+		if (!(desc2 & hsdma->soc->ddone))
+			break;
+
+		cb = &pc->ring.cb[next];
+		if (unlikely(!cb->vd)) {
+			dev_err(hsdma2dev(hsdma), "cb->vd cannot be null\n");
+			break;
+		}
+
+		/* Update residue of VD the associated PD belonged to */
+		hvd = to_hsdma_vdesc(cb->vd);
+		hvd->residue -= MTK_HSDMA_DESC_PLEN_GET(rxd->desc2);
+
+		/* Complete VD until the relevant last PD is finished */
+		if (IS_MTK_HSDMA_VDESC_FINISHED(cb->flag)) {
+			hvc = to_hsdma_vchan(cb->vd->tx.chan);
+
+			spin_lock(&hvc->vc.lock);
+
+			/* Remove VD from list desc_hw_processing */
+			list_del(&cb->vd->node);
+
+			/* Add VD into list desc_completed */
+			vchan_cookie_complete(cb->vd);
+
+			if (hvc->issue_synchronize &&
+			    list_empty(&hvc->desc_hw_processing)) {
+				complete(&hvc->issue_completion);
+				hvc->issue_synchronize = false;
+			}
+			spin_unlock(&hvc->vc.lock);
+
+			cb->flag = 0;
+		}
+
+		cb->vd = 0;
+
+		/*
+		 * Recycle the RXD with the helper WRITE_ONCE that can ensure
+		 * data written into RAM would really happens.
+		 */
+		WRITE_ONCE(rxd->desc1, 0);
+		WRITE_ONCE(rxd->desc2, 0);
+		pc->ring.cur_rptr = next;
+
+		/* Release rooms */
+		atomic_inc(&pc->nr_free);
+	}
+
+	/* Ensure all changes indeed done before we're going on */
+	wmb();
+
+	/* Update CPU pointer for those completed PDs */
+	mtk_dma_write(hsdma, MTK_HSDMA_RX_CPU, pc->ring.cur_rptr);
+
+	/* ASAP handles pending VDs in all VCs after freeing some rooms */
+	for (i = 0; i < hsdma->dma_requests; i++) {
+		hvc = &hsdma->vc[i];
+		spin_lock(&hvc->vc.lock);
+		mtk_hsdma_issue_vchan_pending(hsdma, hvc);
+		spin_unlock(&hvc->vc.lock);
+	}
+
+	/* All completed PDs are cleaned up, so enable interrupt again */
+	mtk_dma_set(hsdma, MTK_HSDMA_INT_ENABLE, MTK_HSDMA_INT_RXDONE);
+}
+
+static irqreturn_t mtk_hsdma_irq(int irq, void *devid)
+{
+	struct mtk_hsdma_device *hsdma = devid;
+
+	/*
+	 * Disable interrupt until all completed PDs are cleaned up in
+	 * mtk_hsdma_free_rooms call.
+	 */
+	mtk_dma_clr(hsdma, MTK_HSDMA_INT_ENABLE, MTK_HSDMA_INT_RXDONE);
+
+	mtk_hsdma_free_rooms_in_ring(hsdma);
+
+	return IRQ_HANDLED;
+}
+
+static struct virt_dma_desc *mtk_hsdma_find_active_desc(struct dma_chan *c,
+							dma_cookie_t cookie)
+{
+	struct mtk_hsdma_vchan *hvc = to_hsdma_vchan(c);
+	struct virt_dma_desc *vd;
+
+	list_for_each_entry(vd, &hvc->desc_hw_processing, node)
+		if (vd->tx.cookie == cookie)
+			return vd;
+
+	list_for_each_entry(vd, &hvc->vc.desc_issued, node)
+		if (vd->tx.cookie == cookie)
+			return vd;
+
+	return NULL;
+}
+
+static enum dma_status mtk_hsdma_tx_status(struct dma_chan *c,
+					   dma_cookie_t cookie,
+					   struct dma_tx_state *txstate)
+{
+	struct mtk_hsdma_vchan *hvc = to_hsdma_vchan(c);
+	struct mtk_hsdma_vdesc *hvd;
+	struct virt_dma_desc *vd;
+	enum dma_status ret;
+	unsigned long flags;
+	size_t bytes = 0;
+
+	ret = dma_cookie_status(c, cookie, txstate);
+	if (ret == DMA_COMPLETE || !txstate)
+		return ret;
+
+	spin_lock_irqsave(&hvc->vc.lock, flags);
+	vd = mtk_hsdma_find_active_desc(c, cookie);
+	spin_unlock_irqrestore(&hvc->vc.lock, flags);
+
+	if (vd) {
+		hvd = to_hsdma_vdesc(vd);
+		bytes = hvd->residue;
+	}
+
+	dma_set_residue(txstate, bytes);
+
+	return ret;
+}
+
+static void mtk_hsdma_issue_pending(struct dma_chan *c)
+{
+	struct mtk_hsdma_device *hsdma = to_hsdma_dev(c);
+	struct mtk_hsdma_vchan *hvc = to_hsdma_vchan(c);
+	unsigned long flags;
+
+	spin_lock_irqsave(&hvc->vc.lock, flags);
+
+	if (vchan_issue_pending(&hvc->vc))
+		mtk_hsdma_issue_vchan_pending(hsdma, hvc);
+
+	spin_unlock_irqrestore(&hvc->vc.lock, flags);
+}
+
+static struct dma_async_tx_descriptor *
+mtk_hsdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest,
+			  dma_addr_t src, size_t len, unsigned long flags)
+{
+	struct mtk_hsdma_vdesc *hvd;
+
+	hvd = kzalloc(sizeof(*hvd), GFP_NOWAIT);
+	if (!hvd)
+		return NULL;
+
+	hvd->len = len;
+	hvd->residue = len;
+	hvd->src = src;
+	hvd->dest = dest;
+
+	return vchan_tx_prep(to_virt_chan(c), &hvd->vd, flags);
+}
+
+static int mtk_hsdma_free_inactive_desc(struct dma_chan *c, bool reuse_clr)
+{
+	struct virt_dma_chan *vc = to_virt_chan(c);
+	unsigned long flags;
+	LIST_HEAD(head);
+
+	/*
+	 * Terminate pending descriptors that have previously been submitted
+	 * to the channel. However, the DMA engine doesn't provide any way to
+	 * stop these descriptors being processed by hardware. The only way is
+	 * just waiting until these descriptors are all processing done. Thus,
+	 * users must synchronize to the DMA channel termination to guarantee
+	 * that all transfers for previously issued descriptors have stopped.
+	 */
+	spin_lock_irqsave(&vc->lock, flags);
+	list_splice_tail_init(&vc->desc_allocated, &head);
+	list_splice_tail_init(&vc->desc_submitted, &head);
+	list_splice_tail_init(&vc->desc_issued, &head);
+
+	if (reuse_clr) {
+		struct virt_dma_desc *vd;
+
+		list_for_each_entry(vd, &head, node)
+			dmaengine_desc_clear_reuse(&vd->tx);
+	}
+	spin_unlock_irqrestore(&vc->lock, flags);
+
+	/* At the point, we don't expect users put descriptor into VC again */
+	vchan_dma_desc_free_list(vc, &head);
+
+	return 0;
+}
+
+static int mtk_hsdma_terminate_all(struct dma_chan *c)
+{
+	mtk_hsdma_free_inactive_desc(c, false);
+
+	return 0;
+}
+
+static void mtk_hsdma_synchronize(struct dma_chan *c)
+{
+	struct mtk_hsdma_vchan *hvc = to_hsdma_vchan(c);
+	bool sync_needed = false;
+
+	/*
+	 * Once issue_synchronize is being set, which means once the hardware
+	 * consumes all descriptors for the channel in the ring, the
+	 * synchronization must be be notified immediately it is completed.
+	 */
+	spin_lock(&hvc->vc.lock);
+	if (!list_empty(&hvc->desc_hw_processing)) {
+		hvc->issue_synchronize = true;
+		sync_needed = true;
+	}
+	spin_unlock(&hvc->vc.lock);
+
+	if (sync_needed)
+		wait_for_completion(&hvc->issue_completion);
+	/*
+	 * At the point, we expect that all remaining descriptors in the ring
+	 * for the channel should be all processing done.
+	 */
+	WARN_ONCE(!list_empty(&hvc->desc_hw_processing),
+		  "Desc pending still in list desc_hw_processing\n");
+
+	/* Free all descriptors in list desc_completed */
+	vchan_synchronize(&hvc->vc);
+
+	WARN_ONCE(!list_empty(&hvc->vc.desc_completed),
+		  "Desc pending still in list desc_completed\n");
+}
+
+static int mtk_hsdma_alloc_chan_resources(struct dma_chan *c)
+{
+	struct mtk_hsdma_device *hsdma = to_hsdma_dev(c);
+	int err;
+
+	/*
+	 * Since HSDMA has only one PC, the resource for PC is being allocated
+	 * when the first VC is being created and the other VCs would run on
+	 * the same PC.
+	 */
+	if (!refcount_read(&hsdma->pc_refcnt)) {
+		err = mtk_hsdma_alloc_pchan(hsdma, hsdma->pc);
+		if (err)
+			return err;
+		/*
+		 * refcount_inc would complain increment on 0; use-after-free.
+		 * Thus, we need to explicitly set it as 1 initially.
+		 */
+		refcount_set(&hsdma->pc_refcnt, 1);
+	} else {
+		refcount_inc(&hsdma->pc_refcnt);
+	}
+
+	return 0;
+}
+
+static void mtk_hsdma_free_chan_resources(struct dma_chan *c)
+{
+	struct mtk_hsdma_device *hsdma = to_hsdma_dev(c);
+
+	/* The resource for PC is not freed until all the VCs are destroyed */
+	if (!refcount_dec_and_test(&hsdma->pc_refcnt))
+		return;
+
+	mtk_hsdma_free_inactive_desc(c, true);
+	mtk_hsdma_synchronize(c);
+
+	mtk_hsdma_free_pchan(hsdma, hsdma->pc);
+}
+
+static int mtk_hsdma_hw_init(struct mtk_hsdma_device *hsdma)
+{
+	int err;
+
+	pm_runtime_enable(hsdma2dev(hsdma));
+	pm_runtime_get_sync(hsdma2dev(hsdma));
+
+	err = clk_prepare_enable(hsdma->clk);
+	if (err)
+		return err;
+
+	mtk_dma_write(hsdma, MTK_HSDMA_INT_ENABLE, 0);
+	mtk_dma_write(hsdma, MTK_HSDMA_GLO, MTK_HSDMA_GLO_DEFAULT);
+
+	return 0;
+}
+
+static int mtk_hsdma_hw_deinit(struct mtk_hsdma_device *hsdma)
+{
+	mtk_dma_write(hsdma, MTK_HSDMA_GLO, 0);
+
+	clk_disable_unprepare(hsdma->clk);
+
+	pm_runtime_put_sync(hsdma2dev(hsdma));
+	pm_runtime_disable(hsdma2dev(hsdma));
+
+	return 0;
+}
+
+static const struct mtk_hsdma_soc mt7623_soc = {
+	.ddone = BIT(31),
+	.ls0 = BIT(30),
+};
+
+static const struct mtk_hsdma_soc mt7622_soc = {
+	.ddone = BIT(15),
+	.ls0 = BIT(14),
+};
+
+static const struct of_device_id mtk_hsdma_match[] = {
+	{ .compatible = "mediatek,mt7623-hsdma", .data = &mt7623_soc},
+	{ .compatible = "mediatek,mt7622-hsdma", .data = &mt7622_soc},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mtk_hsdma_match);
+
+static int mtk_hsdma_probe(struct platform_device *pdev)
+{
+	struct mtk_hsdma_device *hsdma;
+	struct mtk_hsdma_vchan *vc;
+	struct dma_device *dd;
+	struct resource *res;
+	int i, err;
+
+	hsdma = devm_kzalloc(&pdev->dev, sizeof(*hsdma), GFP_KERNEL);
+	if (!hsdma)
+		return -ENOMEM;
+
+	dd = &hsdma->ddev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hsdma->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(hsdma->base))
+		return PTR_ERR(hsdma->base);
+
+	hsdma->soc = of_device_get_match_data(&pdev->dev);
+	if (!hsdma->soc) {
+		dev_err(&pdev->dev, "No device match found\n");
+		return -ENODEV;
+	}
+
+	hsdma->clk = devm_clk_get(&pdev->dev, "hsdma");
+	if (IS_ERR(hsdma->clk)) {
+		dev_err(&pdev->dev, "No clock for %s\n",
+			dev_name(&pdev->dev));
+		return PTR_ERR(hsdma->clk);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "No irq resource for %s\n",
+			dev_name(&pdev->dev));
+		return -EINVAL;
+	}
+	hsdma->irq = res->start;
+
+	refcount_set(&hsdma->pc_refcnt, 0);
+	spin_lock_init(&hsdma->lock);
+
+	dma_cap_set(DMA_MEMCPY, dd->cap_mask);
+
+	dd->copy_align = MTK_HSDMA_ALIGN_SIZE;
+	dd->device_alloc_chan_resources = mtk_hsdma_alloc_chan_resources;
+	dd->device_free_chan_resources = mtk_hsdma_free_chan_resources;
+	dd->device_tx_status = mtk_hsdma_tx_status;
+	dd->device_issue_pending = mtk_hsdma_issue_pending;
+	dd->device_prep_dma_memcpy = mtk_hsdma_prep_dma_memcpy;
+	dd->device_terminate_all = mtk_hsdma_terminate_all;
+	dd->device_synchronize = mtk_hsdma_synchronize;
+	dd->src_addr_widths = MTK_HSDMA_DMA_BUSWIDTHS;
+	dd->dst_addr_widths = MTK_HSDMA_DMA_BUSWIDTHS;
+	dd->directions = BIT(DMA_MEM_TO_MEM);
+	dd->residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+	dd->dev = &pdev->dev;
+	INIT_LIST_HEAD(&dd->channels);
+
+	hsdma->dma_requests = MTK_HSDMA_NR_VCHANS;
+	if (pdev->dev.of_node && of_property_read_u32(pdev->dev.of_node,
+						      "dma-requests",
+						      &hsdma->dma_requests)) {
+		dev_info(&pdev->dev,
+			 "Using %u as missing dma-requests property\n",
+			 MTK_HSDMA_NR_VCHANS);
+	}
+
+	hsdma->pc = devm_kcalloc(&pdev->dev, MTK_HSDMA_NR_MAX_PCHANS,
+				 sizeof(*hsdma->pc), GFP_KERNEL);
+	if (!hsdma->pc)
+		return -ENOMEM;
+
+	hsdma->vc = devm_kcalloc(&pdev->dev, hsdma->dma_requests,
+				 sizeof(*hsdma->vc), GFP_KERNEL);
+	if (!hsdma->vc)
+		return -ENOMEM;
+
+	for (i = 0; i < hsdma->dma_requests; i++) {
+		vc = &hsdma->vc[i];
+		vc->vc.desc_free = mtk_hsdma_vdesc_free;
+		vchan_init(&vc->vc, dd);
+		init_completion(&vc->issue_completion);
+		INIT_LIST_HEAD(&vc->desc_hw_processing);
+	}
+
+	err = dma_async_device_register(dd);
+	if (err)
+		return err;
+
+	err = of_dma_controller_register(pdev->dev.of_node,
+					 of_dma_xlate_by_chan_id, hsdma);
+	if (err) {
+		dev_err(&pdev->dev,
+			"MediaTek HSDMA OF registration failed %d\n", err);
+		goto err_unregister;
+	}
+
+	mtk_hsdma_hw_init(hsdma);
+
+	err = devm_request_irq(&pdev->dev, hsdma->irq,
+			       mtk_hsdma_irq, 0,
+			       dev_name(&pdev->dev), hsdma);
+	if (err) {
+		dev_err(&pdev->dev,
+			"request_irq failed with err %d\n", err);
+		goto err_unregister;
+	}
+
+	platform_set_drvdata(pdev, hsdma);
+
+	dev_info(&pdev->dev, "MediaTek HSDMA driver registered\n");
+
+	return 0;
+
+err_unregister:
+	dma_async_device_unregister(dd);
+
+	return err;
+}
+
+static int mtk_hsdma_remove(struct platform_device *pdev)
+{
+	struct mtk_hsdma_device *hsdma = platform_get_drvdata(pdev);
+	struct mtk_hsdma_vchan *vc;
+	int i;
+
+	/* Kill VC task */
+	for (i = 0; i < hsdma->dma_requests; i++) {
+		vc = &hsdma->vc[i];
+
+		list_del(&vc->vc.chan.device_node);
+		tasklet_kill(&vc->vc.task);
+	}
+
+	/* Disable DMA interrupt */
+	mtk_dma_write(hsdma, MTK_HSDMA_INT_ENABLE, 0);
+
+	/* Waits for any pending IRQ handlers to complete */
+	synchronize_irq(hsdma->irq);
+
+	/* Disable hardware */
+	mtk_hsdma_hw_deinit(hsdma);
+
+	dma_async_device_unregister(&hsdma->ddev);
+	of_dma_controller_free(pdev->dev.of_node);
+
+	return 0;
+}
+
+static struct platform_driver mtk_hsdma_driver = {
+	.probe		= mtk_hsdma_probe,
+	.remove		= mtk_hsdma_remove,
+	.driver = {
+		.name		= MTK_DMA_DEV,
+		.of_match_table	= mtk_hsdma_match,
+	},
+};
+module_platform_driver(mtk_hsdma_driver);
+
+MODULE_DESCRIPTION("MediaTek High-Speed DMA Controller Driver");
+MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH v5 3/3] dmaengine: mediatek: update MAINTAINERS entry with MediaTek DMA driver

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

I work for MediaTek and maintain SoC targeting to home gateway and
also will keep extending and testing the function.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3bdc260..d3c33d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8785,6 +8785,15 @@ M:	Sean Wang <sean.wang@mediatek.com>
 S:	Maintained
 F:	drivers/media/rc/mtk-cir.c
 
+MEDIATEK DMA DRIVER
+M:	Sean Wang <sean.wang@mediatek.com>
+L:	dmaengine@vger.kernel.org
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/dma/mtk-*
+F:	drivers/dma/mediatek/
+
 MEDIATEK PMIC LED DRIVER
 M:	Sean Wang <sean.wang@mediatek.com>
 S:	Maintained
-- 
2.7.4

Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add MediaTek High-Speed DMA controller bindings

[Rob Herring]

On Sun, Feb 18, 2018 at 03:08:29AM +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Document the devicetree bindings for MediaTek High-Speed DMA controller
> which could be found on MT7623 SoC or other similar Mediatek SoCs.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../devicetree/bindings/dma/mtk-hsdma.txt          | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/mtk-hsdma.txt

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC

[Vinod Koul]

On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang@mediatek.com wrote:

> @@ -0,0 +1,1054 @@
> +// SPDX-License-Identifier: GPL-2.0
// Copyright ...

The copyright line needs to follow SPDX tag line

> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/jiffies.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/refcount.h>
> +#include <linux/slab.h>

that's a lot of headers, do u need all those?

> +
> +#include "../virt-dma.h"
> +
> +#define MTK_DMA_DEV KBUILD_MODNAME

why do you need this?

> +
> +#define MTK_HSDMA_USEC_POLL		20
> +#define MTK_HSDMA_TIMEOUT_POLL		200000
> +#define MTK_HSDMA_DMA_BUSWIDTHS		BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED)

Undefined buswidth??

> +/**
> + * struct mtk_hsdma_pdesc - This is the struct holding info describing physical
> + *			    descriptor (PD) and its placement must be kept at
> + *			    4-bytes alignment in little endian order.
> + * @desc[1-4]:		    The control pad used to indicate hardware how to

pls align to 80char or lesser

> +/**
> + * struct mtk_hsdma_ring - This struct holds info describing underlying ring
> + *			   space
> + * @txd:		   The descriptor TX ring which describes DMA source
> + *			   information
> + * @rxd:		   The descriptor RX ring which describes DMA
> + *			   destination information
> + * @cb:			   The extra information pointed at by RX ring
> + * @tphys:		   The physical addr of TX ring
> + * @rphys:		   The physical addr of RX ring
> + * @cur_tptr:		   Pointer to the next free descriptor used by the host
> + * @cur_rptr:		   Pointer to the last done descriptor by the device

here alignment and 80 char wrap will help too and few other places...


> +struct mtk_hsdma_vchan {
> +	struct virt_dma_chan vc;
> +	struct completion issue_completion;
> +	bool issue_synchronize;
> +	/* protected by vc.lock */

this should be at comments above...

> +static struct mtk_hsdma_device *to_hsdma_dev(struct dma_chan *chan)
> +{
> +	return container_of(chan->device, struct mtk_hsdma_device,
> +			    ddev);

and this can fit in a line

> +static int mtk_hsdma_alloc_pchan(struct mtk_hsdma_device *hsdma,
> +				 struct mtk_hsdma_pchan *pc)
> +{
> +	struct mtk_hsdma_ring *ring = &pc->ring;
> +	int err;
> +
> +	memset(pc, 0, sizeof(*pc));
> +
> +	/*
> +	 * Allocate ring space where [0 ... MTK_DMA_SIZE - 1] is for TX ring
> +	 * and [MTK_DMA_SIZE ... 2 * MTK_DMA_SIZE - 1] is for RX ring.
> +	 */
> +	pc->sz_ring = 2 * MTK_DMA_SIZE * sizeof(*ring->txd);
> +	ring->txd = dma_zalloc_coherent(hsdma2dev(hsdma), pc->sz_ring,
> +					&ring->tphys, GFP_ATOMIC);

GFP_NOWAIT please

> +	if (!ring->txd)
> +		return -ENOMEM;
> +
> +	ring->rxd = &ring->txd[MTK_DMA_SIZE];
> +	ring->rphys = ring->tphys + MTK_DMA_SIZE * sizeof(*ring->txd);
> +	ring->cur_tptr = 0;
> +	ring->cur_rptr = MTK_DMA_SIZE - 1;
> +
> +	ring->cb = kcalloc(MTK_DMA_SIZE, sizeof(*ring->cb), GFP_KERNEL);

this is inconsistent with your own pattern! make it GFP_NOWAIT pls

> +static int mtk_hsdma_issue_pending_vdesc(struct mtk_hsdma_device *hsdma,
> +					 struct mtk_hsdma_pchan *pc,
> +					 struct mtk_hsdma_vdesc *hvd)
> +{
> +	struct mtk_hsdma_ring *ring = &pc->ring;
> +	struct mtk_hsdma_pdesc *txd, *rxd;
> +	u16 reserved, prev, tlen, num_sgs;
> +	unsigned long flags;
> +
> +	/* Protect against PC is accessed by multiple VCs simultaneously */
> +	spin_lock_irqsave(&hsdma->lock, flags);
> +
> +	/*
> +	 * Reserve rooms, where pc->nr_free is used to track how many free
> +	 * rooms in the ring being updated in user and IRQ context.
> +	 */
> +	num_sgs = DIV_ROUND_UP(hvd->len, MTK_HSDMA_MAX_LEN);
> +	reserved = min_t(u16, num_sgs, atomic_read(&pc->nr_free));
> +
> +	if (!reserved) {
> +		spin_unlock_irqrestore(&hsdma->lock, flags);
> +		return -ENOSPC;
> +	}
> +
> +	atomic_sub(reserved, &pc->nr_free);
> +
> +	while (reserved--) {
> +		/* Limit size by PD capability for valid data moving */
> +		tlen = (hvd->len > MTK_HSDMA_MAX_LEN) ?
> +		       MTK_HSDMA_MAX_LEN : hvd->len;
> +
> +		/*
> +		 * Setup PDs using the remaining VD info mapped on those
> +		 * reserved rooms. And since RXD is shared memory between the
> +		 * host and the device allocated by dma_alloc_coherent call,
> +		 * the helper macro WRITE_ONCE can ensure the data written to
> +		 * RAM would really happens.
> +		 */
> +		txd = &ring->txd[ring->cur_tptr];
> +		WRITE_ONCE(txd->desc1, hvd->src);
> +		WRITE_ONCE(txd->desc2,
> +			   hsdma->soc->ls0 | MTK_HSDMA_DESC_PLEN(tlen));
> +
> +		rxd = &ring->rxd[ring->cur_tptr];
> +		WRITE_ONCE(rxd->desc1, hvd->dest);
> +		WRITE_ONCE(rxd->desc2, MTK_HSDMA_DESC_PLEN(tlen));
> +
> +		/* Associate VD, the PD belonged to */
> +		ring->cb[ring->cur_tptr].vd = &hvd->vd;
> +
> +		/* Move forward the pointer of TX ring */
> +		ring->cur_tptr = MTK_HSDMA_NEXT_DESP_IDX(ring->cur_tptr,
> +							 MTK_DMA_SIZE);
> +
> +		/* Update VD with remaining data */
> +		hvd->src  += tlen;
> +		hvd->dest += tlen;
> +		hvd->len  -= tlen;
> +	}
> +
> +	/*
> +	 * Tagging flag for the last PD for VD will be responsible for
> +	 * completing VD.
> +	 */
> +	if (!hvd->len) {
> +		prev = MTK_HSDMA_LAST_DESP_IDX(ring->cur_tptr, MTK_DMA_SIZE);
> +		ring->cb[prev].flag = MTK_HSDMA_VDESC_FINISHED;
> +	}
> +
> +	/* Ensure all changes indeed done before we're going on */
> +	wmb();
> +
> +	/*
> +	 * Updating into hardware the pointer of TX ring lets HSDMA to take
> +	 * action for those pending PDs.
> +	 */
> +	mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr);
> +
> +	spin_unlock_irqrestore(&hsdma->lock, flags);
> +
> +	return !hvd->len ? 0 : -ENOSPC;

you already wrote and started txn, so why this?

> +static void mtk_hsdma_free_rooms_in_ring(struct mtk_hsdma_device *hsdma)
> +{
> +	struct mtk_hsdma_vchan *hvc;
> +	struct mtk_hsdma_pdesc *rxd;
> +	struct mtk_hsdma_vdesc *hvd;
> +	struct mtk_hsdma_pchan *pc;
> +	struct mtk_hsdma_cb *cb;
> +	__le32 desc2;
> +	u32 status;
> +	u16 next;
> +	int i;
> +
> +	pc = hsdma->pc;
> +
> +	/* Read IRQ status */
> +	status = mtk_dma_read(hsdma, MTK_HSDMA_INT_STATUS);
> +
> +	/*
> +	 * Ack the pending IRQ all to let hardware know software is handling
> +	 * those finished physical descriptors. Otherwise, the hardware would
> +	 * keep the used IRQ line in certain trigger state.
> +	 */
> +	mtk_dma_write(hsdma, MTK_HSDMA_INT_STATUS, status);
> +
> +	while (1) {
> +		next = MTK_HSDMA_NEXT_DESP_IDX(pc->ring.cur_rptr,
> +					       MTK_DMA_SIZE);

shouldn't we check if next is in range, we can crash if we get bad value
from hardware..

> +		rxd = &pc->ring.rxd[next];
> +
> +		/*
> +		 * If MTK_HSDMA_DESC_DDONE is no specified, that means data
> +		 * moving for the PD is still under going.
> +		 */
> +		desc2 = READ_ONCE(rxd->desc2);
> +		if (!(desc2 & hsdma->soc->ddone))
> +			break;

okay this is one break

> +
> +		cb = &pc->ring.cb[next];
> +		if (unlikely(!cb->vd)) {
> +			dev_err(hsdma2dev(hsdma), "cb->vd cannot be null\n");
> +			break;

and other for null, i feel we need to have checks for while(1) to break,
this can run infinitely if something bad happens, a fail safe would be good,
that too this being invoked from isr.

> +static enum dma_status mtk_hsdma_tx_status(struct dma_chan *c,
> +					   dma_cookie_t cookie,
> +					   struct dma_tx_state *txstate)
> +{
> +	struct mtk_hsdma_vchan *hvc = to_hsdma_vchan(c);
> +	struct mtk_hsdma_vdesc *hvd;
> +	struct virt_dma_desc *vd;
> +	enum dma_status ret;
> +	unsigned long flags;
> +	size_t bytes = 0;
> +
> +	ret = dma_cookie_status(c, cookie, txstate);
> +	if (ret == DMA_COMPLETE || !txstate)
> +		return ret;
> +
> +	spin_lock_irqsave(&hvc->vc.lock, flags);
> +	vd = mtk_hsdma_find_active_desc(c, cookie);
> +	spin_unlock_irqrestore(&hvc->vc.lock, flags);
> +
> +	if (vd) {
> +		hvd = to_hsdma_vdesc(vd);
> +		bytes = hvd->residue;

for active descriptor, shouldn't you read counters from hardware?

> +static struct dma_async_tx_descriptor *
> +mtk_hsdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest,
> +			  dma_addr_t src, size_t len, unsigned long flags)
> +{
> +	struct mtk_hsdma_vdesc *hvd;
> +
> +	hvd = kzalloc(sizeof(*hvd), GFP_NOWAIT);

GFP_NOWAIT here too

> +static int mtk_hsdma_terminate_all(struct dma_chan *c)
> +{
> +	mtk_hsdma_free_inactive_desc(c, false);

only inactive, active ones need to be freed and channel cleaned

-- 
~Vinod

Re: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC

[Sean Wang]

On Thu, 2018-03-01 at 13:53 +0530, Vinod Koul wrote:
> On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang@mediatek.com wrote:
> 
> > @@ -0,0 +1,1054 @@
> > +// SPDX-License-Identifier: GPL-2.0
> // Copyright ...
> 
> The copyright line needs to follow SPDX tag line
> 

okay, I will make it reorder and be something like that

// SPDX-License-Identifier: GPL-2.0
/*
 * Copyright (c) 2017-2018 MediaTek Inc.
 * Author: Sean Wang <sean.wang@mediatek.com>
 *
 * Driver for MediaTek High-Speed DMA Controller
 *
 */


> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_dma.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/refcount.h>
> > +#include <linux/slab.h>
> 
> that's a lot of headers, do u need all those?
> 

okay, I will have more checks whether some header listed here is not
necessary

> > +
> > +#include "../virt-dma.h"
> > +
> > +#define MTK_DMA_DEV KBUILD_MODNAME
> 
> why do you need this?
> 

the point is I learned from other subsystem makes the driver name be
same with the module name with KBUILD_MODNAME.

If you really don't like it, I can just change it into 

#define MTK_DMA_DEV "mtk-hsdma"

> > +
> > +#define MTK_HSDMA_USEC_POLL		20
> > +#define MTK_HSDMA_TIMEOUT_POLL		200000
> > +#define MTK_HSDMA_DMA_BUSWIDTHS		BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED)
> 
> Undefined buswidth??
> 
> > +/**
> > + * struct mtk_hsdma_pdesc - This is the struct holding info describing physical
> > + *			    descriptor (PD) and its placement must be kept at
> > + *			    4-bytes alignment in little endian order.
> > + * @desc[1-4]:		    The control pad used to indicate hardware how to
> 
> pls align to 80char or lesser
> 

weird, it seems the line is already with 80 char and pass the
checkpatch.pl. or do I misunderstand something ?

> > +/**
> > + * struct mtk_hsdma_ring - This struct holds info describing underlying ring
> > + *			   space
> > + * @txd:		   The descriptor TX ring which describes DMA source
> > + *			   information
> > + * @rxd:		   The descriptor RX ring which describes DMA
> > + *			   destination information
> > + * @cb:			   The extra information pointed at by RX ring
> > + * @tphys:		   The physical addr of TX ring
> > + * @rphys:		   The physical addr of RX ring
> > + * @cur_tptr:		   Pointer to the next free descriptor used by the host
> > + * @cur_rptr:		   Pointer to the last done descriptor by the device
> 
> here alignment and 80 char wrap will help too and few other places...
> 

Also weird, it seems the line is already with 80 char and pass the
checkpatch.pl. or do I misunderstand something ?

> 
> > +struct mtk_hsdma_vchan {
> > +	struct virt_dma_chan vc;
> > +	struct completion issue_completion;
> > +	bool issue_synchronize;
> > +	/* protected by vc.lock */
> 
> this should be at comments above...
> 
> > +static struct mtk_hsdma_device *to_hsdma_dev(struct dma_chan *chan)
> > +{
> > +	return container_of(chan->device, struct mtk_hsdma_device,
> > +			    ddev);
> 
> and this can fit in a line
> 

Thanks, I will improve it.

> > +static int mtk_hsdma_alloc_pchan(struct mtk_hsdma_device *hsdma,
> > +				 struct mtk_hsdma_pchan *pc)
> > +{
> > +	struct mtk_hsdma_ring *ring = &pc->ring;
> > +	int err;
> > +
> > +	memset(pc, 0, sizeof(*pc));
> > +
> > +	/*
> > +	 * Allocate ring space where [0 ... MTK_DMA_SIZE - 1] is for TX ring
> > +	 * and [MTK_DMA_SIZE ... 2 * MTK_DMA_SIZE - 1] is for RX ring.
> > +	 */
> > +	pc->sz_ring = 2 * MTK_DMA_SIZE * sizeof(*ring->txd);
> > +	ring->txd = dma_zalloc_coherent(hsdma2dev(hsdma), pc->sz_ring,
> > +					&ring->tphys, GFP_ATOMIC);
> 
> GFP_NOWAIT please
> 

Thanks, I will improve it with GFP_NOWAIT.

> > +	if (!ring->txd)
> > +		return -ENOMEM;
> > +
> > +	ring->rxd = &ring->txd[MTK_DMA_SIZE];
> > +	ring->rphys = ring->tphys + MTK_DMA_SIZE * sizeof(*ring->txd);
> > +	ring->cur_tptr = 0;
> > +	ring->cur_rptr = MTK_DMA_SIZE - 1;
> > +
> > +	ring->cb = kcalloc(MTK_DMA_SIZE, sizeof(*ring->cb), GFP_KERNEL);
> 
> this is inconsistent with your own pattern! make it GFP_NOWAIT pls
> 

Ditto, I will improve it with GFP_NOWAIT.

> > +static int mtk_hsdma_issue_pending_vdesc(struct mtk_hsdma_device *hsdma,
> > +					 struct mtk_hsdma_pchan *pc,
> > +					 struct mtk_hsdma_vdesc *hvd)
> > +{
> > +	struct mtk_hsdma_ring *ring = &pc->ring;
> > +	struct mtk_hsdma_pdesc *txd, *rxd;
> > +	u16 reserved, prev, tlen, num_sgs;
> > +	unsigned long flags;
> > +
> > +	/* Protect against PC is accessed by multiple VCs simultaneously */
> > +	spin_lock_irqsave(&hsdma->lock, flags);
> > +
> > +	/*
> > +	 * Reserve rooms, where pc->nr_free is used to track how many free
> > +	 * rooms in the ring being updated in user and IRQ context.
> > +	 */
> > +	num_sgs = DIV_ROUND_UP(hvd->len, MTK_HSDMA_MAX_LEN);
> > +	reserved = min_t(u16, num_sgs, atomic_read(&pc->nr_free));
> > +
> > +	if (!reserved) {
> > +		spin_unlock_irqrestore(&hsdma->lock, flags);
> > +		return -ENOSPC;
> > +	}
> > +
> > +	atomic_sub(reserved, &pc->nr_free);
> > +
> > +	while (reserved--) {
> > +		/* Limit size by PD capability for valid data moving */
> > +		tlen = (hvd->len > MTK_HSDMA_MAX_LEN) ?
> > +		       MTK_HSDMA_MAX_LEN : hvd->len;
> > +
> > +		/*
> > +		 * Setup PDs using the remaining VD info mapped on those
> > +		 * reserved rooms. And since RXD is shared memory between the
> > +		 * host and the device allocated by dma_alloc_coherent call,
> > +		 * the helper macro WRITE_ONCE can ensure the data written to
> > +		 * RAM would really happens.
> > +		 */
> > +		txd = &ring->txd[ring->cur_tptr];
> > +		WRITE_ONCE(txd->desc1, hvd->src);
> > +		WRITE_ONCE(txd->desc2,
> > +			   hsdma->soc->ls0 | MTK_HSDMA_DESC_PLEN(tlen));
> > +
> > +		rxd = &ring->rxd[ring->cur_tptr];
> > +		WRITE_ONCE(rxd->desc1, hvd->dest);
> > +		WRITE_ONCE(rxd->desc2, MTK_HSDMA_DESC_PLEN(tlen));
> > +
> > +		/* Associate VD, the PD belonged to */
> > +		ring->cb[ring->cur_tptr].vd = &hvd->vd;
> > +
> > +		/* Move forward the pointer of TX ring */
> > +		ring->cur_tptr = MTK_HSDMA_NEXT_DESP_IDX(ring->cur_tptr,
> > +							 MTK_DMA_SIZE);
> > +
> > +		/* Update VD with remaining data */
> > +		hvd->src  += tlen;
> > +		hvd->dest += tlen;
> > +		hvd->len  -= tlen;
> > +	}
> > +
> > +	/*
> > +	 * Tagging flag for the last PD for VD will be responsible for
> > +	 * completing VD.
> > +	 */
> > +	if (!hvd->len) {
> > +		prev = MTK_HSDMA_LAST_DESP_IDX(ring->cur_tptr, MTK_DMA_SIZE);
> > +		ring->cb[prev].flag = MTK_HSDMA_VDESC_FINISHED;
> > +	}
> > +
> > +	/* Ensure all changes indeed done before we're going on */
> > +	wmb();
> > +
> > +	/*
> > +	 * Updating into hardware the pointer of TX ring lets HSDMA to take
> > +	 * action for those pending PDs.
> > +	 */
> > +	mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr);
> > +
> > +	spin_unlock_irqrestore(&hsdma->lock, flags);
> > +
> > +	return !hvd->len ? 0 : -ENOSPC;
> 
> you already wrote and started txn, so why this?
> 

it's possible just partial virtual descriptor fits into hardware and
then return -ENOSPC. And it will start it to complete the remaining part
as soon as possible when some rooms is being freed.


> > +static void mtk_hsdma_free_rooms_in_ring(struct mtk_hsdma_device *hsdma)
> > +{
> > +	struct mtk_hsdma_vchan *hvc;
> > +	struct mtk_hsdma_pdesc *rxd;
> > +	struct mtk_hsdma_vdesc *hvd;
> > +	struct mtk_hsdma_pchan *pc;
> > +	struct mtk_hsdma_cb *cb;
> > +	__le32 desc2;
> > +	u32 status;
> > +	u16 next;
> > +	int i;
> > +
> > +	pc = hsdma->pc;
> > +
> > +	/* Read IRQ status */
> > +	status = mtk_dma_read(hsdma, MTK_HSDMA_INT_STATUS);
> > +
> > +	/*
> > +	 * Ack the pending IRQ all to let hardware know software is handling
> > +	 * those finished physical descriptors. Otherwise, the hardware would
> > +	 * keep the used IRQ line in certain trigger state.
> > +	 */
> > +	mtk_dma_write(hsdma, MTK_HSDMA_INT_STATUS, status);
> > +
> > +	while (1) {
> > +		next = MTK_HSDMA_NEXT_DESP_IDX(pc->ring.cur_rptr,
> > +					       MTK_DMA_SIZE);
> 
> shouldn't we check if next is in range, we can crash if we get bad value
> from hardware..

okay, there are checks for next with ddone bit check and null check in
the corresponding descriptor as the following.

> > +		rxd = &pc->ring.rxd[next];
> > +
> > +		/*
> > +		 * If MTK_HSDMA_DESC_DDONE is no specified, that means data
> > +		 * moving for the PD is still under going.
> > +		 */
> > +		desc2 = READ_ONCE(rxd->desc2);
> > +		if (!(desc2 & hsdma->soc->ddone))
> > +			break;
> 
> okay this is one break
> 
> > +
> > +		cb = &pc->ring.cb[next];
> > +		if (unlikely(!cb->vd)) {
> > +			dev_err(hsdma2dev(hsdma), "cb->vd cannot be null\n");
> > +			break;
> 
> and other for null, i feel we need to have checks for while(1) to break,
> this can run infinitely if something bad happens, a fail safe would be good,
> that too this being invoked from isr.
> 

Agreed, I will have more consideration to add a way for fail safe, such
as timeout.

> > +static enum dma_status mtk_hsdma_tx_status(struct dma_chan *c,
> > +					   dma_cookie_t cookie,
> > +					   struct dma_tx_state *txstate)
> > +{
> > +	struct mtk_hsdma_vchan *hvc = to_hsdma_vchan(c);
> > +	struct mtk_hsdma_vdesc *hvd;
> > +	struct virt_dma_desc *vd;
> > +	enum dma_status ret;
> > +	unsigned long flags;
> > +	size_t bytes = 0;
> > +
> > +	ret = dma_cookie_status(c, cookie, txstate);
> > +	if (ret == DMA_COMPLETE || !txstate)
> > +		return ret;
> > +
> > +	spin_lock_irqsave(&hvc->vc.lock, flags);
> > +	vd = mtk_hsdma_find_active_desc(c, cookie);
> > +	spin_unlock_irqrestore(&hvc->vc.lock, flags);
> > +
> > +	if (vd) {
> > +		hvd = to_hsdma_vdesc(vd);
> > +		bytes = hvd->residue;
> 
> for active descriptor, shouldn't you read counters from hardware?
> 

the hardware doesn't support counters for residue for any active
descriptor. this residue is completely maintained in software way.

> > +static struct dma_async_tx_descriptor *
> > +mtk_hsdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest,
> > +			  dma_addr_t src, size_t len, unsigned long flags)
> > +{
> > +	struct mtk_hsdma_vdesc *hvd;
> > +
> > +	hvd = kzalloc(sizeof(*hvd), GFP_NOWAIT);
> 
> GFP_NOWAIT here too

It's already GFP_NOWAIT. And I will check more with all memory
allocation with the GFP_NOWAIT.

> 
> > +static int mtk_hsdma_terminate_all(struct dma_chan *c)
> > +{
> > +	mtk_hsdma_free_inactive_desc(c, false);
> 
> only inactive, active ones need to be freed and channel cleaned
> 

okay, also make all active ones to be freed.

Re: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC

[Vinod Koul]

On Thu, Mar 01, 2018 at 06:27:01PM +0800, Sean Wang wrote:
> On Thu, 2018-03-01 at 13:53 +0530, Vinod Koul wrote:
> > On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang@mediatek.com wrote:
> > 
> > > @@ -0,0 +1,1054 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > // Copyright ...
> > 
> > The copyright line needs to follow SPDX tag line
> > 
> 
> okay, I will make it reorder and be something like that
> 
> // SPDX-License-Identifier: GPL-2.0
> /*
>  * Copyright (c) 2017-2018 MediaTek Inc.
>  * Author: Sean Wang <sean.wang@mediatek.com>
>  *
>  * Driver for MediaTek High-Speed DMA Controller
>  *
>  */

It needs to be:

// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2017-2018 MediaTek Inc.

/*
 * whatever else you want
 */

The first two lines are in C99 style comment and need to have SPDX tag and
Copyright info

> the point is I learned from other subsystem makes the driver name be
> same with the module name with KBUILD_MODNAME.
> 
> If you really don't like it, I can just change it into 
> 
> #define MTK_DMA_DEV "mtk-hsdma"

It is used only once, why not use KBUILD_MODNAME directly?

> 
> > > +
> > > +#define MTK_HSDMA_USEC_POLL		20
> > > +#define MTK_HSDMA_TIMEOUT_POLL		200000
> > > +#define MTK_HSDMA_DMA_BUSWIDTHS		BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > 
> > Undefined buswidth??

??

> > > +/**
> > > + * struct mtk_hsdma_pdesc - This is the struct holding info describing physical
> > > + *			    descriptor (PD) and its placement must be kept at
> > > + *			    4-bytes alignment in little endian order.
> > > + * @desc[1-4]:		    The control pad used to indicate hardware how to
> > 
> > pls align to 80char or lesser
> > 
> 
> weird, it seems the line is already with 80 char and pass the
> checkpatch.pl. or do I misunderstand something ?

Okay please check. With text it helps to wrap before that

> > > +	/*
> > > +	 * Updating into hardware the pointer of TX ring lets HSDMA to take
> > > +	 * action for those pending PDs.
> > > +	 */
> > > +	mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr);
> > > +
> > > +	spin_unlock_irqrestore(&hsdma->lock, flags);
> > > +
> > > +	return !hvd->len ? 0 : -ENOSPC;
> > 
> > you already wrote and started txn, so why this?
> > 
> 
> it's possible just partial virtual descriptor fits into hardware and
> then return -ENOSPC. And it will start it to complete the remaining part
> as soon as possible when some rooms is being freed.

Either ways you have issued the descriptor, so you succeed right?

> > shouldn't we check if next is in range, we can crash if we get bad value
> > from hardware..
> 
> okay, there are checks for next with ddone bit check and null check in
> the corresponding descriptor as the following.

what if you get bad next value

> 
> > > +		rxd = &pc->ring.rxd[next];

resulting in bad ref here

-- 
~Vinod

Re: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC

[Sean Wang]

Hi, Vinod

On Thu, 2018-03-01 at 18:26 +0530, Vinod Koul wrote:
> On Thu, Mar 01, 2018 at 06:27:01PM +0800, Sean Wang wrote:
> > On Thu, 2018-03-01 at 13:53 +0530, Vinod Koul wrote:
> > > On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang@mediatek.com wrote:
> > > 
> > > > @@ -0,0 +1,1054 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > // Copyright ...
> > > 
> > > The copyright line needs to follow SPDX tag line
> > > 
> > 
> > okay, I will make it reorder and be something like that
> > 
> > // SPDX-License-Identifier: GPL-2.0
> > /*
> >  * Copyright (c) 2017-2018 MediaTek Inc.
> >  * Author: Sean Wang <sean.wang@mediatek.com>
> >  *
> >  * Driver for MediaTek High-Speed DMA Controller
> >  *
> >  */
> 
> It needs to be:
> 
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (c) 2017-2018 MediaTek Inc.
> 
> /*
>  * whatever else you want
>  */
> 
> The first two lines are in C99 style comment and need to have SPDX tag and
> Copyright info

Sure, I can do it using C99 style comments at the first two lines.

In addition, I'm really curious where we can find a reference to the
rule and if it 's a strict rule for all the drivers.

Because I'm considering whether I should turn other driver into using
the same rule.

> > the point is I learned from other subsystem makes the driver name be
> > same with the module name with KBUILD_MODNAME.
> > 
> > If you really don't like it, I can just change it into 
> > 
> > #define MTK_DMA_DEV "mtk-hsdma"
> 
> It is used only once, why not use KBUILD_MODNAME directly?
> 

Yup, it can use KBUILD_MODNAME directly.

> > 
> > > > +
> > > > +#define MTK_HSDMA_USEC_POLL		20
> > > > +#define MTK_HSDMA_TIMEOUT_POLL		200000
> > > > +#define MTK_HSDMA_DMA_BUSWIDTHS		BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > > 
> > > Undefined buswidth??
> 
> ??

Sorry for I didn't answer the question in the short time.

After spending some time on a confirmation with design, it is
DMA_SLAVE_BUSWIDTH_4_BYTES and not be configurable. 

> 
> > > > +/**
> > > > + * struct mtk_hsdma_pdesc - This is the struct holding info describing physical
> > > > + *			    descriptor (PD) and its placement must be kept at
> > > > + *			    4-bytes alignment in little endian order.
> > > > + * @desc[1-4]:		    The control pad used to indicate hardware how to
> > > 
> > > pls align to 80char or lesser
> > > 
> > 
> > weird, it seems the line is already with 80 char and pass the
> > checkpatch.pl. or do I misunderstand something ?
> 
> Okay please check. With text it helps to wrap before that
> 

After check again, these lines are all already aligned to 80 chars

> > > > +	/*
> > > > +	 * Updating into hardware the pointer of TX ring lets HSDMA to take
> > > > +	 * action for those pending PDs.
> > > > +	 */
> > > > +	mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr);
> > > > +
> > > > +	spin_unlock_irqrestore(&hsdma->lock, flags);
> > > > +
> > > > +	return !hvd->len ? 0 : -ENOSPC;
> > > 
> > > you already wrote and started txn, so why this?
> > > 
> > 
> > it's possible just partial virtual descriptor fits into hardware and
> > then return -ENOSPC. And it will start it to complete the remaining part
> > as soon as possible when some rooms is being freed.
> 
> Either ways you have issued the descriptor, so you succeed right?
> 

I think I should get your points.

I guessed what you meant is that it should be returning 0 instead of
-ENOSPC for all successful descriptor issuing either in part or in full

I will refine this flow based on the thought.

> > > shouldn't we check if next is in range, we can crash if we get bad value
> > > from hardware..
> > 
> > okay, there are checks for next with ddone bit check and null check in
> > the corresponding descriptor as the following.
> 
> what if you get bad next value
> 

next is not hardware value. it's maintained by software which is always
between 0 to MTK_DMA_SIZE - 1, and definitely doesn't get a bad value.

> > 
> > > > +		rxd = &pc->ring.rxd[next];
> 
> resulting in bad ref here

rxd is also definitely a good ref


> 

Re: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC

[Vinod Koul]

On Fri, Mar 02, 2018 at 02:47:51PM +0800, Sean Wang wrote:
> Hi, Vinod
> 
> On Thu, 2018-03-01 at 18:26 +0530, Vinod Koul wrote:
> > On Thu, Mar 01, 2018 at 06:27:01PM +0800, Sean Wang wrote:
> > > On Thu, 2018-03-01 at 13:53 +0530, Vinod Koul wrote:
> > > > On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang@mediatek.com wrote:
> > > > 
> > > > > @@ -0,0 +1,1054 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > // Copyright ...
> > > > 
> > > > The copyright line needs to follow SPDX tag line
> > > > 
> > > 
> > > okay, I will make it reorder and be something like that
> > > 
> > > // SPDX-License-Identifier: GPL-2.0
> > > /*
> > >  * Copyright (c) 2017-2018 MediaTek Inc.
> > >  * Author: Sean Wang <sean.wang@mediatek.com>
> > >  *
> > >  * Driver for MediaTek High-Speed DMA Controller
> > >  *
> > >  */
> > 
> > It needs to be:
> > 
> > // SPDX-License-Identifier: GPL-2.0
> > // Copyright (c) 2017-2018 MediaTek Inc.
> > 
> > /*
> >  * whatever else you want
> >  */
> > 
> > The first two lines are in C99 style comment and need to have SPDX tag and
> > Copyright info
> 
> Sure, I can do it using C99 style comments at the first two lines.
> 
> In addition, I'm really curious where we can find a reference to the
> rule and if it 's a strict rule for all the drivers.
> 
> Because I'm considering whether I should turn other driver into using
> the same rule.

Yes that seems to be the rule now https://lkml.org/lkml/2017/11/2/715

> > > > > +#define MTK_HSDMA_USEC_POLL		20
> > > > > +#define MTK_HSDMA_TIMEOUT_POLL		200000
> > > > > +#define MTK_HSDMA_DMA_BUSWIDTHS		BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > > > 
> > > > Undefined buswidth??
> > 
> > ??
> 
> Sorry for I didn't answer the question in the short time.
> 
> After spending some time on a confirmation with design, it is
> DMA_SLAVE_BUSWIDTH_4_BYTES and not be configurable. 

Then it should be DMA_SLAVE_BUSWIDTH_4_BYTES and not
DMA_SLAVE_BUSWIDTH_UNDEFINED...

> > > > shouldn't we check if next is in range, we can crash if we get bad value
> > > > from hardware..
> > > 
> > > okay, there are checks for next with ddone bit check and null check in
> > > the corresponding descriptor as the following.
> > 
> > what if you get bad next value
> > 
> 
> next is not hardware value. it's maintained by software which is always
> between 0 to MTK_DMA_SIZE - 1, and definitely doesn't get a bad value.
> 
> > > 
> > > > > +		rxd = &pc->ring.rxd[next];
> > 
> > resulting in bad ref here
> 
> rxd is also definitely a good ref

not if next is out of range, say you read -1 or 200000?

-- 
~Vinod

Re: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC

[Sean Wang]

On Fri, 2018-03-02 at 13:47 +0530, Vinod Koul wrote:
> On Fri, Mar 02, 2018 at 02:47:51PM +0800, Sean Wang wrote:
> > Hi, Vinod
> > 
> > On Thu, 2018-03-01 at 18:26 +0530, Vinod Koul wrote:
> > > On Thu, Mar 01, 2018 at 06:27:01PM +0800, Sean Wang wrote:
> > > > On Thu, 2018-03-01 at 13:53 +0530, Vinod Koul wrote:
> > > > > On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang@mediatek.com wrote:
> > > > > 
> > > > > > @@ -0,0 +1,1054 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > // Copyright ...
> > > > > 
> > > > > The copyright line needs to follow SPDX tag line
> > > > > 
> > > > 
> > > > okay, I will make it reorder and be something like that
> > > > 
> > > > // SPDX-License-Identifier: GPL-2.0
> > > > /*
> > > >  * Copyright (c) 2017-2018 MediaTek Inc.
> > > >  * Author: Sean Wang <sean.wang@mediatek.com>
> > > >  *
> > > >  * Driver for MediaTek High-Speed DMA Controller
> > > >  *
> > > >  */
> > > 
> > > It needs to be:
> > > 
> > > // SPDX-License-Identifier: GPL-2.0
> > > // Copyright (c) 2017-2018 MediaTek Inc.
> > > 
> > > /*
> > >  * whatever else you want
> > >  */
> > > 
> > > The first two lines are in C99 style comment and need to have SPDX tag and
> > > Copyright info
> > 
> > Sure, I can do it using C99 style comments at the first two lines.
> > 
> > In addition, I'm really curious where we can find a reference to the
> > rule and if it 's a strict rule for all the drivers.
> > 
> > Because I'm considering whether I should turn other driver into using
> > the same rule.
> 
> Yes that seems to be the rule now https://lkml.org/lkml/2017/11/2/715
> 

Where could I find the rule for the copyright line needed to follow SPDX
tag line ?

currently, I still seen tons of drivers putting its copyright inside 
/* ... 
 * 
 */

> > > > > > +#define MTK_HSDMA_USEC_POLL		20
> > > > > > +#define MTK_HSDMA_TIMEOUT_POLL		200000
> > > > > > +#define MTK_HSDMA_DMA_BUSWIDTHS		BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > > > > 
> > > > > Undefined buswidth??
> > > 
> > > ??
> > 
> > Sorry for I didn't answer the question in the short time.
> > 
> > After spending some time on a confirmation with design, it is
> > DMA_SLAVE_BUSWIDTH_4_BYTES and not be configurable. 
> 
> Then it should be DMA_SLAVE_BUSWIDTH_4_BYTES and not
> DMA_SLAVE_BUSWIDTH_UNDEFINED...
> 

understood, I will do it.

> > > > > shouldn't we check if next is in range, we can crash if we get bad value
> > > > > from hardware..
> > > > 
> > > > okay, there are checks for next with ddone bit check and null check in
> > > > the corresponding descriptor as the following.
> > > 
> > > what if you get bad next value
> > > 
> > 
> > next is not hardware value. it's maintained by software which is always
> > between 0 to MTK_DMA_SIZE - 1, and definitely doesn't get a bad value.
> > 
> > > > 
> > > > > > +		rxd = &pc->ring.rxd[next];
> > > 
> > > resulting in bad ref here
> > 
> > rxd is also definitely a good ref
> 
> not if next is out of range, say you read -1 or 200000?
> 

next must be in range because next is calculated by
MTK_HSDMA_NEXT_DESP_IDX macro which is just a modulo operation
with MTK_DMA_SIZE.

Currently, MTK_DMA_SIZE is equal to 64 and thus next must be 0 to 63 and
wraparound.

As to MTK_HSDMA_NEXT_DESP_IDX , it is defined as the following
#define MTK_HSDMA_NEXT_DESP_IDX(x, y)   (((x) + 1) & ((y) - 1))