dmaengine: mtk-hsdma: add support for Mediatek High-Speed DMA controller on MT7623 SoC
Subsystems (3)
v2 cover dmaengine: mtk-hsdma: add support for Mediatek High-Speed DMA controller on MT7623 SoC
From: Sean Wang <[email protected]>
Changes since v1:
- fix typo in the commit message.
- delete status shown in the dt-binding example.
This patchset introduces support for Mediatek High-Speed DMA controller (MTK-HSDMA) Currently, the driver is already tested successfully with dmatest module on MT7623 SoC.
MTK-HSDMA on MT7623 SoC has the single channel which is dedicated to memory-to-memory transfer through ring-based descriptor management. Even though there is only one physical channel available inside HSDMA, the driver is extended to the support for multiple virtual channels processing simultaneously in round-round way by means of the feature DMA_VIRTUAL_CHANNELS.
Sean Wang (2):
dt-bindings: dmaengine: Add Mediatek High-Speed DMA controller
bindings
dmaengine: mtk-hsdma: Add Mediatek High-Speed DMA controller on MT7623
SoC
.../devicetree/bindings/dma/mtk-hsdma.txt | 29 + drivers/dma/Kconfig | 14 + drivers/dma/Makefile | 1 + drivers/dma/mtk-hsdma.c | 890 +++++++++++++++++++++ 4 files changed, 934 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/mtk-hsdma.txt create mode 100644 drivers/dma/mtk-hsdma.c
Signature · 1 line
2.7.4
PATCH v2 1/2 dt-bindings: dmaengine: Add Mediatek High-Speed DMA controller bindings
From: Sean Wang <[email protected]>
Document the devicetree bindings for Mediatek High-Speed DMA controller which could be found on MT7623 SoC or other similar Mediatek SoCs.
--- .../devicetree/bindings/dma/mtk-hsdma.txt | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/mtk-hsdma.txt
Documentation/devicetree/bindings/dma/mtk-hsdma.txt new file +29 -0
@@ -0,0 +1,29 @@ +Mediatek High-Speed DMA Controller +================================== + +This driver follows the generic DMA bindings defined in dma/dma.txt. + +Required properties: + +- compatible: Must be one of + "mediatek,mt7623-hsdma". +- reg: Should contain the register's base address and length. +- interrupts: Should contain a reference to the interrupt used by this + device. +- clocks: Should be the clock specifiers corresponding to the entry in + clock-names property. +- clock-names: Should contain "hsdma" entries. +- #dma-cells: The length of the DMA specifier, must be <1>. This one cell + in dmas property of a client device represents the channel + number. +Example: + + hsdma: hsdma at 1b007000 { + compatible = "mediatek,mt7623-hsdma"; + reg = <0 0x1b007000 0 0x1000>; + interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_LOW>; + clocks = <ðsys CLK_ETHSYS_HSDMA>; + clock-names = "hsdma"; + }; + +DMA clients must use the format described in dma/dma.txt file.
Signature · 1 line
2.7.4
PATCH v2 2/2 dmaengine: mtk-hsdma: Add Mediatek High-Speed DMA controller on MT7623 SoC
In reply to [email protected]From: Sean Wang <[email protected]>
Add dmaengine driver for Mediatek High-Speed DMA based on the feature DMA_VIRTUAL_CHANNELS.
Mediatek High-Speed DMA controller (HSDMA) on MT7623 SoC has the single channel which is dedicated to memory-to-memory transfer through ring-based descriptor management. Even though there is only one physical channel available inside HSDMA, the driver is extended to the support for multiple virtual channels processing simultaneously in round-robin way by means of the feature DMA_VIRTUAL_CHANNELS.
--- drivers/dma/Kconfig | 14 + drivers/dma/Makefile | 1 + drivers/dma/mtk-hsdma.c | 890 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 905 insertions(+) create mode 100644 drivers/dma/mtk-hsdma.c
drivers/dma/Kconfig +14 -0
@@ -329,6 +329,20 @@ config MPC512X_DMA ---help--- Enable support for the Freescale MPC512x built-in DMA engine. +config MTK_HSDMA + tristate "Mediatek High-Speed DMA controller support" + depends on ARCH_MEDIATEK || COMPILE_TEST + select DMA_ENGINE + select DMA_VIRTUAL_CHANNELS + ---help--- + Enable support for High-Speed DMA controller on Mediatek + SoCs. + + This controller provides the channels which is dedicated to + memory-to-memory transfer to offload from CPU through ring- + based descriptor management which could be found on MT7623 + platform. + config MV_XOR bool "Marvell XOR engine support" depends on PLAT_ORION || ARCH_MVEBU || COMPILE_TEST
drivers/dma/Makefile +1 -0
@@ -68,6 +68,7 @@ obj-$(CONFIG_TI_EDMA) += edma.o obj-$(CONFIG_XGENE_DMA) += xgene-dma.o obj-$(CONFIG_ZX_DMA) += zx_dma.o obj-$(CONFIG_ST_FDMA) += st_fdma.o +obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o obj-y += qcom/ obj-y += xilinx/
drivers/dma/mtk-hsdma.c new file +890 -0
890 lines not on this page · raw message
Signature · 1 line
2.7.4
Re: [PATCH v2 2/2] dmaengine: mtk-hsdma: Add Mediatek High-Speed DMA controller on MT7623 SoC
drivers/dma/mtk-hsdma.c line 57
Quoted 7 lines · On Thu, May 25, 2017 at 03:12:49PM +0800, [email protected] wrote:
drivers/dma/mtk-hsdma.c new file +7
-0+/* MTK_DMA_SIZE must be 2 of power and 4 for the minimal */ +#define MTK_DMA_SIZE 256 +#define MTK_HSDMA_NEXT_DESP_IDX (x, y) (((x) + 1) & ((y) - 1)) +#define MTK_HSDMA_PREV_DESP_IDX (x, y) (((x) - 1) & ((y) - 1)) +#define MTK_HSDMA_MAX_LEN 0x3f80 +#define MTK_HSDMA_ALIGN_SIZE 4 +#define MTK_HSDMA_TIMEOUT HZ
Why this unused define?
drivers/dma/mtk-hsdma.c line 167
Quoted 6 lines
drivers/dma/mtk-hsdma.c new file +6
-0+struct mtk_hsdma_device { +struct dma_device ddev; +void __iomem *base; +struct clk *clk; +u32 irq; +bool busy;
what do you mean by device busy, its usually a channel which is..
drivers/dma/mtk-hsdma.c line 264
Quoted 11 lines
drivers/dma/mtk-hsdma.c new file +11
-0+static int mtk_hsdma_alloc_pchan (struct mtk_hsdma_device *hsdma, +struct mtk_hsdma_pchan *pc) +{ +int i, ret; +struct mtk_hsdma_ring *ring = &pc->ring; + +dev_dbg (hsdma2dev (hsdma),"Allocating pchannel\n" ); + +memset (pc,0 ,sizeof (*pc)); + pc->hsdma = hsdma; +atomic_set (&pc->free_count,MTK_DMA_SIZE -1 );
why do you need atomic variables?
drivers/dma/mtk-hsdma.c line 359
Quoted 8 lines
drivers/dma/mtk-hsdma.c new file +8
-0+static int mtk_hsdma_alloc_chan_resources (struct dma_chan *c) +{ +struct mtk_hsdma_device *hsdma =to_hsdma_dev (c); +struct mtk_hsdma_vchan *vc =to_hsdma_vchan (c); +int ret =0 ; + +if (!atomic_read (&hsdma->pc_refcnt)) + ret =mtk_hsdma_alloc_pchan (hsdma, &hsdma->pc);
can you please explain this..?
Quoted 15 lines
drivers/dma/mtk-hsdma.c new file +15
-0+static int mtk_hsdma_consume_one_vdesc (struct mtk_hsdma_pchan *pc, +struct mtk_hsdma_vdesc *hvd) +{ +struct mtk_hsdma_device *hsdma = pc->hsdma; +struct mtk_hsdma_ring *ring = &pc->ring; +struct mtk_hsdma_pdesc *txd, *rxd; +u32 i, tlen; +u16 maxfills, prev, old_ptr, handled; + + maxfills =min_t (u32, hvd->num_sgs,atomic_read (&pc->free_count)); +if (!maxfills) +return -ENOSPC ; + + hsdma->busy = true; + old_ptr = ring->cur_tptr;
drivers/dma/mtk-hsdma.c line 408
Quoted 10 lines
drivers/dma/mtk-hsdma.c new file +10
-0+for (i =0 ; i < maxfills ; i++) { + tlen = (hvd->len >MTK_HSDMA_MAX_LEN ) ? +MTK_HSDMA_MAX_LEN : hvd->len; + txd = &ring->txd[ring->cur_tptr]; +WRITE_ONCE (txd->des1, hvd->src); +WRITE_ONCE (txd->des2, +MTK_HSDMA_DESC_LS0 |MTK_HSDMA_DESC_PLEN (tlen)); + rxd = &ring->rxd[ring->cur_tptr]; +WRITE_ONCE (rxd->des1, hvd->dest); +WRITE_ONCE (rxd->des2,MTK_HSDMA_DESC_PLEN (tlen));
interesting usage of WRITE_ONCE, can you explain why it is used?
Quoted 23 lines
drivers/dma/mtk-hsdma.c new file +23
-0+static void mtk_hsdma_schedule (unsigned long data) +{ +struct mtk_hsdma_device *hsdma = (struct mtk_hsdma_device *)data; +struct mtk_hsdma_vchan *vc; +struct virt_dma_desc *vd; +bool vc_removed; + + vc =mtk_hsdma_pick_vchan (hsdma); +if (!vc) +return ; + +if (!vc->vd_uncompleted) { +spin_lock (&vc->vc.lock); + vd =vchan_next_desc (&vc->vc); +spin_unlock (&vc->vc.lock); + }else { + vd = vc->vd_uncompleted; +atomic_dec (&vc->refcnt); + } + + hsdma->vc_uncompleted =0 ; + vc->vd_uncompleted =0 ; +
drivers/dma/mtk-hsdma.c line 535
Quoted 10 lines
drivers/dma/mtk-hsdma.c new file +10
-0+while (vc && vd) { +spin_lock (&hsdma->lock); + vc_removed =list_empty (&vc->node); +/* +* Refcnt increases for the indication that one more descriptor +* is ready for the process if the corresponding channel is +* active. +*/ +if (!vc_removed) +atomic_inc (&vc->refcnt);
okay now I understand a little bit why these are used, but the question is why.. We can have a simpler implementation using active, pending link lists like other drivers do.. And since you use virt_dma_chan which already keeps track of allocated, submitted, issued and completed descriptors not sure why we need refcount here, can you explain this?
Quoted 1 line
+static void
mtk_hsdma_housekeeping(unsigned long data)
care to explain the purpose of this 'housekeeping' takslet?
Quoted 58 lines
drivers/dma/mtk-hsdma.c new file +58
-0+{ +struct mtk_hsdma_device *hsdma = (struct mtk_hsdma_device *)data; +struct mtk_hsdma_vchan *hvc; +struct mtk_hsdma_pchan *pc; +struct mtk_hsdma_pdesc *rxd; +struct mtk_hsdma_cb *cb; +struct virt_dma_chan *vc; +struct virt_dma_desc *vd, *tmp; +u16 next; +u32 status; +LIST_HEAD (comp); + + pc = &hsdma->pc; + + status =mtk_dma_read (hsdma,MTK_HSDMA_INT_STATUS ); +mtk_dma_write (hsdma,MTK_HSDMA_INT_STATUS , status); + +while (1 ) { + next =MTK_HSDMA_NEXT_DESP_IDX (pc->ring.cur_rptr, +MTK_DMA_SIZE ); + rxd = &pc->ring.rxd[next]; + cb = &pc->ring.cb[next]; + +/* +* If no MTK_HSDMA_DESC_DDONE is specified in rxd->des2, that +* means 1) the hardware doesn't finish the data moving yet +* for the corresponding descriptor or 2) the hardware meets +* the end of data moved. +*/ +if (!(rxd->des2 &MTK_HSDMA_DESC_DDONE )) +break ; + +if (IS_VDESC_FINISHED (cb->flags)) +list_add_tail (&cb->vd->node, &comp); + +WRITE_ONCE (rxd->des1,0 ); +WRITE_ONCE (rxd->des2,0 ); + cb->flags =0 ; + pc->ring.cur_rptr = next; +atomic_inc (&pc->free_count); + } + +/* +* Ensure all changes to all the descriptors in ring space being +* flushed before we continue. +*/ +wmb (); +mtk_dma_write (hsdma,MTK_HSDMA_RX_CPU , pc->ring.cur_rptr); +mtk_dma_set (hsdma,MTK_HSDMA_INT_ENABLE ,MTK_HSDMA_INT_RXDONE ); + +list_for_each_entry_safe (vd, tmp, &comp, node) { + vc =to_virt_chan (vd->tx.chan); +spin_lock (&vc->lock); +vchan_cookie_complete (vd); +spin_unlock (&vc->lock); + + hvc =to_hsdma_vchan (vd->tx.chan); +atomic_dec (&hvc->refcnt);
drivers/dma/mtk-hsdma.c line 624
Quoted 10 lines
drivers/dma/mtk-hsdma.c new file +10
-0+ } + +/* +* An indication to HSDMA as not busy allows the user context to start +* the next HSDMA scheduler. +*/ +if (atomic_read (&pc->free_count) ==MTK_DMA_SIZE -1 ) + hsdma->busy = false; + +tasklet_schedule (&hsdma->scheduler);
and we schedule another, why?
And this would be on top of virt chan tasklet... your latencies should be off the charts
drivers/dma/mtk-hsdma.c line 637
Quoted 13 lines
drivers/dma/mtk-hsdma.c new file +13
-0+} + +static irqreturn_t mtk_hsdma_chan_irq (int irq,void *devid) +{ +struct mtk_hsdma_device *hsdma = devid; + +tasklet_schedule (&hsdma->housekeeping); + +/* Interrupt is enabled until the housekeeping tasklet is completed */ +mtk_dma_clr (hsdma,MTK_HSDMA_INT_ENABLE , +MTK_HSDMA_INT_RXDONE ); + +return IRQ_HANDLED ;
this is bad design, we want to complete the current txn and submit new one here, this is DMAengine and people want it to be done as fast as possible.
Quoted 9 lines
drivers/dma/mtk-hsdma.c new file +9
-0+} + +static void mtk_hsdma_issue_pending (struct dma_chan *c) +{ +struct mtk_hsdma_device *hsdma =to_hsdma_dev (c); +struct mtk_hsdma_vchan *vc =to_hsdma_vchan (c); +bool issued; + +spin_lock_bh (&vc->vc.lock);
drivers/dma/mtk-hsdma.c line 656
Quoted 10 lines
drivers/dma/mtk-hsdma.c new file +10
-0+ issued =vchan_issue_pending (&vc->vc); +spin_unlock_bh (&vc->vc.lock); + +spin_lock_bh (&hsdma->lock); +if (list_empty (&vc->node)) +list_add_tail (&vc->node, &hsdma->vc_pending); +spin_unlock_bh (&hsdma->lock); + +if (issued && !hsdma->busy) +tasklet_schedule (&hsdma->scheduler);
another tasklet, we are supposed to start the txn _now_
drivers/dma/mtk-hsdma.c line 871
Quoted 9 lines
drivers/dma/mtk-hsdma.c new file +9
-0+static int mtk_dma_remove (struct platform_device *pdev) +{ +struct mtk_hsdma_device *hsdma =platform_get_drvdata (pdev); + +of_dma_controller_free (pdev->dev.of_node); +dma_async_device_unregister (&hsdma->ddev); + +tasklet_kill (&hsdma->scheduler); +tasklet_kill (&hsdma->housekeeping);
you need to kill vc task as well
you still have a dangling irq which can fire tasklets after you have killed those
Signature · 1 line
~Vinod