2019-01-02 03:57:27

by Long Cheng

Subject: [PATCH v9 0/2] add uart DMA function

In Mediatek SOCs, the uart can support DMA function.
Base on DMA engine formwork, we add the DMA code to support uart. And put the code under drivers/dma.

This series contains document bindings, Kconfig to control the function enable or not,
device tree including interrupt and dma device node, the code of UART DMA

Changes compared to v8
-revise missing items
Changes compared to v7:
-modify apdma uart tx
Changes compared to v6:
-Correct spelling
Changes compared to v5:
-move 'requst irqs' to alloc channel
-remove tasklet.
Changes compared to v4:
-modify Kconfig depends on.
Changes compared to v3:
-fix CONFIG_PM, will cause build fail
Changes compared to v2:
-remove unimportant parameters
-instead of cookie, use APIs of virtual channel.
-use of_dma_xlate_by_chan_id.
Changes compared to v1:
-mian revised file, 8250_mtk_dma.c
--parameters renamed for standard
--remove atomic operation

2019-01-02 04:02:56

by Long Cheng

Subject: [PATCH v9 2/2] arm: dts: mt2712: add uart APDMA to device tree

1. add uart APDMA controller device node
2. add uart 0/1/2/3/4/5 DMA function

Signed-off-by: Long Cheng <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 50 +++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
index 976d92a..be1a22a 100644
--- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
@@ -300,6 +300,9 @@
interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 10
+ &apdma 11>;
+ dma-names = "tx", "rx";
status = "disabled";
};

@@ -369,6 +372,38 @@
(GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_HIGH)>;
};

+ apdma: dma-controller@11000400 {
+ compatible = "mediatek,mt2712-uart-dma",
+ "mediatek,mt6577-uart-dma";
+ reg = <0 0x11000400 0 0x80>,
+ <0 0x11000480 0 0x80>,
+ <0 0x11000500 0 0x80>,
+ <0 0x11000580 0 0x80>,
+ <0 0x11000600 0 0x80>,
+ <0 0x11000680 0 0x80>,
+ <0 0x11000700 0 0x80>,
+ <0 0x11000780 0 0x80>,
+ <0 0x11000800 0 0x80>,
+ <0 0x11000880 0 0x80>,
+ <0 0x11000900 0 0x80>,
+ <0 0x11000980 0 0x80>;
+ interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 104 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 105 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 106 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 107 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 108 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 109 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 110 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 111 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 112 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 113 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 114 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&pericfg CLK_PERI_AP_DMA>;
+ clock-names = "apdma";
+ #dma-cells = <1>;
+ };
+
auxadc: adc@11001000 {
compatible = "mediatek,mt2712-auxadc";
reg = <0 0x11001000 0 0x1000>;
@@ -385,6 +420,9 @@
interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 0
+ &apdma 1>;
+ dma-names = "tx", "rx";
status = "disabled";
};

@@ -395,6 +433,9 @@
interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 2
+ &apdma 3>;
+ dma-names = "tx", "rx";
status = "disabled";
};

@@ -405,6 +446,9 @@
interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 4
+ &apdma 5>;
+ dma-names = "tx", "rx";
status = "disabled";
};

@@ -415,6 +459,9 @@
interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 6
+ &apdma 7>;
+ dma-names = "tx", "rx";
status = "disabled";
};

@@ -629,6 +676,9 @@
interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 8
+ &apdma 9>;
+ dma-names = "tx", "rx";
status = "disabled";
};

--
1.7.9.5

2019-01-02 04:12:05

by Long Cheng

Subject: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

In DMA engine framework, add 8250 uart dma to support MediaTek uart.
If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
the performance, can enable the function.

Signed-off-by: Long Cheng <[email protected]>
---
drivers/dma/mediatek/8250_mtk_dma.c | 652 +++++++++++++++++++++++++++++++++++
drivers/dma/mediatek/Kconfig | 11 +
drivers/dma/mediatek/Makefile | 1 +
3 files changed, 664 insertions(+)
create mode 100644 drivers/dma/mediatek/8250_mtk_dma.c

diff --git a/drivers/dma/mediatek/8250_mtk_dma.c b/drivers/dma/mediatek/8250_mtk_dma.c
new file mode 100644
index 0000000..dbf811e
--- /dev/null
+++ b/drivers/dma/mediatek/8250_mtk_dma.c
@@ -0,0 +1,652 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MediaTek 8250 DMA driver.
+ *
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Long Cheng <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_dma.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "../virt-dma.h"
+
+#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
+
+#define VFF_EN_B BIT(0)
+#define VFF_STOP_B BIT(0)
+#define VFF_FLUSH_B BIT(0)
+#define VFF_4G_SUPPORT_B BIT(0)
+#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
+#define VFF_RX_INT_EN1_B BIT(1)
+#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
+#define VFF_WARM_RST_B BIT(0)
+#define VFF_RX_INT_FLAG_CLR_B (BIT(0) | BIT(1))
+#define VFF_TX_INT_FLAG_CLR_B 0
+#define VFF_STOP_CLR_B 0
+#define VFF_INT_EN_CLR_B 0
+#define VFF_4G_SUPPORT_CLR_B 0
+
+/* interrupt trigger level for tx */
+#define VFF_TX_THRE(n) ((n) * 7 / 8)
+/* interrupt trigger level for rx */
+#define VFF_RX_THRE(n) ((n) * 3 / 4)
+
+#define VFF_RING_SIZE 0xffffU
+/* invert this bit when wrap ring head again*/
+#define VFF_RING_WRAP 0x10000U
+
+#define VFF_INT_FLAG 0x00
+#define VFF_INT_EN 0x04
+#define VFF_EN 0x08
+#define VFF_RST 0x0c
+#define VFF_STOP 0x10
+#define VFF_FLUSH 0x14
+#define VFF_ADDR 0x1c
+#define VFF_LEN 0x24
+#define VFF_THRE 0x28
+#define VFF_WPT 0x2c
+#define VFF_RPT 0x30
+/*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
+#define VFF_VALID_SIZE 0x3c
+/*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
+#define VFF_LEFT_SIZE 0x40
+#define VFF_DEBUG_STATUS 0x50
+#define VFF_4G_SUPPORT 0x54
+
+struct mtk_uart_apdmadev {
+ struct dma_device ddev;
+ struct clk *clk;
+ bool support_33bits;
+ unsigned int dma_irq[MTK_UART_APDMA_CHANNELS];
+};
+
+struct mtk_uart_apdma_desc {
+ struct virt_dma_desc vd;
+
+ unsigned int avail_len;
+};
+
+struct mtk_chan {
+ struct virt_dma_chan vc;
+ struct dma_slave_config cfg;
+ void __iomem *base;
+ struct mtk_uart_apdma_desc *desc;
+
+ bool requested;
+
+ unsigned int rx_status;
+};
+
+static inline struct mtk_uart_apdmadev *
+to_mtk_uart_apdma_dev(struct dma_device *d)
+{
+ return container_of(d, struct mtk_uart_apdmadev, ddev);
+}
+
+static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
+{
+ return container_of(c, struct mtk_chan, vc.chan);
+}
+
+static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
+ (struct dma_async_tx_descriptor *t)
+{
+ return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
+}
+
+static void mtk_uart_apdma_write(struct mtk_chan *c,
+ unsigned int reg, unsigned int val)
+{
+ writel(val, c->base + reg);
+}
+
+static unsigned int mtk_uart_apdma_read(struct mtk_chan *c, unsigned int reg)
+{
+ return readl(c->base + reg);
+}
+
+static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
+{
+ struct dma_chan *chan = vd->tx.chan;
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+
+ kfree(c->desc);
+}
+
+static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
+{
+ unsigned int len, send, left, wpt, d_wpt, tmp;
+ int ret;
+
+ left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
+ if (!left) {
+ mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
+ return;
+ }
+
+ /* Wait 1sec for flush, can't sleep*/
+ ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
+ tmp != VFF_FLUSH_B, 0, 1000000);
+ if (ret)
+ dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
+ mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
+
+ send = min_t(unsigned int, left, c->desc->avail_len);
+ wpt = mtk_uart_apdma_read(c, VFF_WPT);
+ len = mtk_uart_apdma_read(c, VFF_LEN);
+
+ d_wpt = wpt + send;
+ if ((d_wpt & VFF_RING_SIZE) >= len) {
+ d_wpt = d_wpt - len;
+ d_wpt = d_wpt ^ VFF_RING_WRAP;
+ }
+ mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
+
+ c->desc->avail_len -= send;
+
+ mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
+ if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
+ mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
+}
+
+static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
+{
+ struct mtk_uart_apdma_desc *d = c->desc;
+ unsigned int len, wg, rg, cnt;
+
+ if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
+ !d || !vchan_next_desc(&c->vc))
+ return;
+
+ len = mtk_uart_apdma_read(c, VFF_LEN);
+ rg = mtk_uart_apdma_read(c, VFF_RPT);
+ wg = mtk_uart_apdma_read(c, VFF_WPT);
+ if ((rg ^ wg) & VFF_RING_WRAP)
+ cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
+ else
+ cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
+
+ c->rx_status = cnt;
+ mtk_uart_apdma_write(c, VFF_RPT, wg);
+
+ list_del(&d->vd.node);
+ vchan_cookie_complete(&d->vd);
+}
+
+static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
+{
+ struct dma_chan *chan = (struct dma_chan *)dev_id;
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ struct mtk_uart_apdma_desc *d;
+ unsigned long flags;
+
+ spin_lock_irqsave(&c->vc.lock, flags);
+ if (c->cfg.direction == DMA_DEV_TO_MEM) {
+ mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
+ mtk_uart_apdma_start_rx(c);
+ } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
+ d = c->desc;
+
+ mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
+
+ if (d->avail_len != 0U) {
+ mtk_uart_apdma_start_tx(c);
+ } else {
+ list_del(&d->vd.node);
+ vchan_cookie_complete(&d->vd);
+ }
+ }
+ spin_unlock_irqrestore(&c->vc.lock, flags);
+
+ return IRQ_HANDLED;
+}
+
+static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
+{
+ struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ u32 tmp;
+ int ret;
+
+ pm_runtime_get_sync(mtkd->ddev.dev);
+
+ mtk_uart_apdma_write(c, VFF_ADDR, 0);
+ mtk_uart_apdma_write(c, VFF_THRE, 0);
+ mtk_uart_apdma_write(c, VFF_LEN, 0);
+ mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
+
+ ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
+ tmp == 0, 10, 100);
+ if (ret) {
+ dev_err(chan->device->dev, "dma reset: fail, timeout\n");
+ return ret;
+ }
+
+ if (!c->requested) {
+ c->requested = true;
+ ret = request_irq(mtkd->dma_irq[chan->chan_id],
+ mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
+ KBUILD_MODNAME, chan);
+ if (ret < 0) {
+ dev_err(chan->device->dev, "Can't request dma IRQ\n");
+ return -EINVAL;
+ }
+ }
+
+ if (mtkd->support_33bits)
+ mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
+
+ return ret;
+}
+
+static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
+{
+ struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+
+ if (c->requested) {
+ c->requested = false;
+ free_irq(mtkd->dma_irq[chan->chan_id], chan);
+ }
+
+ tasklet_kill(&c->vc.task);
+
+ vchan_free_chan_resources(&c->vc);
+
+ pm_runtime_put_sync(mtkd->ddev.dev);
+}
+
+static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
+ dma_cookie_t cookie,
+ struct dma_tx_state *txstate)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ enum dma_status ret;
+ unsigned long flags;
+
+ if (!txstate)
+ return DMA_ERROR;
+
+ ret = dma_cookie_status(chan, cookie, txstate);
+ spin_lock_irqsave(&c->vc.lock, flags);
+ if (ret == DMA_IN_PROGRESS) {
+ c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
+ dma_set_residue(txstate, c->rx_status);
+ } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
+ dma_set_residue(txstate, c->rx_status);
+ } else {
+ dma_set_residue(txstate, 0);
+ }
+ spin_unlock_irqrestore(&c->vc.lock, flags);
+
+ return ret;
+}
+
+/*
+ * dmaengine_prep_slave_single will call the function. and sglen is 1.
+ * 8250 uart using one ring buffer, and deal with one sg.
+ */
+static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
+ (struct dma_chan *chan, struct scatterlist *sgl,
+ unsigned int sglen, enum dma_transfer_direction dir,
+ unsigned long tx_flags, void *context)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ struct mtk_uart_apdma_desc *d;
+
+ if ((dir != DMA_DEV_TO_MEM) &&
+ (dir != DMA_MEM_TO_DEV)) {
+ dev_err(chan->device->dev, "bad direction\n");
+ return NULL;
+ }
+
+ /* Now allocate and setup the descriptor */
+ d = kzalloc(sizeof(*d), GFP_ATOMIC);
+ if (!d)
+ return NULL;
+
+ /* sglen is 1 */
+ d->avail_len = sg_dma_len(sgl);
+
+ return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
+}
+
+static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ struct virt_dma_desc *vd;
+ unsigned long flags;
+
+ spin_lock_irqsave(&c->vc.lock, flags);
+ if (c->cfg.direction == DMA_DEV_TO_MEM) {
+ if (vchan_issue_pending(&c->vc)) {
+ vd = vchan_next_desc(&c->vc);
+ c->desc = to_mtk_uart_apdma_desc(&vd->tx);
+ mtk_uart_apdma_start_rx(c);
+ }
+ } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
+ if (vchan_issue_pending(&c->vc)) {
+ vd = vchan_next_desc(&c->vc);
+ c->desc = to_mtk_uart_apdma_desc(&vd->tx);
+ mtk_uart_apdma_start_tx(c);
+ }
+ }
+ spin_unlock_irqrestore(&c->vc.lock, flags);
+}
+
+static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
+ struct dma_slave_config *cfg)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ struct mtk_uart_apdmadev *mtkd =
+ to_mtk_uart_apdma_dev(c->vc.chan.device);
+
+ c->cfg = *cfg;
+
+ if (cfg->direction == DMA_DEV_TO_MEM) {
+ unsigned int rx_len = cfg->src_addr_width * 1024;
+
+ mtk_uart_apdma_write(c, VFF_ADDR, cfg->src_addr);
+ mtk_uart_apdma_write(c, VFF_LEN, rx_len);
+ mtk_uart_apdma_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
+ mtk_uart_apdma_write(c, VFF_INT_EN,
+ VFF_RX_INT_EN0_B | VFF_RX_INT_EN1_B);
+ mtk_uart_apdma_write(c, VFF_RPT, 0);
+ mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
+ mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
+ } else if (cfg->direction == DMA_MEM_TO_DEV) {
+ unsigned int tx_len = cfg->dst_addr_width * 1024;
+
+ mtk_uart_apdma_write(c, VFF_ADDR, cfg->dst_addr);
+ mtk_uart_apdma_write(c, VFF_LEN, tx_len);
+ mtk_uart_apdma_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
+ mtk_uart_apdma_write(c, VFF_WPT, 0);
+ mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
+ mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
+ }
+
+ if (mtkd->support_33bits)
+ mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
+
+ if (mtk_uart_apdma_read(c, VFF_EN) != VFF_EN_B) {
+ dev_err(chan->device->dev, "dir[%d] fail\n", cfg->direction);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ unsigned long flags;
+ u32 tmp;
+ int ret;
+
+ spin_lock_irqsave(&c->vc.lock, flags);
+
+ mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
+ /* Wait 1sec for flush, can't sleep*/
+ ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
+ tmp != VFF_FLUSH_B, 0, 1000000);
+ if (ret)
+ dev_err(c->vc.chan.device->dev, "flush: fail, debug=0x%x\n",
+ mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
+
+ /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
+ mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_B);
+ ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
+ tmp == 0, 10, 100);
+ if (ret)
+ dev_err(c->vc.chan.device->dev, "stop: fail, debug=0x%x\n",
+ mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
+
+ mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_CLR_B);
+ mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
+
+ if (c->cfg.direction == DMA_DEV_TO_MEM)
+ mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
+ else if (c->cfg.direction == DMA_MEM_TO_DEV)
+ mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
+
+ spin_unlock_irqrestore(&c->vc.lock, flags);
+
+ return 0;
+}
+
+static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
+{
+ /* just for check caps pass */
+ return 0;
+}
+
+static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
+{
+ /* just for check caps pass */
+ return 0;
+}
+
+static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
+{
+ while (list_empty(&mtkd->ddev.channels) == 0) {
+ struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
+ struct mtk_chan, vc.chan.device_node);
+
+ list_del(&c->vc.chan.device_node);
+ tasklet_kill(&c->vc.task);
+ }
+}
+
+static const struct of_device_id mtk_uart_apdma_match[] = {
+ { .compatible = "mediatek,mt6577-uart-dma", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
+
+static int mtk_uart_apdma_probe(struct platform_device *pdev)
+{
+ struct mtk_uart_apdmadev *mtkd;
+ struct resource *res;
+ struct mtk_chan *c;
+ unsigned int i;
+ int rc;
+
+ mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
+ if (!mtkd)
+ return -ENOMEM;
+
+ mtkd->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(mtkd->clk)) {
+ dev_err(&pdev->dev, "No clock specified\n");
+ rc = PTR_ERR(mtkd->clk);
+ return rc;
+ }
+
+ if (of_property_read_bool(pdev->dev.of_node, "dma-33bits"))
+ mtkd->support_33bits = true;
+
+ rc = dma_set_mask_and_coherent(&pdev->dev,
+ DMA_BIT_MASK(32 | mtkd->support_33bits));
+ if (rc)
+ return rc;
+
+ dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
+ mtkd->ddev.device_alloc_chan_resources =
+ mtk_uart_apdma_alloc_chan_resources;
+ mtkd->ddev.device_free_chan_resources =
+ mtk_uart_apdma_free_chan_resources;
+ mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
+ mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
+ mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
+ mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
+ mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
+ mtkd->ddev.device_resume = mtk_uart_apdma_device_resume;
+ mtkd->ddev.device_terminate_all = mtk_uart_apdma_terminate_all;
+ mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
+ mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
+ mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
+ mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+ mtkd->ddev.dev = &pdev->dev;
+ INIT_LIST_HEAD(&mtkd->ddev.channels);
+
+ for (i = 0; i < MTK_UART_APDMA_CHANNELS; i++) {
+ c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
+ if (!c) {
+ rc = -ENODEV;
+ goto err_no_dma;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+ if (!res) {
+ rc = -ENODEV;
+ goto err_no_dma;
+ }
+
+ c->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(c->base)) {
+ rc = PTR_ERR(c->base);
+ goto err_no_dma;
+ }
+ c->requested = false;
+ c->vc.desc_free = mtk_uart_apdma_desc_free;
+ vchan_init(&c->vc, &mtkd->ddev);
+
+ mtkd->dma_irq[i] = platform_get_irq(pdev, i);
+ if ((int)mtkd->dma_irq[i] < 0) {
+ dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
+ rc = -EINVAL;
+ goto err_no_dma;
+ }
+ }
+
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+
+ rc = dma_async_device_register(&mtkd->ddev);
+ if (rc)
+ goto rpm_disable;
+
+ platform_set_drvdata(pdev, mtkd);
+
+ if (pdev->dev.of_node) {
+ /* Device-tree DMA controller registration */
+ rc = of_dma_controller_register(pdev->dev.of_node,
+ of_dma_xlate_by_chan_id,
+ mtkd);
+ if (rc)
+ goto dma_remove;
+ }
+
+ return rc;
+
+dma_remove:
+ dma_async_device_unregister(&mtkd->ddev);
+rpm_disable:
+ pm_runtime_disable(&pdev->dev);
+err_no_dma:
+ mtk_uart_apdma_free(mtkd);
+ return rc;
+}
+
+static int mtk_uart_apdma_remove(struct platform_device *pdev)
+{
+ struct mtk_uart_apdmadev *mtkd = platform_get_drvdata(pdev);
+
+ if (pdev->dev.of_node)
+ of_dma_controller_free(pdev->dev.of_node);
+
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+
+ dma_async_device_unregister(&mtkd->ddev);
+ mtk_uart_apdma_free(mtkd);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mtk_uart_apdma_suspend(struct device *dev)
+{
+ struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
+
+ if (!pm_runtime_suspended(dev))
+ clk_disable_unprepare(mtkd->clk);
+
+ return 0;
+}
+
+static int mtk_uart_apdma_resume(struct device *dev)
+{
+ int ret;
+ struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
+
+ if (!pm_runtime_suspended(dev)) {
+ ret = clk_prepare_enable(mtkd->clk);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM
+static int mtk_uart_apdma_runtime_suspend(struct device *dev)
+{
+ struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(mtkd->clk);
+
+ return 0;
+}
+
+static int mtk_uart_apdma_runtime_resume(struct device *dev)
+{
+ int ret;
+ struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
+
+ ret = clk_prepare_enable(mtkd->clk);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
+ SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
+ mtk_uart_apdma_runtime_resume, NULL)
+};
+
+static struct platform_driver mtk_uart_apdma_driver = {
+ .probe = mtk_uart_apdma_probe,
+ .remove = mtk_uart_apdma_remove,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .pm = &mtk_uart_apdma_pm_ops,
+ .of_match_table = of_match_ptr(mtk_uart_apdma_match),
+ },
+};
+
+module_platform_driver(mtk_uart_apdma_driver);
+
+MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
+MODULE_AUTHOR("Long Cheng <[email protected]>");
+MODULE_LICENSE("GPL v2");
+
diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
index 27bac0b..1a523c87 100644
--- a/drivers/dma/mediatek/Kconfig
+++ b/drivers/dma/mediatek/Kconfig
@@ -1,4 +1,15 @@

+config DMA_MTK_UART
+ tristate "MediaTek SoCs APDMA support for UART"
+ depends on OF && SERIAL_8250_MT6577
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support for the UART DMA engine found on MediaTek MTK SoCs.
+ when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
+ you can enable the config. the DMA engine can only be used
+ with MediaTek SoCs.
+
config MTK_HSDMA
tristate "MediaTek High-Speed DMA controller support"
depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
index 6e778f8..2f2efd9 100644
--- a/drivers/dma/mediatek/Makefile
+++ b/drivers/dma/mediatek/Makefile
@@ -1 +1,2 @@
+obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
--
1.7.9.5

2019-01-03 04:49:31

by Nicolas Boichat

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

On Wed, Jan 2, 2019 at 10:13 AM Long Cheng <[email protected]> wrote:
>
> In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> the performance, can enable the function.
>
> Signed-off-by: Long Cheng <[email protected]>
> ---
> drivers/dma/mediatek/8250_mtk_dma.c | 652 +++++++++++++++++++++++++++++++++++
> drivers/dma/mediatek/Kconfig | 11 +
> drivers/dma/mediatek/Makefile | 1 +
> 3 files changed, 664 insertions(+)
> create mode 100644 drivers/dma/mediatek/8250_mtk_dma.c
>
> diff --git a/drivers/dma/mediatek/8250_mtk_dma.c b/drivers/dma/mediatek/8250_mtk_dma.c
> new file mode 100644
> index 0000000..dbf811e
> --- /dev/null
> +++ b/drivers/dma/mediatek/8250_mtk_dma.c
> @@ -0,0 +1,652 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MediaTek 8250 DMA driver.
> + *
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Long Cheng <[email protected]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "../virt-dma.h"
> +
> +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> +
> +#define VFF_EN_B BIT(0)
> +#define VFF_STOP_B BIT(0)
> +#define VFF_FLUSH_B BIT(0)
> +#define VFF_4G_SUPPORT_B BIT(0)
> +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> +#define VFF_RX_INT_EN1_B BIT(1)
> +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> +#define VFF_WARM_RST_B BIT(0)
> +#define VFF_RX_INT_FLAG_CLR_B (BIT(0) | BIT(1))
> +#define VFF_TX_INT_FLAG_CLR_B 0
> +#define VFF_STOP_CLR_B 0
> +#define VFF_INT_EN_CLR_B 0
> +#define VFF_4G_SUPPORT_CLR_B 0
> +
> +/* interrupt trigger level for tx */
> +#define VFF_TX_THRE(n) ((n) * 7 / 8)
> +/* interrupt trigger level for rx */
> +#define VFF_RX_THRE(n) ((n) * 3 / 4)
> +
> +#define VFF_RING_SIZE 0xffffU

Well, the size is actually 0x10000. Maybe call this VFF_RING_SIZE_MASK?

> +/* invert this bit when wrap ring head again*/
> +#define VFF_RING_WRAP 0x10000U
> +
> +#define VFF_INT_FLAG 0x00
> +#define VFF_INT_EN 0x04
> +#define VFF_EN 0x08
> +#define VFF_RST 0x0c
> +#define VFF_STOP 0x10
> +#define VFF_FLUSH 0x14
> +#define VFF_ADDR 0x1c
> +#define VFF_LEN 0x24
> +#define VFF_THRE 0x28
> +#define VFF_WPT 0x2c
> +#define VFF_RPT 0x30
> +/*TX: the buffer size HW can read. RX: the buffer size SW can read.*/

nit: Spaces after /* and before */ (and a lot more occurences below,
please fix them all).

> +#define VFF_VALID_SIZE 0x3c
> +/*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
> +#define VFF_LEFT_SIZE 0x40
> +#define VFF_DEBUG_STATUS 0x50
> +#define VFF_4G_SUPPORT 0x54
> +
> +struct mtk_uart_apdmadev {
> + struct dma_device ddev;
> + struct clk *clk;
> + bool support_33bits;
> + unsigned int dma_irq[MTK_UART_APDMA_CHANNELS];
> +};
> +
> +struct mtk_uart_apdma_desc {
> + struct virt_dma_desc vd;
> +
> + unsigned int avail_len;
> +};
> +
> +struct mtk_chan {
> + struct virt_dma_chan vc;
> + struct dma_slave_config cfg;
> + void __iomem *base;
> + struct mtk_uart_apdma_desc *desc;
> +
> + bool requested;
> +
> + unsigned int rx_status;
> +};
> +
> +static inline struct mtk_uart_apdmadev *
> +to_mtk_uart_apdma_dev(struct dma_device *d)
> +{
> + return container_of(d, struct mtk_uart_apdmadev, ddev);
> +}
> +
> +static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
> +{
> + return container_of(c, struct mtk_chan, vc.chan);
> +}
> +
> +static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
> + (struct dma_async_tx_descriptor *t)
> +{
> + return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
> +}
> +
> +static void mtk_uart_apdma_write(struct mtk_chan *c,
> + unsigned int reg, unsigned int val)
> +{
> + writel(val, c->base + reg);
> +}
> +
> +static unsigned int mtk_uart_apdma_read(struct mtk_chan *c, unsigned int reg)
> +{
> + return readl(c->base + reg);
> +}
> +
> +static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
> +{
> + struct dma_chan *chan = vd->tx.chan;
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + kfree(c->desc);
> +}
> +
> +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> +{
> + unsigned int len, send, left, wpt, d_wpt, tmp;
> + int ret;
> +
> + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> + if (!left) {
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + return;
> + }
> +
> + /* Wait 1sec for flush, can't sleep*/

nit: one space after ',', period after 'sleep', space before '*'.

> + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> + tmp != VFF_FLUSH_B, 0, 1000000);
> + if (ret)
> + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));

Why do we need to wait for flush now? The previous implementation did
not require this...

> +
> + send = min_t(unsigned int, left, c->desc->avail_len);
> + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> + len = mtk_uart_apdma_read(c, VFF_LEN);
> +
> + d_wpt = wpt + send;
> + if ((d_wpt & VFF_RING_SIZE) >= len) {

I don't get why you need to add "& VFF_RING_SIZE". If wpt + send >
VFF_RING_SIZE, don't you need to toggle VFF_RING_WRAP too?

> + d_wpt = d_wpt - len;
> + d_wpt = d_wpt ^ VFF_RING_WRAP;
> + }
> + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> +
> + c->desc->avail_len -= send;
> +
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> +}

(thanks for the rest of the changes, this looks much more readable)

> +
> +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> +{
> + struct mtk_uart_apdma_desc *d = c->desc;
> + unsigned int len, wg, rg, cnt;
> +
> + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> + !d || !vchan_next_desc(&c->vc))
> + return;
> +
> + len = mtk_uart_apdma_read(c, VFF_LEN);
> + rg = mtk_uart_apdma_read(c, VFF_RPT);
> + wg = mtk_uart_apdma_read(c, VFF_WPT);
> + if ((rg ^ wg) & VFF_RING_WRAP)
> + cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
> + else
> + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> +
> + c->rx_status = cnt;
> + mtk_uart_apdma_write(c, VFF_RPT, wg);
> +
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> +}
> +
> +static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
> +{
> + struct dma_chan *chan = (struct dma_chan *)dev_id;
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + mtk_uart_apdma_start_rx(c);
> + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> + d = c->desc;
> +
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> +
> + if (d->avail_len != 0U) {
> + mtk_uart_apdma_start_tx(c);
> + } else {
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> + }
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + u32 tmp;

In the rest of the code, you use `unsigned int` as type. I think u32
is a little bit better, but please be consistent.

> + int ret;
> +
> + pm_runtime_get_sync(mtkd->ddev.dev);
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> + mtk_uart_apdma_write(c, VFF_THRE, 0);
> + mtk_uart_apdma_write(c, VFF_LEN, 0);
> + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> +
> + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> + tmp == 0, 10, 100);
> + if (ret) {
> + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> + return ret;
> + }
> +
> + if (!c->requested) {
> + c->requested = true;
> + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> + KBUILD_MODNAME, chan);
> + if (ret < 0) {
> + dev_err(chan->device->dev, "Can't request dma IRQ\n");
> + return -EINVAL;
> + }
> + }
> +
> + if (mtkd->support_33bits)
> + mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> +
> + return ret;
> +}
> +
> +static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + if (c->requested) {
> + c->requested = false;
> + free_irq(mtkd->dma_irq[chan->chan_id], chan);
> + }
> +
> + tasklet_kill(&c->vc.task);
> +
> + vchan_free_chan_resources(&c->vc);
> +
> + pm_runtime_put_sync(mtkd->ddev.dev);
> +}
> +
> +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + enum dma_status ret;
> + unsigned long flags;
> +
> + if (!txstate)
> + return DMA_ERROR;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (ret == DMA_IN_PROGRESS) {
> + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> + dma_set_residue(txstate, c->rx_status);
> + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> + dma_set_residue(txstate, c->rx_status);
> + } else {
> + dma_set_residue(txstate, 0);
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return ret;
> +}
> +
> +/*
> + * dmaengine_prep_slave_single will call the function. and sglen is 1.
> + * 8250 uart using one ring buffer, and deal with one sg.
> + */
> +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> + (struct dma_chan *chan, struct scatterlist *sgl,
> + unsigned int sglen, enum dma_transfer_direction dir,
> + unsigned long tx_flags, void *context)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> +
> + if ((dir != DMA_DEV_TO_MEM) &&
> + (dir != DMA_MEM_TO_DEV)) {

This line fits in 80 chars.

> + dev_err(chan->device->dev, "bad direction\n");
> + return NULL;
> + }
> +
> + /* Now allocate and setup the descriptor */
> + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> + if (!d)
> + return NULL;
> +
> + /* sglen is 1 */
> + d->avail_len = sg_dma_len(sgl);
> +
> + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> +}
> +
> +static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> + if (vchan_issue_pending(&c->vc)) {
> + vd = vchan_next_desc(&c->vc);
> + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> + mtk_uart_apdma_start_rx(c);
> + }
> + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> + if (vchan_issue_pending(&c->vc)) {
> + vd = vchan_next_desc(&c->vc);
> + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> + mtk_uart_apdma_start_tx(c);
> + }
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +}
> +
> +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> + struct dma_slave_config *cfg)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdmadev *mtkd =
> + to_mtk_uart_apdma_dev(c->vc.chan.device);
> +
> + c->cfg = *cfg;
> +
> + if (cfg->direction == DMA_DEV_TO_MEM) {
> + unsigned int rx_len = cfg->src_addr_width * 1024;
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, cfg->src_addr);
> + mtk_uart_apdma_write(c, VFF_LEN, rx_len);
> + mtk_uart_apdma_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> + mtk_uart_apdma_write(c, VFF_INT_EN,
> + VFF_RX_INT_EN0_B | VFF_RX_INT_EN1_B);
> + mtk_uart_apdma_write(c, VFF_RPT, 0);
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
> + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> + unsigned int tx_len = cfg->dst_addr_width * 1024;
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, cfg->dst_addr);
> + mtk_uart_apdma_write(c, VFF_LEN, tx_len);
> + mtk_uart_apdma_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> + mtk_uart_apdma_write(c, VFF_WPT, 0);
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> + mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
> + }
> +
> + if (mtkd->support_33bits)
> + mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> +
> + if (mtk_uart_apdma_read(c, VFF_EN) != VFF_EN_B) {
> + dev_err(chan->device->dev, "dir[%d] fail\n", cfg->direction);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + unsigned long flags;
> + u32 tmp;
> + int ret;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> +
> + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> + /* Wait 1sec for flush, can't sleep*/

ditto about comment format

> + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> + tmp != VFF_FLUSH_B, 0, 1000000);
> + if (ret)
> + dev_err(c->vc.chan.device->dev, "flush: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> + mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_B);
> + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> + tmp == 0, 10, 100);
> + if (ret)
> + dev_err(c->vc.chan.device->dev, "stop: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_CLR_B);
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> +
> + if (c->cfg.direction == DMA_DEV_TO_MEM)
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + else if (c->cfg.direction == DMA_MEM_TO_DEV)
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> +
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + return 0;
> +}

This is still not right... Hopefully somebody more familiar with the
DMA subsystem can weigh in, but maybe it's enough to wait for the
current transfer to be flushed and temporarily disable interrupts?
e.g. call mtk_uart_apdma_terminate_all above?

> +
> +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + return 0;
> +}

Drop this one since you don't really need it.

> +
> +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> +{
> + while (list_empty(&mtkd->ddev.channels) == 0) {

!list_empty(

> + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> + struct mtk_chan, vc.chan.device_node);
> +
> + list_del(&c->vc.chan.device_node);
> + tasklet_kill(&c->vc.task);
> + }
> +}
> +
> +static const struct of_device_id mtk_uart_apdma_match[] = {
> + { .compatible = "mediatek,mt6577-uart-dma", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> +
> +static int mtk_uart_apdma_probe(struct platform_device *pdev)
> +{
> + struct mtk_uart_apdmadev *mtkd;
> + struct resource *res;
> + struct mtk_chan *c;
> + unsigned int i;
> + int rc;
> +
> + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> + if (!mtkd)
> + return -ENOMEM;
> +
> + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(mtkd->clk)) {
> + dev_err(&pdev->dev, "No clock specified\n");
> + rc = PTR_ERR(mtkd->clk);
> + return rc;
> + }
> +
> + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits"))
> + mtkd->support_33bits = true;

I don't think this should be a device tree property. Typically we'd
have multiple compatible strings for (slightly) different HW blocks,
and enable 33bits only on HW that have support.

See how it's done in drivers/i2c/busses/i2c-mt65xx.c, for example.

> +
> + rc = dma_set_mask_and_coherent(&pdev->dev,
> + DMA_BIT_MASK(32 | mtkd->support_33bits));

I'd feel a little more confortable if you used a variable instead:

int dma_bits = 32;

if (support_33bits)
dma_bits = 33;

..., DMA_BIT_MASK(dma_bits));

> + if (rc)
> + return rc;
> +
> + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> + mtkd->ddev.device_alloc_chan_resources =
> + mtk_uart_apdma_alloc_chan_resources;
> + mtkd->ddev.device_free_chan_resources =
> + mtk_uart_apdma_free_chan_resources;
> + mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
> + mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
> + mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
> + mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
> + mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
> + mtkd->ddev.device_resume = mtk_uart_apdma_device_resume;
> + mtkd->ddev.device_terminate_all = mtk_uart_apdma_terminate_all;
> + mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> + mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> + mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> + mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> + mtkd->ddev.dev = &pdev->dev;
> + INIT_LIST_HEAD(&mtkd->ddev.channels);
> +
> + for (i = 0; i < MTK_UART_APDMA_CHANNELS; i++) {
> + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> + if (!c) {
> + rc = -ENODEV;
> + goto err_no_dma;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> + if (!res) {
> + rc = -ENODEV;
> + goto err_no_dma;
> + }
> +
> + c->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(c->base)) {
> + rc = PTR_ERR(c->base);
> + goto err_no_dma;
> + }
> + c->requested = false;
> + c->vc.desc_free = mtk_uart_apdma_desc_free;
> + vchan_init(&c->vc, &mtkd->ddev);
> +
> + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> + if ((int)mtkd->dma_irq[i] < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> + rc = -EINVAL;
> + goto err_no_dma;
> + }
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> +
> + rc = dma_async_device_register(&mtkd->ddev);
> + if (rc)
> + goto rpm_disable;
> +
> + platform_set_drvdata(pdev, mtkd);
> +
> + if (pdev->dev.of_node) {
> + /* Device-tree DMA controller registration */
> + rc = of_dma_controller_register(pdev->dev.of_node,
> + of_dma_xlate_by_chan_id,
> + mtkd);
> + if (rc)
> + goto dma_remove;
> + }
> +
> + return rc;
> +
> +dma_remove:
> + dma_async_device_unregister(&mtkd->ddev);
> +rpm_disable:
> + pm_runtime_disable(&pdev->dev);
> +err_no_dma:
> + mtk_uart_apdma_free(mtkd);
> + return rc;
> +}
> +
> +static int mtk_uart_apdma_remove(struct platform_device *pdev)
> +{
> + struct mtk_uart_apdmadev *mtkd = platform_get_drvdata(pdev);
> +
> + if (pdev->dev.of_node)
> + of_dma_controller_free(pdev->dev.of_node);
> +
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_put_noidle(&pdev->dev);
> +
> + dma_async_device_unregister(&mtkd->ddev);
> + mtk_uart_apdma_free(mtkd);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_uart_apdma_suspend(struct device *dev)
> +{
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev))
> + clk_disable_unprepare(mtkd->clk);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev)) {
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM
> +static int mtk_uart_apdma_runtime_suspend(struct device *dev)
> +{
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(mtkd->clk);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_runtime_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
> + SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
> + mtk_uart_apdma_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver mtk_uart_apdma_driver = {
> + .probe = mtk_uart_apdma_probe,
> + .remove = mtk_uart_apdma_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .pm = &mtk_uart_apdma_pm_ops,
> + .of_match_table = of_match_ptr(mtk_uart_apdma_match),
> + },
> +};
> +
> +module_platform_driver(mtk_uart_apdma_driver);
> +
> +MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
> +MODULE_AUTHOR("Long Cheng <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> +
> diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> index 27bac0b..1a523c87 100644
> --- a/drivers/dma/mediatek/Kconfig
> +++ b/drivers/dma/mediatek/Kconfig
> @@ -1,4 +1,15 @@
>
> +config DMA_MTK_UART
> + tristate "MediaTek SoCs APDMA support for UART"
> + depends on OF && SERIAL_8250_MT6577
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Support for the UART DMA engine found on MediaTek MTK SoCs.
> + when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
> + you can enable the config. the DMA engine can only be used
> + with MediaTek SoCs.
> +
> config MTK_HSDMA
> tristate "MediaTek High-Speed DMA controller support"
> depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
> index 6e778f8..2f2efd9 100644
> --- a/drivers/dma/mediatek/Makefile
> +++ b/drivers/dma/mediatek/Makefile
> @@ -1 +1,2 @@
> +obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
> obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> --
> 1.7.9.5
>

2019-01-03 05:59:00

by Randy Dunlap

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

Hi,

While you are making changes, here are a few more:


On 1/2/19 5:39 PM, Nicolas Boichat wrote:
> diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> index 27bac0b..1a523c87 100644
> --- a/drivers/dma/mediatek/Kconfig
> +++ b/drivers/dma/mediatek/Kconfig
> @@ -1,4 +1,15 @@
>
> +config DMA_MTK_UART
> + tristate "MediaTek SoCs APDMA support for UART"
> + depends on OF && SERIAL_8250_MT6577
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Support for the UART DMA engine found on MediaTek MTK SoCs.
> + when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,

When

> + you can enable the config. the DMA engine can only be used

The

> + with MediaTek SoCs.
> +

Also, use tabs to indent instead of spaces.
The lines (tristate, depends, select, and help) should be indented with one tab.
The help text lines should be indented with one tab + 2 spaces.

> config MTK_HSDMA
> tristate "MediaTek High-Speed DMA controller support"
> depends on ARCH_MEDIATEK || COMPILE_TEST


thanks,
--
~Randy

2019-01-04 18:59:40

by Vinod Koul

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

On 02-01-19, 10:12, Long Cheng wrote:
> In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> the performance, can enable the function.

Is the DMA controller UART specific, can it work with other controllers
as well, if so you should get rid of uart name in patch

> +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)

Why are the channels not coming from DT?

> +
> +#define VFF_EN_B BIT(0)
> +#define VFF_STOP_B BIT(0)
> +#define VFF_FLUSH_B BIT(0)
> +#define VFF_4G_SUPPORT_B BIT(0)
> +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> +#define VFF_RX_INT_EN1_B BIT(1)
> +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/

space around /* space */ also run checkpatch to check for style errors

> +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> +{
> + unsigned int len, send, left, wpt, d_wpt, tmp;
> + int ret;
> +
> + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> + if (!left) {
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + return;
> + }
> +
> + /* Wait 1sec for flush, can't sleep*/
> + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> + tmp != VFF_FLUSH_B, 0, 1000000);
> + if (ret)
> + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + send = min_t(unsigned int, left, c->desc->avail_len);
> + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> + len = mtk_uart_apdma_read(c, VFF_LEN);
> +
> + d_wpt = wpt + send;
> + if ((d_wpt & VFF_RING_SIZE) >= len) {
> + d_wpt = d_wpt - len;
> + d_wpt = d_wpt ^ VFF_RING_WRAP;
> + }
> + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> +
> + c->desc->avail_len -= send;
> +
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> +}
> +
> +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> +{
> + struct mtk_uart_apdma_desc *d = c->desc;
> + unsigned int len, wg, rg, cnt;
> +
> + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> + !d || !vchan_next_desc(&c->vc))
> + return;
> +
> + len = mtk_uart_apdma_read(c, VFF_LEN);
> + rg = mtk_uart_apdma_read(c, VFF_RPT);
> + wg = mtk_uart_apdma_read(c, VFF_WPT);
> + if ((rg ^ wg) & VFF_RING_WRAP)
> + cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
> + else
> + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> +
> + c->rx_status = cnt;
> + mtk_uart_apdma_write(c, VFF_RPT, wg);
> +
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> +}

this looks odd, why do you have different rx and tx start routines?

> +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + u32 tmp;
> + int ret;
> +
> + pm_runtime_get_sync(mtkd->ddev.dev);
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> + mtk_uart_apdma_write(c, VFF_THRE, 0);
> + mtk_uart_apdma_write(c, VFF_LEN, 0);
> + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> +
> + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> + tmp == 0, 10, 100);
> + if (ret) {
> + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> + return ret;
> + }

register read does reset?

> +
> + if (!c->requested) {
> + c->requested = true;
> + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> + KBUILD_MODNAME, chan);

why is the irq not requested in driver probe?

> +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + enum dma_status ret;
> + unsigned long flags;
> +
> + if (!txstate)
> + return DMA_ERROR;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (ret == DMA_IN_PROGRESS) {
> + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> + dma_set_residue(txstate, c->rx_status);
> + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {

why set reside when it is complete? also reside can be null, that should
be checked as well

> +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> + (struct dma_chan *chan, struct scatterlist *sgl,
> + unsigned int sglen, enum dma_transfer_direction dir,
> + unsigned long tx_flags, void *context)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> +
> + if ((dir != DMA_DEV_TO_MEM) &&
> + (dir != DMA_MEM_TO_DEV)) {
> + dev_err(chan->device->dev, "bad direction\n");
> + return NULL;
> + }

we have a macro for this

> +
> + /* Now allocate and setup the descriptor */
> + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> + if (!d)
> + return NULL;
> +
> + /* sglen is 1 */

?

> +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> + struct dma_slave_config *cfg)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdmadev *mtkd =
> + to_mtk_uart_apdma_dev(c->vc.chan.device);
> +
> + c->cfg = *cfg;
> +
> + if (cfg->direction == DMA_DEV_TO_MEM) {

fg->direction is deprecated, in fact I have removed all users recently,
please do not use this

> + unsigned int rx_len = cfg->src_addr_width * 1024;
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, cfg->src_addr);
> + mtk_uart_apdma_write(c, VFF_LEN, rx_len);
> + mtk_uart_apdma_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> + mtk_uart_apdma_write(c, VFF_INT_EN,
> + VFF_RX_INT_EN0_B | VFF_RX_INT_EN1_B);
> + mtk_uart_apdma_write(c, VFF_RPT, 0);
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);

why are we writing this here, we are supposed to do that when txn
starts!

> +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + return 0;
> +}

if you do not support this please remove this!

> +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> +{
> + while (list_empty(&mtkd->ddev.channels) == 0) {
> + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> + struct mtk_chan, vc.chan.device_node);
> +
> + list_del(&c->vc.chan.device_node);
> + tasklet_kill(&c->vc.task);
> + }
> +}
> +
> +static const struct of_device_id mtk_uart_apdma_match[] = {
> + { .compatible = "mediatek,mt6577-uart-dma", },

where is the binding document for ediatek,mt6577-uart-dma ??

--
~Vinod

2019-01-10 08:27:22

by Long Cheng

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

On Thu, 2019-01-03 at 09:39 +0800, Nicolas Boichat wrote:
> On Wed, Jan 2, 2019 at 10:13 AM Long Cheng <[email protected]> wrote:
> >

.....

> > +/* interrupt trigger level for tx */
> > +#define VFF_TX_THRE(n) ((n) * 7 / 8)
> > +/* interrupt trigger level for rx */
> > +#define VFF_RX_THRE(n) ((n) * 3 / 4)
> > +
> > +#define VFF_RING_SIZE 0xffffU
>
> Well, the size is actually 0x10000. Maybe call this VFF_RING_SIZE_MASK?
>

the max length is 0xffff. the bit 16 is wrap bit. our buffer is ring
buffer. So not mask.

> > +/* invert this bit when wrap ring head again*/
> > +#define VFF_RING_WRAP 0x10000U
> > +
> > + writel(val, c->base + reg);
> > +}
> > +

......

> > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > +{
> > + unsigned int len, send, left, wpt, d_wpt, tmp;
> > + int ret;
> > +
> > + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> > + if (!left) {
> > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > + return;
> > + }
> > +
> > + /* Wait 1sec for flush, can't sleep*/
>
> nit: one space after ',', period after 'sleep', space before '*'.
>
> > + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> > + tmp != VFF_FLUSH_B, 0, 1000000);
> > + if (ret)
> > + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> > + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
>
> Why do we need to wait for flush now? The previous implementation did
> not require this...
>

because our HW buffer is cyclic. at one tx, if the size of data is
bigger, data maybe cover. So must wait flush finish. confirm the data is
right. like 128 bytes size, small length can't reproduce the issue.

> > +
> > + send = min_t(unsigned int, left, c->desc->avail_len);
> > + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > +
> > + d_wpt = wpt + send;
> > + if ((d_wpt & VFF_RING_SIZE) >= len) {
>
> I don't get why you need to add "& VFF_RING_SIZE". If wpt + send >
> VFF_RING_SIZE, don't you need to toggle VFF_RING_WRAP too?

the longest actual length is VFF_RING_SIZE. one cyclic, will set bit[16]
to ~bit[16]. the bit[0 ~ 15] is actual address. So need get rid of
bit[16]

>
> > + d_wpt = d_wpt - len;
> > + d_wpt = d_wpt ^ VFF_RING_WRAP;
> > + }
> > + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> > +
> > + c->desc->avail_len -= send;
> > +
> > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> > + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > +}
>
> (thanks for the rest of the changes, this looks much more readable)
>
> > +
......

> > +
> > +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> > +{
> > + /* just for check caps pass */
> > + return 0;
> > +}
>
> This is still not right... Hopefully somebody more familiar with the
> DMA subsystem can weigh in, but maybe it's enough to wait for the
> current transfer to be flushed and temporarily disable interrupts?
> e.g. call mtk_uart_apdma_terminate_all above?
>

i had review the 8250 UART framework. just check the function pointer,
not use. and our HW can't support the feature. So i just keep it at
first version.

> > +
> > +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> > +{
> > + /* just for check caps pass */
> > + return 0;
> > +}
>
> Drop this one since you don't really need it.
>

ok, i will remove it.

> > +
> > +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> > +{
> > + while (list_empty(&mtkd->ddev.channels) == 0) {
>
> !list_empty(
>
> > + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> > + struct mtk_chan, vc.chan.device_node);
> > +
> > + list_del(&c->vc.chan.device_node);
> > + tasklet_kill(&c->vc.task);
> > + }
> > +}
> > +
> > +static const struct of_device_id mtk_uart_apdma_match[] = {
> > + { .compatible = "mediatek,mt6577-uart-dma", },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> > +
> > +static int mtk_uart_apdma_probe(struct platform_device *pdev)
> > +{
> > + struct mtk_uart_apdmadev *mtkd;
> > + struct resource *res;
> > + struct mtk_chan *c;
> > + unsigned int i;
> > + int rc;
> > +
> > + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> > + if (!mtkd)
> > + return -ENOMEM;
> > +
> > + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(mtkd->clk)) {
> > + dev_err(&pdev->dev, "No clock specified\n");
> > + rc = PTR_ERR(mtkd->clk);
> > + return rc;
> > + }
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits"))
> > + mtkd->support_33bits = true;
>
> I don't think this should be a device tree property. Typically we'd
> have multiple compatible strings for (slightly) different HW blocks,
> and enable 33bits only on HW that have support.
>
> See how it's done in drivers/i2c/busses/i2c-mt65xx.c, for example.
>

in MediaTek, not all IC can support 33bit. So i want to configure by DT.
So don't need to modify code in New IC.

> > +
> > + rc = dma_set_mask_and_coherent(&pdev->dev,
> > + DMA_BIT_MASK(32 | mtkd->support_33bits));
>
> I'd feel a little more confortable if you used a variable instead:
>
> int dma_bits = 32;
>
> if (support_33bits)
> dma_bits = 33;
>
> ..., DMA_BIT_MASK(dma_bits));
>

OK, i can modify it. thanks.

> > + if (rc)
> > + return rc;
> > +
> > + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> > + mtkd->ddev.device_alloc_chan_resources =
> > + mtk_uart_apdma_alloc_chan_resources;
> > + mtkd->ddev.device_free_chan_resources =
> > + mtk_uart_apdma_free_chan_resources;
> > + mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
> > + mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
> > + mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
> > + mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
> > + mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
> > + mtkd->ddev.device_resume = mtk_uart_apdma_device_resume;
> > + mtkd->ddev.device_terminate_all = mtk_uart_apdma_terminate_all;
> > + mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > + mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > + mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> > + mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> > + mtkd->ddev.dev = &pdev->dev;
> > + INIT_LIST_HEAD(&mtkd->ddev.channels);
> > +
> > + for (i = 0; i < MTK_UART_APDMA_CHANNELS; i++) {
> > + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> > + if (!c) {
> > + rc = -ENODEV;
> > + goto err_no_dma;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > + if (!res) {
> > + rc = -ENODEV;
> > + goto err_no_dma;
> > + }
> > +
> > + c->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(c->base)) {
> > + rc = PTR_ERR(c->base);
> > + goto err_no_dma;
> > + }
> > + c->requested = false;
> > + c->vc.desc_free = mtk_uart_apdma_desc_free;
> > + vchan_init(&c->vc, &mtkd->ddev);
> > +
> > + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> > + if ((int)mtkd->dma_irq[i] < 0) {
> > + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> > + rc = -EINVAL;
> > + goto err_no_dma;
> > + }
> > + }
> > +
> > + pm_runtime_enable(&pdev->dev);
> > + pm_runtime_set_active(&pdev->dev);
> > +
> > + rc = dma_async_device_register(&mtkd->ddev);
> > + if (rc)
> > + goto rpm_disable;
> > +
> > + platform_set_drvdata(pdev, mtkd);
> > +
> > + if (pdev->dev.of_node) {
> > + /* Device-tree DMA controller registration */
> > + rc = of_dma_controller_register(pdev->dev.of_node,
> > + of_dma_xlate_by_chan_id,
> > + mtkd);
> > + if (rc)
> > + goto dma_remove;
> > + }
> > +
> > + return rc;
> > +
> > +dma_remove:
> > + dma_async_device_unregister(&mtkd->ddev);
> > +rpm_disable:
> > + pm_runtime_disable(&pdev->dev);
> > +err_no_dma:
> > + mtk_uart_apdma_free(mtkd);
> > + return rc;
> > +}
> > +

......

2019-01-10 10:06:50

by Long Cheng

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

On Wed, 2019-01-02 at 18:03 -0800, Randy Dunlap wrote:
> Hi,
>
> While you are making changes, here are a few more:
>

hi

OK, thanks for your comments.

>
> On 1/2/19 5:39 PM, Nicolas Boichat wrote:
> > diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> > index 27bac0b..1a523c87 100644
> > --- a/drivers/dma/mediatek/Kconfig
> > +++ b/drivers/dma/mediatek/Kconfig
> > @@ -1,4 +1,15 @@
> >
> > +config DMA_MTK_UART
> > + tristate "MediaTek SoCs APDMA support for UART"
> > + depends on OF && SERIAL_8250_MT6577
> > + select DMA_ENGINE
> > + select DMA_VIRTUAL_CHANNELS
> > + help
> > + Support for the UART DMA engine found on MediaTek MTK SoCs.
> > + when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
>
> When
>
> > + you can enable the config. the DMA engine can only be used
>
> The
>
> > + with MediaTek SoCs.
> > +
>

above, i will modify.

> Also, use tabs to indent instead of spaces.
> The lines (tristate, depends, select, and help) should be indented with one tab.
> The help text lines should be indented with one tab + 2 spaces.
>

in my patch, i already do this. So don't need modify.

> > config MTK_HSDMA
> > tristate "MediaTek High-Speed DMA controller support"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
>
>
> thanks,

2019-01-10 10:57:53

by Long Cheng

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

On Fri, 2019-01-04 at 22:49 +0530, Vinod Koul wrote:
> On 02-01-19, 10:12, Long Cheng wrote:
> > In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> > If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> > the performance, can enable the function.
>
> Is the DMA controller UART specific, can it work with other controllers
> as well, if so you should get rid of uart name in patch
>

I don't know that it can work or not on other controller. but it's for
MediaTek SOC

> > +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
>
> Why are the channels not coming from DT?
>

i will using dma-requests install of it.

> > +
> > +#define VFF_EN_B BIT(0)
> > +#define VFF_STOP_B BIT(0)
> > +#define VFF_FLUSH_B BIT(0)
> > +#define VFF_4G_SUPPORT_B BIT(0)
> > +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> > +#define VFF_RX_INT_EN1_B BIT(1)
> > +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
>
> space around /* space */ also run checkpatch to check for style errors
>

ok.

> > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > +{
> > + unsigned int len, send, left, wpt, d_wpt, tmp;
> > + int ret;
> > +
> > + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> > + if (!left) {
> > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > + return;
> > + }
> > +
> > + /* Wait 1sec for flush, can't sleep*/
> > + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> > + tmp != VFF_FLUSH_B, 0, 1000000);
> > + if (ret)
> > + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> > + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> > +
> > + send = min_t(unsigned int, left, c->desc->avail_len);
> > + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > +
> > + d_wpt = wpt + send;
> > + if ((d_wpt & VFF_RING_SIZE) >= len) {
> > + d_wpt = d_wpt - len;
> > + d_wpt = d_wpt ^ VFF_RING_WRAP;
> > + }
> > + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> > +
> > + c->desc->avail_len -= send;
> > +
> > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> > + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > +}
> > +
> > +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> > +{
> > + struct mtk_uart_apdma_desc *d = c->desc;
> > + unsigned int len, wg, rg, cnt;
> > +
> > + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> > + !d || !vchan_next_desc(&c->vc))
> > + return;
> > +
> > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > + rg = mtk_uart_apdma_read(c, VFF_RPT);
> > + wg = mtk_uart_apdma_read(c, VFF_WPT);
> > + if ((rg ^ wg) & VFF_RING_WRAP)
> > + cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
> > + else
> > + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> > +
> > + c->rx_status = cnt;
> > + mtk_uart_apdma_write(c, VFF_RPT, wg);
> > +
> > + list_del(&d->vd.node);
> > + vchan_cookie_complete(&d->vd);
> > +}
>
> this looks odd, why do you have different rx and tx start routines?
>

Would you like explain it in more detail? thanks.
In tx function, will wait the last data flush done. and the count the
size that send.
In Rx function, will count the size that receive.
Any way, in rx / tx, need andle WPT or RPT.

> > +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + u32 tmp;
> > + int ret;
> > +
> > + pm_runtime_get_sync(mtkd->ddev.dev);
> > +
> > + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> > + mtk_uart_apdma_write(c, VFF_THRE, 0);
> > + mtk_uart_apdma_write(c, VFF_LEN, 0);
> > + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> > +
> > + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> > + tmp == 0, 10, 100);
> > + if (ret) {
> > + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> > + return ret;
> > + }
>
> register read does reset?
>

'mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);' is reset. resd just
poll reset done.

> > +
> > + if (!c->requested) {
> > + c->requested = true;
> > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> > + KBUILD_MODNAME, chan);
>
> why is the irq not requested in driver probe?
>

I have explained in below,
http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016418.html

> > +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + enum dma_status ret;
> > + unsigned long flags;
> > +
> > + if (!txstate)
> > + return DMA_ERROR;
> > +
> > + ret = dma_cookie_status(chan, cookie, txstate);
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (ret == DMA_IN_PROGRESS) {
> > + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> > + dma_set_residue(txstate, c->rx_status);
> > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
>
> why set reside when it is complete? also reside can be null, that should
> be checked as well
>
In different status, set different reside.

> > +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> > + (struct dma_chan *chan, struct scatterlist *sgl,
> > + unsigned int sglen, enum dma_transfer_direction dir,
> > + unsigned long tx_flags, void *context)
> > +{
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + struct mtk_uart_apdma_desc *d;
> > +
> > + if ((dir != DMA_DEV_TO_MEM) &&
> > + (dir != DMA_MEM_TO_DEV)) {
> > + dev_err(chan->device->dev, "bad direction\n");
> > + return NULL;
> > + }
>
> we have a macro for this

Thanks for your suggestion. i will modify it.
>
> > +
> > + /* Now allocate and setup the descriptor */
> > + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> > + if (!d)
> > + return NULL;
> > +
> > + /* sglen is 1 */
>
> ?
>
> > +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> > + struct dma_slave_config *cfg)
> > +{
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + struct mtk_uart_apdmadev *mtkd =
> > + to_mtk_uart_apdma_dev(c->vc.chan.device);
> > +
> > + c->cfg = *cfg;
> > +
> > + if (cfg->direction == DMA_DEV_TO_MEM) {
>
> fg->direction is deprecated, in fact I have removed all users recently,
> please do not use this

You will remove 'direction' in 'struct dma_slave_config'? if remove, how
do confirm direction?

>
> > + unsigned int rx_len = cfg->src_addr_width * 1024;
> > +
> > + mtk_uart_apdma_write(c, VFF_ADDR, cfg->src_addr);
> > + mtk_uart_apdma_write(c, VFF_LEN, rx_len);
> > + mtk_uart_apdma_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > + mtk_uart_apdma_write(c, VFF_INT_EN,
> > + VFF_RX_INT_EN0_B | VFF_RX_INT_EN1_B);
> > + mtk_uart_apdma_write(c, VFF_RPT, 0);
> > + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
>
> why are we writing this here, we are supposed to do that when txn
> starts!
>

also you can review the link
http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016418.html

> > +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> > +{
> > + /* just for check caps pass */
> > + return 0;
> > +}
>
> if you do not support this please remove this!
>
> > +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> > +{
> > + while (list_empty(&mtkd->ddev.channels) == 0) {
> > + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> > + struct mtk_chan, vc.chan.device_node);
> > +
> > + list_del(&c->vc.chan.device_node);
> > + tasklet_kill(&c->vc.task);
> > + }
> > +}
> > +
> > +static const struct of_device_id mtk_uart_apdma_match[] = {
> > + { .compatible = "mediatek,mt6577-uart-dma", },
>
> where is the binding document for ediatek,mt6577-uart-dma ??
>

http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016154.html

2019-01-11 01:22:06

by Long Cheng

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

On Thu, 2019-01-10 at 18:33 +0800, Long Cheng wrote:
fix spell error

> On Fri, 2019-01-04 at 22:49 +0530, Vinod Koul wrote:
> > On 02-01-19, 10:12, Long Cheng wrote:
> > > In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> > > If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> > > the performance, can enable the function.
> >
> > Is the DMA controller UART specific, can it work with other controllers
> > as well, if so you should get rid of uart name in patch
> >
>
> I don't know that it can work or not on other controller. but it's for
> MediaTek SOC
>
> > > +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> >
> > Why are the channels not coming from DT?
> >
>
> i will using dma-requests install of it.
>
i will using 'dma-requests' instead of it.

> > > +
> > > +#define VFF_EN_B BIT(0)
> > > +#define VFF_STOP_B BIT(0)
> > > +#define VFF_FLUSH_B BIT(0)
> > > +#define VFF_4G_SUPPORT_B BIT(0)
> > > +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> > > +#define VFF_RX_INT_EN1_B BIT(1)
> > > +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> >
> > space around /* space */ also run checkpatch to check for style errors
> >
>
> ok.
>
> > > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > > +{
> > > + unsigned int len, send, left, wpt, d_wpt, tmp;
> > > + int ret;
> > > +
> > > + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> > > + if (!left) {
> > > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > > + return;
> > > + }
> > > +
> > > + /* Wait 1sec for flush, can't sleep*/
> > > + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> > > + tmp != VFF_FLUSH_B, 0, 1000000);
> > > + if (ret)
> > > + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> > > + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> > > +
> > > + send = min_t(unsigned int, left, c->desc->avail_len);
> > > + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> > > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > > +
> > > + d_wpt = wpt + send;
> > > + if ((d_wpt & VFF_RING_SIZE) >= len) {
> > > + d_wpt = d_wpt - len;
> > > + d_wpt = d_wpt ^ VFF_RING_WRAP;
> > > + }
> > > + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> > > +
> > > + c->desc->avail_len -= send;
> > > +
> > > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > > + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> > > + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > > +}
> > > +
> > > +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> > > +{
> > > + struct mtk_uart_apdma_desc *d = c->desc;
> > > + unsigned int len, wg, rg, cnt;
> > > +
> > > + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> > > + !d || !vchan_next_desc(&c->vc))
> > > + return;
> > > +
> > > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > > + rg = mtk_uart_apdma_read(c, VFF_RPT);
> > > + wg = mtk_uart_apdma_read(c, VFF_WPT);
> > > + if ((rg ^ wg) & VFF_RING_WRAP)
> > > + cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
> > > + else
> > > + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> > > +
> > > + c->rx_status = cnt;
> > > + mtk_uart_apdma_write(c, VFF_RPT, wg);
> > > +
> > > + list_del(&d->vd.node);
> > > + vchan_cookie_complete(&d->vd);
> > > +}
> >
> > this looks odd, why do you have different rx and tx start routines?
> >
>
> Would you like explain it in more detail? thanks.
> In tx function, will wait the last data flush done. and the count the
> size that send.
> In Rx function, will count the size that receive.
> Any way, in rx / tx, need andle WPT or RPT.
>
Any way, in rx / tx, need handle WPT or RPT.

> > > +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> > > +{
> > > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + u32 tmp;
> > > + int ret;
> > > +
> > > + pm_runtime_get_sync(mtkd->ddev.dev);
> > > +
> > > + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> > > + mtk_uart_apdma_write(c, VFF_THRE, 0);
> > > + mtk_uart_apdma_write(c, VFF_LEN, 0);
> > > + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> > > +
> > > + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> > > + tmp == 0, 10, 100);
> > > + if (ret) {
> > > + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> > > + return ret;
> > > + }
> >
> > register read does reset?
> >
>
> 'mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);' is reset. resd just
> poll reset done.
>
> > > +
> > > + if (!c->requested) {
> > > + c->requested = true;
> > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> > > + KBUILD_MODNAME, chan);
> >
> > why is the irq not requested in driver probe?
> >
>
> I have explained in below,
> http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016418.html
>
> > > +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> > > + dma_cookie_t cookie,
> > > + struct dma_tx_state *txstate)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + enum dma_status ret;
> > > + unsigned long flags;
> > > +
> > > + if (!txstate)
> > > + return DMA_ERROR;
> > > +
> > > + ret = dma_cookie_status(chan, cookie, txstate);
> > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > + if (ret == DMA_IN_PROGRESS) {
> > > + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> > > + dma_set_residue(txstate, c->rx_status);
> > > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> >
> > why set reside when it is complete? also reside can be null, that should
> > be checked as well
> >
> In different status, set different reside.
>
> > > +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> > > + (struct dma_chan *chan, struct scatterlist *sgl,
> > > + unsigned int sglen, enum dma_transfer_direction dir,
> > > + unsigned long tx_flags, void *context)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + struct mtk_uart_apdma_desc *d;
> > > +
> > > + if ((dir != DMA_DEV_TO_MEM) &&
> > > + (dir != DMA_MEM_TO_DEV)) {
> > > + dev_err(chan->device->dev, "bad direction\n");
> > > + return NULL;
> > > + }
> >
> > we have a macro for this
>
> Thanks for your suggestion. i will modify it.
> >
> > > +
> > > + /* Now allocate and setup the descriptor */
> > > + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> > > + if (!d)
> > > + return NULL;
> > > +
> > > + /* sglen is 1 */
> >
> > ?
> >
> > > +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> > > + struct dma_slave_config *cfg)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + struct mtk_uart_apdmadev *mtkd =
> > > + to_mtk_uart_apdma_dev(c->vc.chan.device);
> > > +
> > > + c->cfg = *cfg;
> > > +
> > > + if (cfg->direction == DMA_DEV_TO_MEM) {
> >
> > fg->direction is deprecated, in fact I have removed all users recently,
> > please do not use this
>
> You will remove 'direction' in 'struct dma_slave_config'? if remove, how
> do confirm direction?
>
> >
> > > + unsigned int rx_len = cfg->src_addr_width * 1024;
> > > +
> > > + mtk_uart_apdma_write(c, VFF_ADDR, cfg->src_addr);
> > > + mtk_uart_apdma_write(c, VFF_LEN, rx_len);
> > > + mtk_uart_apdma_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > > + mtk_uart_apdma_write(c, VFF_INT_EN,
> > > + VFF_RX_INT_EN0_B | VFF_RX_INT_EN1_B);
> > > + mtk_uart_apdma_write(c, VFF_RPT, 0);
> > > + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > + mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
> >
> > why are we writing this here, we are supposed to do that when txn
> > starts!
> >
>
> also you can review the link
> http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016418.html
>
> > > +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> > > +{
> > > + /* just for check caps pass */
> > > + return 0;
> > > +}
> >
> > if you do not support this please remove this!
> >
> > > +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> > > +{
> > > + while (list_empty(&mtkd->ddev.channels) == 0) {
> > > + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> > > + struct mtk_chan, vc.chan.device_node);
> > > +
> > > + list_del(&c->vc.chan.device_node);
> > > + tasklet_kill(&c->vc.task);
> > > + }
> > > +}
> > > +
> > > +static const struct of_device_id mtk_uart_apdma_match[] = {
> > > + { .compatible = "mediatek,mt6577-uart-dma", },
> >
> > where is the binding document for ediatek,mt6577-uart-dma ??
> >
>
> http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016154.html
>

2019-01-20 05:31:55

by Vinod Koul

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

On 10-01-19, 18:33, Long Cheng wrote:
> On Fri, 2019-01-04 at 22:49 +0530, Vinod Koul wrote:
> > On 02-01-19, 10:12, Long Cheng wrote:
> > > In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> > > If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> > > the performance, can enable the function.
> >
> > Is the DMA controller UART specific, can it work with other controllers
> > as well, if so you should get rid of uart name in patch
>
> I don't know that it can work or not on other controller. but it's for
> MediaTek SOC

What I meant was that if can work with other controllers (users) apart
from UART, how about say audio, spi etc!!

>
> > > +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> >
> > Why are the channels not coming from DT?
> >
>
> i will using dma-requests install of it.
>
> > > +
> > > +#define VFF_EN_B BIT(0)
> > > +#define VFF_STOP_B BIT(0)
> > > +#define VFF_FLUSH_B BIT(0)
> > > +#define VFF_4G_SUPPORT_B BIT(0)
> > > +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> > > +#define VFF_RX_INT_EN1_B BIT(1)
> > > +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> >
> > space around /* space */ also run checkpatch to check for style errors
> >
>
> ok.
>
> > > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > > +{
> > > + unsigned int len, send, left, wpt, d_wpt, tmp;
> > > + int ret;
> > > +
> > > + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> > > + if (!left) {
> > > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > > + return;
> > > + }
> > > +
> > > + /* Wait 1sec for flush, can't sleep*/
> > > + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> > > + tmp != VFF_FLUSH_B, 0, 1000000);
> > > + if (ret)
> > > + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> > > + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> > > +
> > > + send = min_t(unsigned int, left, c->desc->avail_len);
> > > + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> > > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > > +
> > > + d_wpt = wpt + send;
> > > + if ((d_wpt & VFF_RING_SIZE) >= len) {
> > > + d_wpt = d_wpt - len;
> > > + d_wpt = d_wpt ^ VFF_RING_WRAP;
> > > + }
> > > + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> > > +
> > > + c->desc->avail_len -= send;
> > > +
> > > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > > + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> > > + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > > +}
> > > +
> > > +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> > > +{
> > > + struct mtk_uart_apdma_desc *d = c->desc;
> > > + unsigned int len, wg, rg, cnt;
> > > +
> > > + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> > > + !d || !vchan_next_desc(&c->vc))
> > > + return;
> > > +
> > > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > > + rg = mtk_uart_apdma_read(c, VFF_RPT);
> > > + wg = mtk_uart_apdma_read(c, VFF_WPT);
> > > + if ((rg ^ wg) & VFF_RING_WRAP)
> > > + cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
> > > + else
> > > + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> > > +
> > > + c->rx_status = cnt;
> > > + mtk_uart_apdma_write(c, VFF_RPT, wg);
> > > +
> > > + list_del(&d->vd.node);
> > > + vchan_cookie_complete(&d->vd);
> > > +}
> >
> > this looks odd, why do you have different rx and tx start routines?
> >
>
> Would you like explain it in more detail? thanks.
> In tx function, will wait the last data flush done. and the count the
> size that send.
> In Rx function, will count the size that receive.
> Any way, in rx / tx, need andle WPT or RPT.
>
> > > +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> > > +{
> > > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + u32 tmp;
> > > + int ret;
> > > +
> > > + pm_runtime_get_sync(mtkd->ddev.dev);
> > > +
> > > + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> > > + mtk_uart_apdma_write(c, VFF_THRE, 0);
> > > + mtk_uart_apdma_write(c, VFF_LEN, 0);
> > > + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> > > +
> > > + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> > > + tmp == 0, 10, 100);
> > > + if (ret) {
> > > + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> > > + return ret;
> > > + }
> >
> > register read does reset?
> >
>
> 'mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);' is reset. resd just
> poll reset done.
>
> > > +
> > > + if (!c->requested) {
> > > + c->requested = true;
> > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> > > + KBUILD_MODNAME, chan);
> >
> > why is the irq not requested in driver probe?
> >
>
> I have explained in below,
> http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016418.html
>
> > > +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> > > + dma_cookie_t cookie,
> > > + struct dma_tx_state *txstate)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + enum dma_status ret;
> > > + unsigned long flags;
> > > +
> > > + if (!txstate)
> > > + return DMA_ERROR;
> > > +
> > > + ret = dma_cookie_status(chan, cookie, txstate);
> > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > + if (ret == DMA_IN_PROGRESS) {
> > > + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> > > + dma_set_residue(txstate, c->rx_status);
> > > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> >
> > why set reside when it is complete? also reside can be null, that should
> > be checked as well
> >
> In different status, set different reside.

Can you explain that..

>
> > > +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> > > + (struct dma_chan *chan, struct scatterlist *sgl,
> > > + unsigned int sglen, enum dma_transfer_direction dir,
> > > + unsigned long tx_flags, void *context)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + struct mtk_uart_apdma_desc *d;
> > > +
> > > + if ((dir != DMA_DEV_TO_MEM) &&
> > > + (dir != DMA_MEM_TO_DEV)) {
> > > + dev_err(chan->device->dev, "bad direction\n");
> > > + return NULL;
> > > + }
> >
> > we have a macro for this
>
> Thanks for your suggestion. i will modify it.
> >
> > > +
> > > + /* Now allocate and setup the descriptor */
> > > + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> > > + if (!d)
> > > + return NULL;
> > > +
> > > + /* sglen is 1 */
> >
> > ?
> >
> > > +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> > > + struct dma_slave_config *cfg)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + struct mtk_uart_apdmadev *mtkd =
> > > + to_mtk_uart_apdma_dev(c->vc.chan.device);
> > > +
> > > + c->cfg = *cfg;
> > > +
> > > + if (cfg->direction == DMA_DEV_TO_MEM) {
> >
> > fg->direction is deprecated, in fact I have removed all users recently,
> > please do not use this
>
> You will remove 'direction' in 'struct dma_slave_config'? if remove, how
> do confirm direction?

Yes please use the direction argument in prep_xx calls
--
~Vinod

2019-01-21 08:14:06

by Long Cheng

Subject: Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support

On Sun, 2019-01-20 at 10:57 +0530, Vinod Koul wrote:
> On 10-01-19, 18:33, Long Cheng wrote:
> > On Fri, 2019-01-04 at 22:49 +0530, Vinod Koul wrote:
> > > On 02-01-19, 10:12, Long Cheng wrote:
> > > > In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> > > > If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> > > > the performance, can enable the function.
> > >
> > > Is the DMA controller UART specific, can it work with other controllers
> > > as well, if so you should get rid of uart name in patch
> >
> > I don't know that it can work or not on other controller. but it's for
> > MediaTek SOC
>
> What I meant was that if can work with other controllers (users) apart
> from UART, how about say audio, spi etc!!
>

it's just for UART APDMA. can't work on spi ...

> >
> > > > +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> > >
> > > Why are the channels not coming from DT?
> > >
> >
> > i will using dma-requests install of it.
> >
> > > > +
> > > > +#define VFF_EN_B BIT(0)
> > > > +#define VFF_STOP_B BIT(0)
> > > > +#define VFF_FLUSH_B BIT(0)
> > > > +#define VFF_4G_SUPPORT_B BIT(0)
> > > > +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> > > > +#define VFF_RX_INT_EN1_B BIT(1)
> > > > +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> > >
> > > space around /* space */ also run checkpatch to check for style errors
> > >
> >
> > ok.
> >
> > > > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > > > +{
> > > > + unsigned int len, send, left, wpt, d_wpt, tmp;
> > > > + int ret;
> > > > +
> > > > + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> > > > + if (!left) {
> > > > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > > > + return;
> > > > + }
> > > > +
> > > > + /* Wait 1sec for flush, can't sleep*/
> > > > + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> > > > + tmp != VFF_FLUSH_B, 0, 1000000);
> > > > + if (ret)
> > > > + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> > > > + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> > > > +
> > > > + send = min_t(unsigned int, left, c->desc->avail_len);
> > > > + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> > > > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > > > +
> > > > + d_wpt = wpt + send;
> > > > + if ((d_wpt & VFF_RING_SIZE) >= len) {
> > > > + d_wpt = d_wpt - len;
> > > > + d_wpt = d_wpt ^ VFF_RING_WRAP;
> > > > + }
> > > > + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> > > > +
> > > > + c->desc->avail_len -= send;
> > > > +
> > > > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > > > + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> > > > + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > > > +}
> > > > +
> > > > +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> > > > +{
> > > > + struct mtk_uart_apdma_desc *d = c->desc;
> > > > + unsigned int len, wg, rg, cnt;
> > > > +
> > > > + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> > > > + !d || !vchan_next_desc(&c->vc))
> > > > + return;
> > > > +
> > > > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > > > + rg = mtk_uart_apdma_read(c, VFF_RPT);
> > > > + wg = mtk_uart_apdma_read(c, VFF_WPT);
> > > > + if ((rg ^ wg) & VFF_RING_WRAP)
> > > > + cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
> > > > + else
> > > > + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> > > > +
> > > > + c->rx_status = cnt;
> > > > + mtk_uart_apdma_write(c, VFF_RPT, wg);
> > > > +
> > > > + list_del(&d->vd.node);
> > > > + vchan_cookie_complete(&d->vd);
> > > > +}
> > >
> > > this looks odd, why do you have different rx and tx start routines?
> > >
> >
> > Would you like explain it in more detail? thanks.
> > In tx function, will wait the last data flush done. and the count the
> > size that send.
> > In Rx function, will count the size that receive.
> > Any way, in rx / tx, need andle WPT or RPT.
> >
> > > > +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> > > > +{
> > > > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + u32 tmp;
> > > > + int ret;
> > > > +
> > > > + pm_runtime_get_sync(mtkd->ddev.dev);
> > > > +
> > > > + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> > > > + mtk_uart_apdma_write(c, VFF_THRE, 0);
> > > > + mtk_uart_apdma_write(c, VFF_LEN, 0);
> > > > + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> > > > +
> > > > + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> > > > + tmp == 0, 10, 100);
> > > > + if (ret) {
> > > > + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> > > > + return ret;
> > > > + }
> > >
> > > register read does reset?
> > >
> >
> > 'mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);' is reset. resd just
> > poll reset done.
> >
> > > > +
> > > > + if (!c->requested) {
> > > > + c->requested = true;
> > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> > > > + KBUILD_MODNAME, chan);
> > >
> > > why is the irq not requested in driver probe?
> > >
> >
> > I have explained in below,
> > http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016418.html
> >
> > > > +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> > > > + dma_cookie_t cookie,
> > > > + struct dma_tx_state *txstate)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + enum dma_status ret;
> > > > + unsigned long flags;
> > > > +
> > > > + if (!txstate)
> > > > + return DMA_ERROR;
> > > > +
> > > > + ret = dma_cookie_status(chan, cookie, txstate);
> > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > + if (ret == DMA_IN_PROGRESS) {
> > > > + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> > > > + dma_set_residue(txstate, c->rx_status);
> > > > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> > >
> > > why set reside when it is complete? also reside can be null, that should
> > > be checked as well
> > >
> > In different status, set different reside.
>
> Can you explain that..
>

RPT is different every time. When RX interrupt coming, update the
rx_status and notify uart that there have data. One transfer is
complete. Uart will call tx_status to get the length at the time of
interruption. Other case, if want to get the tx_status, directly read
register.

> >
> > > > +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> > > > + (struct dma_chan *chan, struct scatterlist *sgl,
> > > > + unsigned int sglen, enum dma_transfer_direction dir,
> > > > + unsigned long tx_flags, void *context)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + struct mtk_uart_apdma_desc *d;
> > > > +
> > > > + if ((dir != DMA_DEV_TO_MEM) &&
> > > > + (dir != DMA_MEM_TO_DEV)) {
> > > > + dev_err(chan->device->dev, "bad direction\n");
> > > > + return NULL;
> > > > + }
> > >
> > > we have a macro for this
> >
> > Thanks for your suggestion. i will modify it.
> > >
> > > > +
> > > > + /* Now allocate and setup the descriptor */
> > > > + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> > > > + if (!d)
> > > > + return NULL;
> > > > +
> > > > + /* sglen is 1 */
> > >
> > > ?

dmaengine_prep_slave_single is called. the parameter 'sglen' is 1.
Add annotation to indicate that the total length is sg_dma_len(sgl), no
more.

> > >
> > > > +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> > > > + struct dma_slave_config *cfg)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + struct mtk_uart_apdmadev *mtkd =
> > > > + to_mtk_uart_apdma_dev(c->vc.chan.device);
> > > > +
> > > > + c->cfg = *cfg;
> > > > +
> > > > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > >
> > > fg->direction is deprecated, in fact I have removed all users recently,
> > > please do not use this
> >
> > You will remove 'direction' in 'struct dma_slave_config'? if remove, how
> > do confirm direction?
>
> Yes please use the direction argument in prep_xx calls

I had modified on v10. thanks.