Re: [PATCH 2/2] dmaengine: mediatek: Add MediaTek Command-Queue DMA controller for MT6765 SoC

[Shun-Chih.Yu <shun-chih.yu@mediatek.com>]

On Wed, 2019-01-02 at 12:17 -0800, Sean Wang wrote:
> go on other parts not finished review at the last time
> 
> On Sat, Dec 29, 2018 at 3:03 AM Sean Wang <sean.wang@kernel.org> wrote:
> >
> > The version looks like better than the earlier version, but there are
> > still a few nitpicks I post at the inline.
> >
> > On Thu, Dec 27, 2018 at 5:11 AM <shun-chih.yu@mediatek.com> wrote:
> > >
> > > From: Shun-Chih Yu <shun-chih.yu@mediatek.com>
> > >
> > > MediaTek Command-Queue DMA controller (CQDMA) on MT6765 SoC is dedicated
> > > to memory-to-memory transfer through queue based descriptor management.
> > >
> > > There are only 3 physical channels inside CQDMA, while the driver is
> > > extended to support 32 virtual channels for multiple dma users to issue
> > > dma requests onto the CQDMA simultaneously.
> > >
> > > Change-Id: I1e8d116c5ecbbc49190ffc925cb59a0d035d886b
> >
> > should be more careful drop the change-id every time
Sure, I would be more careful about this.
> >
> > > Signed-off-by: Shun-Chih Yu <shun-chih.yu@mediatek.com>
> > > Reviewed-by: Vinod Koul <vkoul@kernel.org>
> > >
> > > ---
> > >  drivers/dma/mediatek/Kconfig     |   12 +
> > >  drivers/dma/mediatek/Makefile    |    1 +
> > >  drivers/dma/mediatek/mtk-cqdma.c |  867 ++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 880 insertions(+)
> > >  create mode 100644 drivers/dma/mediatek/mtk-cqdma.c
> > >
> > > diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> > > index 27bac0b..4a1582d 100644
> > > --- a/drivers/dma/mediatek/Kconfig
> > > +++ b/drivers/dma/mediatek/Kconfig
> > > @@ -11,3 +11,15 @@ config MTK_HSDMA
> > >           This controller provides the channels which is dedicated to
> > >           memory-to-memory transfer to offload from CPU through ring-
> > >           based descriptor management.
> > > +
> > > +config MTK_CQDMA
> > > +       tristate "MediaTek Command-Queue DMA controller support"
> > > +       depends on ARCH_MEDIATEK || COMPILE_TEST
> > > +       select DMA_ENGINE
> > > +       select DMA_VIRTUAL_CHANNELS
> > > +       help
> > > +         Enable support for Command-Queue DMA controller on MediaTek
> > > +         SoCs.
> > > +
> > > +         This controller provides the channels which is dedicated to
> > > +         memory-to-memory transfer to offload from CPU.
> > > diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
> > > index 6e778f8..41bb381 100644
> > > --- a/drivers/dma/mediatek/Makefile
> > > +++ b/drivers/dma/mediatek/Makefile
> > > @@ -1 +1,2 @@
> > >  obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> > > +obj-$(CONFIG_MTK_CQDMA) += mtk-cqdma.o
> > > diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c
> > > new file mode 100644
> > > index 0000000..304eb0a
> > > --- /dev/null
> > > +++ b/drivers/dma/mediatek/mtk-cqdma.c
> > > @@ -0,0 +1,867 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2018-2019 MediaTek Inc.
> > > +
> > > +/*
> > > + * Driver for MediaTek Command-Queue DMA Controller
> > > + *
> > > + * Author: Shun-Chih Yu <shun-chih.yu@mediatek.com>
> > > + *
> > > + */
> > > +
> > > +#include <linux/bitops.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/dmaengine.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/err.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/list.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_dma.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/refcount.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#include "../virt-dma.h"
> > > +
> > > +#define MTK_CQDMA_USEC_POLL            10
> > > +#define MTK_CQDMA_TIMEOUT_POLL         1000
> > > +#define MTK_CQDMA_DMA_BUSWIDTHS                BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)
> > > +#define MTK_CQDMA_ALIGN_SIZE           1
> > > +
> > > +/* The default number of virtual channel */
> > > +#define MTK_CQDMA_NR_VCHANS            32
> > > +
> > > +/* The default number of physical channel */
> > > +#define MTK_CQDMA_NR_PCHANS            3
> > > +
> > > +/* Registers for underlying dma manipulation */
> > > +#define MTK_CQDMA_INT_FLAG             0x0
> > > +#define MTK_CQDMA_INT_EN               0x4
> > > +#define MTK_CQDMA_EN                   0x8
> > > +#define MTK_CQDMA_RESET                        0xc
> > > +#define MTK_CQDMA_FLUSH                        0x14
> > > +#define MTK_CQDMA_SRC                  0x1c
> > > +#define MTK_CQDMA_DST                  0x20
> > > +#define MTK_CQDMA_LEN1                 0x24
> > > +#define MTK_CQDMA_LEN2                 0x28
> >
> > drop unused macro and check if it happens at other places
I would remove this in next revision.
> >
> > > +#define MTK_CQDMA_SRC2                 0x60
> > > +#define MTK_CQDMA_DST2                 0x64
> > > +
> > > +/* Registers setting */
> > > +#define MTK_CQDMA_EN_BIT               BIT(0)
> > > +#define MTK_CQDMA_INT_FLAG_BIT         BIT(0)
> > > +#define MTK_CQDMA_INT_EN_BIT           BIT(0)
> > > +#define MTK_CQDMA_FLUSH_BIT            BIT(0)
> > > +
> > > +#define MTK_CQDMA_WARM_RST_BIT         BIT(0)
> > > +#define MTK_CQDMA_HARD_RST_BIT         BIT(1)
> > > +
> > > +#define MTK_CQDMA_MAX_LEN              GENMASK(27, 0)
> > > +#define MTK_CQDMA_ADDR_LIMIT           GENMASK(31, 0)
> > > +#define MTK_CQDMA_ADDR2_SHFIT          (32)
> > > +
> > > +/**
> > > + * struct mtk_cqdma_vdesc - The struct holding info describing virtual
> > > + *                         descriptor (CVD)
> > > + * @vd:                    An instance for struct virt_dma_desc
> > > + * @len:                   The total data size device wants to move
> > > + * @dest:                  The destination address device wants to move to
> > > + * @src:                   The source address device wants to move from
> > > + * @ch:                    The pointer to the corresponding dma channel
> > > + * @node:                  To build linked-list for PC queue
> > > + */
> > > +struct mtk_cqdma_vdesc {
> > > +       struct virt_dma_desc vd;
> > > +       size_t len;
> > > +       dma_addr_t dest;
> > > +       dma_addr_t src;
> > > +       struct dma_chan *ch;
> > > +
> > > +       /* protected by pc.lock */
> > > +       struct list_head node;
> > > +};
> > > +
> > > +/**
> > > + * struct mtk_cqdma_pchan - The struct holding info describing physical
> > > + *                         channel (PC)
> > > + * @queue:                 Queue for the CVDs issued to this PC
> > > + * @base:                  The mapped register I/O base of this PC
> > > + * @irq:                   The IRQ that this PC are using
> > > + * @refcnt:                Track how many VCs are using this PC
> > > + * @lock:                 Lock protect agaisting multiple VCs access PC
> > > + */
> > > +struct mtk_cqdma_pchan {
> > > +       struct list_head queue;
> > > +       void __iomem *base;
> > > +       u32 irq;
> > > +       refcount_t refcnt;
> > > +
> > > +       /* lock to protect PC */
> > > +       spinlock_t lock;
> > > +};
> > > +
> > > +/**
> > > + * struct mtk_cqdma_vchan - The struct holding info describing virtual
> > > + *                         channel (VC)
> > > + * @vc:                    An instance for struct virt_dma_chan
> > > + * @pc:                    The pointer to the underlying PC
> > > + * @issue_completion:     The wait for all issued descriptors completited
> > > + * @issue_synchronize:    Bool indicating channel synchronization starts
> > > + */
> > > +struct mtk_cqdma_vchan {
> > > +       struct virt_dma_chan vc;
> > > +       struct mtk_cqdma_pchan *pc;
> > > +       struct completion issue_completion;
> > > +       bool issue_synchronize;
> > > +       /* protected by vc.lock */
> >
> > the line should be dropped
I would remove this.
> >
> > > +};
> > > +
> > > +/**
> > > + * struct mtk_cqdma_device - The struct holding info describing CQDMA
> > > + *                          device
> > > + * @ddev:                   An instance for struct dma_device
> > > + * @clk:                    The clock that device internal is using
> > > + * @dma_requests:           The number of VCs the device supports to
> > > + * @dma_channels:           The number of PCs the device supports to
> > > + * @vc:                     The pointer to all available VCs
> > > + * @pc:                     The pointer to all the underlying PCs
> > > + */
> > > +struct mtk_cqdma_device {
> > > +       struct dma_device ddev;
> > > +       struct clk *clk;
> > > +
> > > +       u32 dma_requests;
> > > +       u32 dma_channels;
> > > +       struct mtk_cqdma_vchan *vc;
> > > +       struct mtk_cqdma_pchan **pc;
> > > +};
> > > +
> > > +static struct mtk_cqdma_device *to_cqdma_dev(struct dma_chan *chan)
> > > +{
> > > +       return container_of(chan->device, struct mtk_cqdma_device, ddev);
> > > +}
> > > +
> > > +static struct mtk_cqdma_vchan *to_cqdma_vchan(struct dma_chan *chan)
> > > +{
> > > +       return container_of(chan, struct mtk_cqdma_vchan, vc.chan);
> > > +}
> > > +
> > > +static struct mtk_cqdma_vdesc *to_cqdma_vdesc(struct virt_dma_desc *vd)
> > > +{
> > > +       return container_of(vd, struct mtk_cqdma_vdesc, vd);
> > > +}
> > > +
> > > +static struct device *cqdma2dev(struct mtk_cqdma_device *cqdma)
> > > +{
> > > +       return cqdma->ddev.dev;
> > > +}
> > > +
> > > +static u32 mtk_dma_read(struct mtk_cqdma_pchan *pc, u32 reg)
> > > +{
> > > +       return readl(pc->base + reg);
> > > +}
> > > +
> > > +static void mtk_dma_write(struct mtk_cqdma_pchan *pc, u32 reg, u32 val)
> > > +{
> > > +       writel_relaxed(val, pc->base + reg);
> >
> > what is the reason register write using relaxed version not consistent
> > with register read?
Both of them should be relaxed version, and I would fix the read one.
> >
> > > +}
> > > +
> > > +static void mtk_dma_rmw(struct mtk_cqdma_pchan *pc, u32 reg,
> > > +                       u32 mask, u32 set)
> > > +{
> > > +       u32 val;
> > > +
> > > +       val = mtk_dma_read(pc, reg);
> > > +       val &= ~mask;
> > > +       val |= set;
> > > +       mtk_dma_write(pc, reg, val);
> > > +}
> > > +
> > > +static void mtk_dma_set(struct mtk_cqdma_pchan *pc, u32 reg, u32 val)
> > > +{
> > > +       mtk_dma_rmw(pc, reg, 0, val);
> > > +}
> > > +
> > > +static void mtk_dma_clr(struct mtk_cqdma_pchan *pc, u32 reg, u32 val)
> > > +{
> > > +       mtk_dma_rmw(pc, reg, val, 0);
> > > +}
> > > +
> > > +static void mtk_cqdma_vdesc_free(struct virt_dma_desc *vd)
> > > +{
> > > +       kfree(to_cqdma_vdesc(vd));
> > > +}
> > > +
> > > +static int mtk_cqdma_poll_engine_done(struct mtk_cqdma_pchan *pc, bool atomic)
> > > +{
> > > +       u32 status = 0;
> > > +
> > > +       if (!atomic)
> > > +               return readl_poll_timeout(pc->base + MTK_CQDMA_EN,
> > > +                                         status,
> > > +                                         !(status & MTK_CQDMA_EN_BIT),
> > > +                                         MTK_CQDMA_USEC_POLL,
> > > +                                         MTK_CQDMA_TIMEOUT_POLL);
> > > +
> > > +       return readl_poll_timeout_atomic(pc->base + MTK_CQDMA_EN,
> > > +                                        status,
> > > +                                        !(status & MTK_CQDMA_EN_BIT),
> > > +                                        MTK_CQDMA_USEC_POLL,
> > > +                                        MTK_CQDMA_TIMEOUT_POLL);
> >
> > it seems we can use macro in_task to check the current context and
> > drop the variable atomic passing.
Sure, is would be better to use macro in_task.
> >
> > > +}
> > > +
> > > +static int mtk_cqdma_hard_reset(struct mtk_cqdma_pchan *pc)
> > > +{
> > > +       mtk_dma_set(pc, MTK_CQDMA_RESET, MTK_CQDMA_HARD_RST_BIT);
> > > +       mtk_dma_clr(pc, MTK_CQDMA_RESET, MTK_CQDMA_HARD_RST_BIT);
> > > +
> > > +       return mtk_cqdma_poll_engine_done(pc, false);
> > > +}
> > > +
> > > +static void mtk_cqdma_start(struct mtk_cqdma_pchan *pc,
> > > +                           struct mtk_cqdma_vdesc *cvd)
> > > +{
> > > +       /* wait for the previous transaction done */
> > > +       if (mtk_cqdma_poll_engine_done(pc, true) < 0)
> > > +               dev_err(cqdma2dev(to_cqdma_dev(cvd->ch)),
> > > +                       "cqdma wait transaction timeout\n");
> >
> > I thought the poll can be dropped since the irq fire and then next
> > transaction starts guarantees the previous transaction was already
> > finished.
> >
Yes, it is not necessary to poll here since the driver guarantees the
previous one retired before the next transaction starts.
> > > +
> > > +       /* warm reset the dma engine for the new transaction */
> > > +       mtk_dma_set(pc, MTK_CQDMA_RESET, MTK_CQDMA_WARM_RST_BIT);
> > > +       if (mtk_cqdma_poll_engine_done(pc, true) < 0)
> > > +               dev_err(cqdma2dev(to_cqdma_dev(cvd->ch)),
> > > +                       "cqdma warm reset timeout\n");
> > > +
> >
> > In general, warm reset is only present at the beginning setup or at a
> > necessary time such as hardware fault happens, and not blindly done
> > for each descriptor. Otherwise, it will hide some errors from hardware
> > and can't be found in time and fixed on the next version.
> >
Yes, yet the CQDMA hardware engine must be reset for every transaction,
which is a limitation in the current version of CQDMA.

> > > +       /* setup the source */
> > > +       mtk_dma_set(pc, MTK_CQDMA_SRC, cvd->src & MTK_CQDMA_ADDR_LIMIT);
> > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> > > +       mtk_dma_set(pc, MTK_CQDMA_SRC2, cvd->src >> MTK_CQDMA_ADDR2_SHFIT);
> > > +#else
> > > +       mtk_dma_set(pc, MTK_CQDMA_SRC2, 0);
> > > +#endif
> > > +
> > > +       /* setup the destination */
> > > +       mtk_dma_set(pc, MTK_CQDMA_DST, cvd->dest & MTK_CQDMA_ADDR_LIMIT);
> > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> > > +       mtk_dma_set(pc, MTK_CQDMA_DST2, cvd->dest >> MTK_CQDMA_ADDR2_SHFIT);
> > > +#else
> > > +       mtk_dma_set(pc, MTK_CQDMA_SRC2, 0);
> > > +#endif
> > > +
> > > +       /* setup the length */
> > > +       mtk_dma_set(pc, MTK_CQDMA_LEN1,
> > > +                   (cvd->len < MTK_CQDMA_MAX_LEN) ?
> > > +                   cvd->len : MTK_CQDMA_MAX_LEN);
> >
> > it can be close to 80 chars wrap as much as possible
Sure, I would fix this.
> >
> > > +
> > > +       /* start dma engine */
> > > +       mtk_dma_set(pc, MTK_CQDMA_EN, MTK_CQDMA_EN_BIT);
> > > +}
> > > +
> > > +static void mtk_cqdma_issue_vchan_pending(struct mtk_cqdma_vchan *cvc)
> > > +{
> > > +       struct virt_dma_desc *vd, *vd2;
> > > +       struct mtk_cqdma_pchan *pc = cvc->pc;
> > > +       struct mtk_cqdma_vdesc *cvd;
> > > +       bool trigger_engine = false;
> > > +
> > > +       lockdep_assert_held(&cvc->vc.lock);
> > > +       lockdep_assert_held(&pc->lock);
> > > +
> > > +       list_for_each_entry_safe(vd, vd2, &cvc->vc.desc_issued, node) {
> > > +               /* need to trigger dma engine if PC's queue is empty */
> > > +               if (list_empty(&pc->queue))
> > > +                       trigger_engine = true;
> > > +
> > > +               cvd = to_cqdma_vdesc(vd);
> > > +
> > > +               /* add VD into PC's queue */
> > > +               list_add_tail(&cvd->node, &pc->queue);
> > > +
> > > +               /* start the dma engine */
> > > +               if (trigger_engine)
> > > +                       mtk_cqdma_start(pc, cvd);
> > > +
> > > +               /* remove VD from list desc_issued */
> > > +               list_del(&vd->node);
> >
> > it is unnecessary to use an additional pc->queue because the hardware
> > only can handle most up to one descriptor at a time.
> >
> > Instead, it would make more sense to only use a pointer
> > pc->active_vdesc pointing to the descriptor at the head of list
> > desc_issued indicates the descriptor the hardware is processing, then
> > delete the head, then still leave other pending descriptors in the
> > list desc_issued until the irq fire and then determine if go on the
> > current uncompleted or load the next descriptor from the list
> > desc_issued.
> >
It is make sense to use active_vdesc instead.I would refactor this part
in next revision.
> > > +       }
> > > +}
> > > +
> > > +/*
> > > + * return true if this VC is active,
> > > + * meaning that there are VDs under processing by the PC
> > > + */
> > > +static bool mtk_cqdma_is_vchan_active(struct mtk_cqdma_vchan *cvc)
> > > +{
> > > +       struct mtk_cqdma_vdesc *cvd;
> > > +
> > > +       list_for_each_entry(cvd, &cvc->pc->queue, node)
> > > +               if (cvc == to_cqdma_vchan(cvd->ch))
> > > +                       return true;
> >
> > Similar to the above, it would be simple if we can add a variable in
> > pc called pc->active_vchan and just return pc->active_vchan == cvc
> > instead of a loop searching.
> >
> > pc->active_vchan can be determined at mtk_cqdma_issue_vchan_pending
Sure. Maybe there is no need to add variable active_vchan, because it
can be replaced with to_cqdma_vchan(cvc->pc->active_vdesc->ch).
> > > +
> > > +       return false;
> > > +}
> > > +
> > > +/*
> > > + * return the pointer of the CVD that is just consumed by the PC
> > > + */
> > > +static void mtk_cqdma_consume_work_queue(struct mtk_cqdma_pchan *pc)
> > > +{
> > > +       struct mtk_cqdma_vchan *cvc;
> > > +       struct mtk_cqdma_vdesc *cvd;
> > > +       size_t tlen;
> > > +
> > > +       /* consume a CVD from PC's queue */
> > > +       cvd = list_first_entry_or_null(&pc->queue,
> > > +                                      struct mtk_cqdma_vdesc, node);
> > > +       if (unlikely(!cvd))
> > > +               return;
> > > +
> > > +       cvc = to_cqdma_vchan(cvd->ch);
> > > +
> > > +       tlen = (cvd->len < MTK_CQDMA_MAX_LEN) ? cvd->len : MTK_CQDMA_MAX_LEN;
> > > +       cvd->len -= tlen;
> > > +       cvd->src += tlen;
> > > +       cvd->dest += tlen;
> > > +
> > > +       /* check whether the CVD completed */
> > > +       if (!cvd->len) {
> > > +               /* delete CVD from PC's queue */
> > > +               list_del(&cvd->node);
> > > +
> > > +               spin_lock(&cvc->vc.lock);
> > > +
> > > +               /* add the VD into list desc_completed */
> > > +               vchan_cookie_complete(&cvd->vd);
> > > +
> > > +               /* setup completion if this VC is under synchronization */
> > > +               if (cvc->issue_synchronize && !mtk_cqdma_is_vchan_active(cvc)) {
> > > +                       complete(&cvc->issue_completion);
> > > +                       cvc->issue_synchronize = false;
> > > +               }
> > > +
> > > +               spin_unlock(&cvc->vc.lock);
> > > +       }
> > > +
> 
> Below code snippet that can call mtk_cqdma_issue_vchan_pending to
> share same code involved from the user context.
Yes, it would be better to call mtk_cqdma_issue_vchan_pending instead.
> 
> If you really want to schedule virtual channels on the same physical
> channel on the first-issued and first-served basis, we can use
> pc->sched_vdesc (I would like the naming instead of pc->queue for
> being a little straightforward read the code) for the purpose and put
> the issued descriptors at the tail of pc->sched_vdesc at
> mtk_cqdma_issue_pending at a time by
> list_splice_tail_init(&vc->desc_issued, &pc->sched_vdesc) by the
> issuing order of each virtual channel. Finally, pc->active_vdesc
> requires being got from the head of pc->sched_vdesc in
> mtk_cqdma_issue_vchan_pending.
> 
> The extra pc->sched_vdesc and pc->active_vdesc can help split the
> channel schedule and hardware real work into a separate logic that
> would allow people to know the scheduling policy and what setup the
> hardware really must do.
> 
Using pc->sched_vdesc might be a good implementation for scheduling on
multiple VCs that share the same PC, I would add this part in the next
revision.

> > > +       /* iterate on the next CVD if the current CVD completed */
> > > +       if (!cvd->len)
> > > +               cvd = list_first_entry_or_null(&pc->queue,
> > > +                                              struct mtk_cqdma_vdesc, node);
> > > +
> > > +       /* start the next transaction */
> > > +       if (cvd)
> > > +               mtk_cqdma_start(pc, cvd);
> > > +}
> > > +
> > > +static irqreturn_t mtk_cqdma_irq(int irq, void *devid)
> > > +{
> > > +       struct mtk_cqdma_device *cqdma = devid;
> > > +       irqreturn_t ret = IRQ_NONE;
> > > +       u32 i;
> > > +
> > > +       /* clear interrupt flags for each PC */
> > > +       for (i = 0; i < cqdma->dma_channels; ++i) {
> > > +               spin_lock(&cqdma->pc[i]->lock);
> > > +               if (mtk_dma_read(cqdma->pc[i],
> > > +                                MTK_CQDMA_INT_FLAG) & MTK_CQDMA_INT_FLAG_BIT) {
> > > +                       /* clear interrupt */
> > > +                       mtk_dma_clr(cqdma->pc[i], MTK_CQDMA_INT_FLAG,
> > > +                                   MTK_CQDMA_INT_FLAG_BIT);
> > > +
> > > +                       mtk_cqdma_consume_work_queue(cqdma->pc[i]);
> > > +
> > > +                       ret = IRQ_HANDLED;
> > > +               }
> > > +               spin_unlock(&cqdma->pc[i]->lock);
> > > +       }
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static struct virt_dma_desc *mtk_cqdma_find_active_desc(struct dma_chan *c,
> > > +                                                       dma_cookie_t cookie)
> > > +{
> > > +       struct mtk_cqdma_vchan *cvc = to_cqdma_vchan(c);
> > > +       struct virt_dma_desc *vd;
> > > +       unsigned long flags;
> > > +
> > > +       spin_lock_irqsave(&cvc->pc->lock, flags);
> > > +       list_for_each_entry(vd, &cvc->pc->queue, node)
> > > +               if (vd->tx.cookie == cookie) {
> > > +                       spin_unlock_irqrestore(&cvc->pc->lock, flags);
> > > +                       return vd;
> > > +               }
> > > +       spin_unlock_irqrestore(&cvc->pc->lock, flags);
> > > +
> > > +       list_for_each_entry(vd, &cvc->vc.desc_issued, node)
> > > +               if (vd->tx.cookie == cookie)
> > > +                       return vd;
> > > +
> 
> sure, we should look for cvc->active_vdesc, cvc->pc->sched_vdesc and
> cvc->vc.desc_issued that all should be protected by a proper lock.
> 
Sure, we should look for all of them.
> > > +       return NULL;
> > > +}
> > > +
> > > +static enum dma_status mtk_cqdma_tx_status(struct dma_chan *c,
> > > +                                          dma_cookie_t cookie,
> > > +                                          struct dma_tx_state *txstate)
> > > +{
> > > +       struct mtk_cqdma_vchan *cvc = to_cqdma_vchan(c);
> > > +       struct mtk_cqdma_vdesc *cvd;
> > > +       struct virt_dma_desc *vd;
> > > +       enum dma_status ret;
> > > +       unsigned long flags;
> > > +       size_t bytes = 0;
> > > +
> > > +       ret = dma_cookie_status(c, cookie, txstate);
> > > +       if (ret == DMA_COMPLETE || !txstate)
> > > +               return ret;
> > > +
> > > +       spin_lock_irqsave(&cvc->vc.lock, flags);
> > > +       vd = mtk_cqdma_find_active_desc(c, cookie);
> > > +       spin_unlock_irqrestore(&cvc->vc.lock, flags);
> > > +
> > > +       if (vd) {
> > > +               cvd = to_cqdma_vdesc(vd);
> > > +               bytes = cvd->len;
> >
> > we can consider register MTK_CQDMA_LEN1 to know what left data the
> > hardware not finished on the fly.
I would consider about this. Yet reading register MTK_CQDMA_LEN1
requires pc->lock to make source the current processed VD is identical
with the VD we are looking for, which might make the driver more complex
and degrade the performance (one more user on pc->lock).
> > > +       }
> > > +
> > > +       dma_set_residue(txstate, bytes);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static void mtk_cqdma_issue_pending(struct dma_chan *c)
> > > +{
> > > +       struct mtk_cqdma_vchan *cvc = to_cqdma_vchan(c);
> > > +       unsigned long pc_flags;
> > > +       unsigned long vc_flags;
> > > +
> > > +       /* acquire PC's lock before VS's lock for lock dependency in ISR */
> > > +       spin_lock_irqsave(&cvc->pc->lock, pc_flags);
> > > +       spin_lock_irqsave(&cvc->vc.lock, vc_flags);
> > > +
> > > +       if (vchan_issue_pending(&cvc->vc))
> > > +               mtk_cqdma_issue_vchan_pending(cvc);
> 
> we can queue the waiting list at a time by
> list_splice_tail_init(&vc->desc_issued, &pc->sched_vdesc) instead of
> one by one and then call mtk_cqdma_issue_vchan_pending to determine
> active_vdesc and active_vchan the hardware should be working at in the
> current run.
> 
Sure, I would add this in the next revision.
> > > +
> > > +       spin_unlock_irqrestore(&cvc->vc.lock, vc_flags);
> > > +       spin_unlock_irqrestore(&cvc->pc->lock, pc_flags);
> > > +}
> > > +
> > > +static struct dma_async_tx_descriptor *
> > > +mtk_cqdma_prep_dma_memcpy(struct dma_chan *c, dma_addr_t dest,
> > > +                         dma_addr_t src, size_t len, unsigned long flags)
> > > +{
> > > +       struct mtk_cqdma_vdesc *cvd;
> > > +
> > > +       cvd = kzalloc(sizeof(*cvd), GFP_NOWAIT);
> > > +       if (!cvd)
> > > +               return NULL;
> > > +
> > > +       /* setup dma channel */
> > > +       cvd->ch = c;
> > > +
> > > +       /* setup sourece, destination, and length */
> > > +       cvd->len = len;
> > > +       cvd->src = src;
> > > +       cvd->dest = dest;
> > > +
> > > +       return vchan_tx_prep(to_virt_chan(c), &cvd->vd, flags);
> > > +}
> > > +
> > > +static void mtk_cqdma_free_inactive_desc(struct dma_chan *c)
> > > +{
> > > +       struct virt_dma_chan *vc = to_virt_chan(c);
> > > +       unsigned long flags;
> > > +       LIST_HEAD(head);
> > > +
> > > +       /*
> > > +        * set desc_allocated, desc_submitted,
> > > +        * and desc_issued as the candicates to be freed
> > > +        */
> > > +       spin_lock_irqsave(&vc->lock, flags);
> > > +       list_splice_tail_init(&vc->desc_allocated, &head);
> > > +       list_splice_tail_init(&vc->desc_submitted, &head);
> > > +       list_splice_tail_init(&vc->desc_issued, &head);
> > > +       spin_unlock_irqrestore(&vc->lock, flags);
> > > +
> 
> sched_vdesc also should be free up here by
> list_splice_tail_init(&pc->sched_vdesc, &head); with a proper lock
Sure.
> > > +       /* free descriptor lists */
> > > +       vchan_dma_desc_free_list(vc, &head);
> > > +}
> > > +
> > > +static void mtk_cqdma_free_active_desc(struct dma_chan *c)
> > > +{
> > > +       struct mtk_cqdma_vchan *cvc = to_cqdma_vchan(c);
> > > +       bool sync_needed = false;
> > > +       unsigned long pc_flags;
> > > +       unsigned long vc_flags;
> > > +
> > > +       /* acquire PC's lock first due to lock dependency in dma ISR */
> > > +       spin_lock_irqsave(&cvc->pc->lock, pc_flags);
> > > +       spin_lock_irqsave(&cvc->vc.lock, vc_flags);
> > > +
> > > +       /* synchronization is required if this VC is active */
> > > +       if (mtk_cqdma_is_vchan_active(cvc)) {
> > > +               cvc->issue_synchronize = true;
> > > +               sync_needed = true;
> > > +       }
> > > +
> > > +       spin_unlock_irqrestore(&cvc->vc.lock, vc_flags);
> > > +       spin_unlock_irqrestore(&cvc->pc->lock, pc_flags);
> > > +
> > > +       /* waiting for the completion of this VC */
> > > +       if (sync_needed)
> > > +               wait_for_completion(&cvc->issue_completion);
> > > +
> > > +       /* free all descriptors in list desc_completed */
> > > +       vchan_synchronize(&cvc->vc);
> > > +
> > > +       WARN_ONCE(!list_empty(&cvc->vc.desc_completed),
> > > +                 "Desc pending still in list desc_completed\n");
> > > +}
> > > +
> > > +static int mtk_cqdma_terminate_all(struct dma_chan *c)
> > > +{
> > > +       /* free descriptors not processed yet by hardware */
> > > +       mtk_cqdma_free_inactive_desc(c);
> > > +
> > > +       /* free descriptors being processed by hardware */
> > > +       mtk_cqdma_free_active_desc(c);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int mtk_cqdma_alloc_chan_resources(struct dma_chan *c)
> > > +{
> > > +       struct mtk_cqdma_device *cqdma = to_cqdma_dev(c);
> > > +       struct mtk_cqdma_vchan *vc = to_cqdma_vchan(c);
> > > +       struct mtk_cqdma_pchan *pc = NULL;
> > > +       u32 i, min_refcnt = U32_MAX, refcnt;
> > > +       unsigned long flags;
> > > +
> > > +       /* allocate PC with the minimum refcount */
> > > +       for (i = 0; i < cqdma->dma_channels; ++i) {
> > > +               refcnt = refcount_read(&cqdma->pc[i]->refcnt);
> > > +               if (refcnt < min_refcnt) {
> > > +                       pc = cqdma->pc[i];
> > > +                       min_refcnt = refcnt;
> > > +               }
> > > +       }
> > > +
> > > +       if (!pc)
> > > +               return -ENOSPC;
> > > +
> > > +       spin_lock_irqsave(&pc->lock, flags);
> > > +
> > > +       if (!refcount_read(&pc->refcnt)) {
> > > +               /* allocate PC when the refcount is zero */
> > > +               mtk_cqdma_hard_reset(pc);
> > > +
> > > +               /* enable interrupt for this PC */
> > > +               mtk_dma_set(pc, MTK_CQDMA_INT_EN, MTK_CQDMA_INT_EN_BIT);
> > > +
> > > +               /*
> > > +                * refcount_inc would complain increment on 0; use-after-free.
> > > +                * Thus, we need to explicitly set it as 1 initially.
> > > +                */
> > > +               refcount_set(&pc->refcnt, 1);
> > > +       } else {
> > > +               refcount_inc(&pc->refcnt);
> > > +       }
> > > +
> > > +       spin_unlock_irqrestore(&pc->lock, flags);
> > > +
> > > +       vc->pc = pc;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void mtk_cqdma_free_chan_resources(struct dma_chan *c)
> > > +{
> > > +       struct mtk_cqdma_vchan *cvc = to_cqdma_vchan(c);
> > > +       unsigned long flags;
> > > +
> > > +       /* free all descriptors in all lists on the VC */
> > > +       mtk_cqdma_terminate_all(c);
> > > +
> > > +       spin_lock_irqsave(&cvc->pc->lock, flags);
> > > +
> > > +       /* PC is not freed until there is no VC mapped to it */
> > > +       if (refcount_dec_and_test(&cvc->pc->refcnt)) {
> > > +               /* start the flush operation and stop the engine */
> > > +               mtk_dma_set(cvc->pc, MTK_CQDMA_FLUSH, MTK_CQDMA_FLUSH_BIT);
> > > +
> > > +               /* wait for the completion of flush operation */
> > > +               if (mtk_cqdma_poll_engine_done(cvc->pc, false) < 0)
> > > +                       dev_err(cqdma2dev(to_cqdma_dev(c)),
> > > +                               "cqdma flush timeout\n");
> > > +
> > > +               /* clear the flush bit and interrupt flag */
> > > +               mtk_dma_clr(cvc->pc, MTK_CQDMA_FLUSH, MTK_CQDMA_FLUSH_BIT);
> > > +               mtk_dma_clr(cvc->pc, MTK_CQDMA_INT_FLAG,
> > > +                           MTK_CQDMA_INT_FLAG_BIT);
> > > +
> > > +               /* disable interrupt for this PC */
> > > +               mtk_dma_clr(cvc->pc, MTK_CQDMA_INT_EN, MTK_CQDMA_INT_EN_BIT);
> > > +       }
> > > +
> > > +       spin_unlock_irqrestore(&cvc->pc->lock, flags);
> > > +}
> > > +
> > > +static int mtk_cqdma_hw_init(struct mtk_cqdma_device *cqdma)
> > > +{
> > > +       unsigned long flags;
> > > +       int err;
> > > +       u32 i;
> > > +
> > > +       pm_runtime_enable(cqdma2dev(cqdma));
> > > +       pm_runtime_get_sync(cqdma2dev(cqdma));
> > > +
> > > +       err = clk_prepare_enable(cqdma->clk);
> > > +
> > > +       if (err) {
> > > +               pm_runtime_put_sync(cqdma2dev(cqdma));
> > > +               pm_runtime_disable(cqdma2dev(cqdma));
> 
> use goto clk_err; something like to have an error path.
> 
Sure, I would fix in the next revision.
> > > +               return err;
> > > +       }
> > > +
> > > +       /* reset all PCs */
> > > +       for (i = 0; i < cqdma->dma_channels; ++i) {
> > > +               spin_lock_irqsave(&cqdma->pc[i]->lock, flags);
> > > +               if (mtk_cqdma_hard_reset(cqdma->pc[i]) < 0) {
> > > +                       dev_err(cqdma2dev(cqdma), "cqdma hard reset timeout\n");
> > > +                       spin_unlock_irqrestore(&cqdma->pc[i]->lock, flags);
> > > +
> > > +                       clk_disable_unprepare(cqdma->clk);
> > > +                       pm_runtime_put_sync(cqdma2dev(cqdma));
> > > +                       pm_runtime_disable(cqdma2dev(cqdma));
> > > +                       return -EINVAL;
> 
> use goto reset_err; something like to have an error path.
> 
Sure, I would fix in the next revision.
> > > +               }
> > > +               spin_unlock_irqrestore(&cqdma->pc[i]->lock, flags);
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void mtk_cqdma_hw_deinit(struct mtk_cqdma_device *cqdma)
> > > +{
> > > +       unsigned long flags;
> > > +       u32 i;
> > > +
> > > +       /* reset all PCs */
> > > +       for (i = 0; i < cqdma->dma_channels; ++i) {
> > > +               spin_lock_irqsave(&cqdma->pc[i]->lock, flags);
> > > +               if (mtk_cqdma_hard_reset(cqdma->pc[i]) < 0)
> > > +                       dev_err(cqdma2dev(cqdma), "cqdma hard reset timeout\n");
> > > +               spin_unlock_irqrestore(&cqdma->pc[i]->lock, flags);
> > > +       }
> > > +
> > > +       clk_disable_unprepare(cqdma->clk);
> > > +
> > > +       pm_runtime_put_sync(cqdma2dev(cqdma));
> > > +       pm_runtime_disable(cqdma2dev(cqdma));
> > > +}
> > > +
> > > +static const struct of_device_id mtk_cqdma_match[] = {
> > > +       { .compatible = "mediatek,mt6765-cqdma" },
> > > +       { /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_cqdma_match);
> > > +
> > > +static int mtk_cqdma_probe(struct platform_device *pdev)
> > > +{
> > > +       struct mtk_cqdma_device *cqdma;
> > > +       struct mtk_cqdma_vchan *vc;
> > > +       struct dma_device *dd;
> > > +       struct resource *res;
> > > +       int err;
> > > +       u32 i;
> > > +
> > > +       cqdma = devm_kzalloc(&pdev->dev, sizeof(*cqdma), GFP_KERNEL);
> > > +       if (!cqdma)
> > > +               return -ENOMEM;
> > > +
> > > +       dd = &cqdma->ddev;
> > > +
> > > +       cqdma->clk = devm_clk_get(&pdev->dev, "cqdma");
> > > +       if (IS_ERR(cqdma->clk)) {
> > > +               dev_err(&pdev->dev, "No clock for %s\n",
> > > +                       dev_name(&pdev->dev));
> > > +               return PTR_ERR(cqdma->clk);
> > > +       }
> > > +
> > > +       dma_cap_set(DMA_MEMCPY, dd->cap_mask);
> > > +
> > > +       dd->copy_align = MTK_CQDMA_ALIGN_SIZE;
> > > +       dd->device_alloc_chan_resources = mtk_cqdma_alloc_chan_resources;
> > > +       dd->device_free_chan_resources = mtk_cqdma_free_chan_resources;
> > > +       dd->device_tx_status = mtk_cqdma_tx_status;
> > > +       dd->device_issue_pending = mtk_cqdma_issue_pending;
> > > +       dd->device_prep_dma_memcpy = mtk_cqdma_prep_dma_memcpy;
> > > +       dd->device_terminate_all = mtk_cqdma_terminate_all;
> > > +       dd->src_addr_widths = MTK_CQDMA_DMA_BUSWIDTHS;
> > > +       dd->dst_addr_widths = MTK_CQDMA_DMA_BUSWIDTHS;
> > > +       dd->directions = BIT(DMA_MEM_TO_MEM);
> > > +       dd->residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> > > +       dd->dev = &pdev->dev;
> > > +       INIT_LIST_HEAD(&dd->channels);
> > > +
> > > +       if (pdev->dev.of_node && of_property_read_u32(pdev->dev.of_node,
> > > +                                                     "dma-requests",
> > > +                                                     &cqdma->dma_requests)) {
> >
> > pdev->dev.of_node can be dropped if the driver is of based
> >
Sure, I would fix in the next revision.
> > > +               dev_dbg(&pdev->dev,
> > > +                       "Using %u as missing dma-requests property\n",
> > > +                       MTK_CQDMA_NR_VCHANS);
> > > +
> > > +               cqdma->dma_requests = MTK_CQDMA_NR_VCHANS;
> > > +       }
> > > +
> > > +       if (pdev->dev.of_node && of_property_read_u32(pdev->dev.of_node,
> > > +                                                     "dma-channels",
> > > +                                                     &cqdma->dma_channels)) {
> >
> > pdev->dev.of_node can be dropped if the driver is of based
> >
Sure, I would fix in the next revision.
> > > +               dev_dbg(&pdev->dev,
> > > +                       "Using %u as missing dma-channels property\n",
> > > +                       MTK_CQDMA_NR_PCHANS);
> > > +
> > > +               cqdma->dma_channels = MTK_CQDMA_NR_PCHANS;
> > > +       }
> > > +
> > > +       cqdma->pc = devm_kcalloc(&pdev->dev, cqdma->dma_channels,
> > > +                                sizeof(*cqdma->pc), GFP_KERNEL);
> > > +       if (!cqdma->pc)
> > > +               return -ENOMEM;
> > > +
> > > +       /* initialization for PCs */
> > > +       for (i = 0; i < cqdma->dma_channels; ++i) {
> > > +               cqdma->pc[i] = devm_kcalloc(&pdev->dev, 1,
> > > +                                           sizeof(**cqdma->pc), GFP_KERNEL);
> > > +               if (!cqdma->pc[i])
> > > +                       return -ENOMEM;
> > > +
> > > +               INIT_LIST_HEAD(&cqdma->pc[i]->queue);
> > > +               spin_lock_init(&cqdma->pc[i]->lock);
> > > +               refcount_set(&cqdma->pc[i]->refcnt, 0);
> > > +
> > > +               res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > > +               if (!res) {
> > > +                       dev_err(&pdev->dev, "No mem resource for %s\n",
> > > +                               dev_name(&pdev->dev));
> > > +                       return -EINVAL;
> > > +               }
> > > +
> > > +               cqdma->pc[i]->base = devm_ioremap_resource(&pdev->dev, res);
> > > +               if (IS_ERR(cqdma->pc[i]->base))
> > > +                       return PTR_ERR(cqdma->pc[i]->base);
> > > +
> > > +               /* allocate IRQ resource */
> > > +               res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> > > +               if (!res) {
> > > +                       dev_err(&pdev->dev, "No irq resource for %s\n",
> > > +                               dev_name(&pdev->dev));
> > > +                       return -EINVAL;
> > > +               }
> > > +               cqdma->pc[i]->irq = res->start;
> > > +
> > > +               err = devm_request_irq(&pdev->dev, cqdma->pc[i]->irq,
> > > +                                      mtk_cqdma_irq, 0, dev_name(&pdev->dev),
> > > +                                      cqdma);
> > > +               if (err) {
> > > +                       dev_err(&pdev->dev,
> > > +                               "request_irq failed with err %d\n", err);
> > > +                       return -EINVAL;
> > > +               }
> > > +       }
> > > +
> > > +       /* allocate resource for VCs */
> > > +       cqdma->vc = devm_kcalloc(&pdev->dev, cqdma->dma_requests,
> > > +                                sizeof(*cqdma->vc), GFP_KERNEL);
> > > +       if (!cqdma->vc)
> > > +               return -ENOMEM;
> > > +
> > > +       for (i = 0; i < cqdma->dma_requests; i++) {
> > > +               vc = &cqdma->vc[i];
> > > +               vc->vc.desc_free = mtk_cqdma_vdesc_free;
> > > +               vchan_init(&vc->vc, dd);
> > > +               init_completion(&vc->issue_completion);
> > > +       }
> > > +
> > > +       err = dma_async_device_register(dd);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       err = of_dma_controller_register(pdev->dev.of_node,
> > > +                                        of_dma_xlate_by_chan_id, cqdma);
> > > +       if (err) {
> > > +               dev_err(&pdev->dev,
> > > +                       "MediaTek CQDMA OF registration failed %d\n", err);
> > > +               goto err_unregister;
> > > +       }
> > > +
> > > +       err = mtk_cqdma_hw_init(cqdma);
> > > +       if (err) {
> > > +               dev_err(&pdev->dev,
> > > +                       "MediaTek CQDMA HW initialization failed %d\n", err);
> > > +               goto err_unregister;
> > > +       }
> > > +
> > > +       platform_set_drvdata(pdev, cqdma);
> > > +
> > > +       dev_dbg(&pdev->dev, "MediaTek CQDMA driver registered\n");
> > > +
> > > +       return 0;
> > > +
> > > +err_unregister:
> > > +       dma_async_device_unregister(dd);
> > > +
> > > +       return err;
> > > +}
> > > +
> > > +static int mtk_cqdma_remove(struct platform_device *pdev)
> > > +{
> > > +       struct mtk_cqdma_device *cqdma = platform_get_drvdata(pdev);
> > > +       struct mtk_cqdma_vchan *vc;
> > > +       unsigned long flags;
> > > +       int i;
> > > +
> > > +       /* kill VC task */
> > > +       for (i = 0; i < cqdma->dma_requests; i++) {
> > > +               vc = &cqdma->vc[i];
> > > +
> > > +               list_del(&vc->vc.chan.device_node);
> > > +               tasklet_kill(&vc->vc.task);
> > > +       }
> > > +
> > > +       /* disable interrupt */
> > > +       for (i = 0; i < cqdma->dma_channels; i++) {
> > > +               spin_lock_irqsave(&cqdma->pc[i]->lock, flags);
> > > +               mtk_dma_clr(cqdma->pc[i], MTK_CQDMA_INT_EN,
> > > +                           MTK_CQDMA_INT_EN_BIT);
> > > +               spin_unlock_irqrestore(&cqdma->pc[i]->lock, flags);
> > > +
> > > +               /* Waits for any pending IRQ handlers to complete */
> > > +               synchronize_irq(cqdma->pc[i]->irq);
> > > +       }
> > > +
> > > +       /* disable hardware */
> > > +       mtk_cqdma_hw_deinit(cqdma);
> > > +
> > > +       dma_async_device_unregister(&cqdma->ddev);
> > > +       of_dma_controller_free(pdev->dev.of_node);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static struct platform_driver mtk_cqdma_driver = {
> > > +       .probe = mtk_cqdma_probe,
> > > +       .remove = mtk_cqdma_remove,
> > > +       .driver = {
> > > +               .name           = KBUILD_MODNAME,
> > > +               .of_match_table = mtk_cqdma_match,
> > > +       },
> > > +};
> > > +module_platform_driver(mtk_cqdma_driver);
> > > +
> > > +MODULE_DESCRIPTION("MediaTek CQDMA Controller Driver");
> > > +MODULE_AUTHOR("Shun-Chih Yu <shun-chih.yu@mediatek.com>");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 1.7.9.5
> > >
> > >
> > > _______________________________________________
> > > Linux-mediatek mailing list
> > > Linux-mediatek@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel