On Sun, 2019-01-20 at 10:57 +0530, Vinod Koul wrote:

On 10-01-19, 18:33, Long Cheng wrote:

quoted

On Fri, 2019-01-04 at 22:49 +0530, Vinod Koul wrote:

quoted

On 02-01-19, 10:12, Long Cheng wrote:

quoted

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 ...

quoted

quoted

quoted

+#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.

quoted

quoted

+
+#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.

quoted

quoted

+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.

quoted

quoted

+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.

quoted

quoted

+
+	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

quoted

quoted

+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.

quoted

quoted

quoted

+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.

quoted

quoted

+
+	/* 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.

quoted

quoted

quoted

+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.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel