* [18/20] dmaengine: sa11x0: drop useless LIST_HEAD
From: Vinod Koul @ 2019-01-02 6:04 UTC (permalink / raw)
To: Julia Lawall; +Cc: Dan Williams, kernel-janitors, dmaengine, linux-kernel
On 23-12-18, 09:57, Julia Lawall wrote:
> Drop LIST_HEAD where the variable it declares has never been
> used.
>
> The semantic patch that fixes this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> identifier x;
> @@
> - LIST_HEAD(x);
> ... when != x
> // </smpl>
Applied, thanks
^ permalink raw reply
* [07/20] dmaengine: at_hdmac: drop useless LIST_HEAD
From: Vinod Koul @ 2019-01-02 6:04 UTC (permalink / raw)
To: Julia Lawall
Cc: Ludovic Desroches, kernel-janitors, Dan Williams,
linux-arm-kernel, dmaengine, linux-kernel
On 23-12-18, 09:57, Julia Lawall wrote:
> Drop LIST_HEAD where the variable it declares is never used.
>
> tmp_list has been declared since the introduction of the driver
> and has never been used. The two declarations of list were
> introduced with the containing functions but were also not used.
>
> The semantic patch that fixes this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> identifier x;
> @@
> - LIST_HEAD(x);
> ... when != x
> // </smpl>
Applied with Ludovic's ack, thanks
^ permalink raw reply
* [2/3] dmaengine: Extend the k3dma driver binding
From: John Stultz @ 2019-01-02 18:00 UTC (permalink / raw)
To: h00249924
Cc: dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-arm-kernel, Mark Rutland, wwx575822, hanxiaolong3,
Zhuangluan Su, Hantanglei, Kongfei, ninggaoyu, Vinod Koul,
Rob Herring, Liyuequan, huangli (I), wangyoulin, Qianli (A)
On Thu, Dec 27, 2018 at 10:39 PM h00249924 <hutenghui@huawei.com> wrote:
>
> From: Youlin Wang <wwx575822@notesmail.huawei.com>
>
> Extend the k3dma driver binding to support hisi-asp hardware variants.
>
> Signed-off-by: Youlin Wang <wwx575822@notesmail.huawei.com>
> Signed-off-by: Tanglei Han <hantanglei@huawei.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
> Documentation/devicetree/bindings/dma/k3dma.txt | 33 ++++++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> index 4945aea..cd21b82 100644
> --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> @@ -3,7 +3,9 @@
> See dma.txt first
>
> Required properties:
> -- compatible: Should be "hisilicon,k3-dma-1.0"
> +- compatible: Must be one of
> +- "hisilicon,k3-dma-1.0"
> +- "hisilicon,hisi-pcm-asp-dma-1.0"
> - reg: Should contain DMA registers location and length.
> - interrupts: Should contain one interrupt shared by all channel
> - #dma-cells: see dma.txt, should be 1, para number
> @@ -43,3 +45,32 @@ For example, i2c0 read channel request line is 18, while write channel use 19
> dma-names = "rx", "tx";
> };
>
> +
> +
> +
> +Controller:
> + asp_dmac: asp_dmac@E804B000 {
> + compatible = "hisilicon,hisi-pcm-asp-dma-1.0";
> + reg = <0x0 0xe804b000 0x0 0x1000>;
> + #dma-cells = <1>;
> + dma-channels = <16>;
> + dma-requests = <32>;
> + dma-min-chan = <0>;
> + dma-used-chans = <0xFFFE>;
> + dma-share;
Thanks for sending this out!
So min-chan, used-chans and dma-share aren't in the existing binding
document. So they probably should be removed from this example, or the
binding document needs to be updated first.
thanks
-john
^ permalink raw reply
* [2/2] dmaengine: mediatek: Add MediaTek Command-Queue DMA controller for MT6765 SoC
From: Sean Wang @ 2019-01-02 20:17 UTC (permalink / raw)
To: shun-chih.yu
Cc: Sean Wang, Vinod Koul, Rob Herring, Matthias Brugger,
Dan Williams, devicetree, linux-kernel, srv_wsdupstream,
linux-mediatek, dmaengine, linux-arm-kernel
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
>
> > 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
>
> > +#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
>
> > +};
> > +
> > +/**
> > + * 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?
>
> > +}
> > +
> > +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.
>
> > +}
> > +
> > +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.
>
> > +
> > + /* 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.
>
> > + /* 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
>
> > +
> > + /* 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.
>
> > + }
> > +}
> > +
> > +/*
> > + * 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
>
> > +
> > + 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.
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.
> > + /* 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.
> > + 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.
>
> > + }
> > +
> > + 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.
> > +
> > + 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
> > + /* 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.
> > + 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.
> > + }
> > + 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
>
> > + 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
>
> > + 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
^ permalink raw reply
* [v9,1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Nicolas Boichat @ 2019-01-03 1:39 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Matthias Brugger, Dan Williams, Greg Kroah-Hartman,
Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm Mailing List, linux-mediatek, lkml, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen, Zhenbao Liu
On Wed, Jan 2, 2019 at 10:13 AM Long Cheng <long.cheng@mediatek.com> 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 <long.cheng@mediatek.com>
> ---
> 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 <long.cheng@mediatek.com>
> + */
> +
> +#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 <long.cheng@mediatek.com>");
> +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
>
^ permalink raw reply
* [v9,1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Randy Dunlap @ 2019-01-03 2:03 UTC (permalink / raw)
To: Nicolas Boichat, Long Cheng
Cc: Vinod Koul, Rob Herring, Mark Rutland, Ryder Lee, Sean Wang,
Matthias Brugger, Dan Williams, Greg Kroah-Hartman, Jiri Slaby,
Sean Wang, dmaengine, devicetree, linux-arm Mailing List,
linux-mediatek, lkml, linux-serial, srv_heupstream, Yingjoe Chen,
YT Shen, Zhenbao Liu
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,
^ permalink raw reply
* [RFC,1/6] dma: Add Synopsys eDMA IP core driver
From: Gustavo Pimentel @ 2019-01-03 9:53 UTC (permalink / raw)
To: Vinod Koul, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Eugeniy Paltsev, Andy Shevchenko, Joao Pinto
Hi Vinod,
(snipped)
>>> +{
>>> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>>> + const struct dw_edma_core_ops *ops = chan2ops(chan);
>>> + enum dma_transfer_direction dir;
>>> + unsigned long flags;
>>> + int err = 0;
>>> +
>>> + spin_lock_irqsave(&chan->vc.lock, flags);
>>> +
>>> + if (!config) {
>>> + err = -EINVAL;
>>> + goto err_config;
>>> + }
>>> +
>>> + if (chan->configured) {
>>> + dev_err(chan2dev(chan), ": channel already configured\n");
>>> + err = -EPERM;
>>> + goto err_config;
>>> + }
>>> +
>>> + dir = config->direction;
>>
>> Direction is depreciated, I have already removed the usages, so please
>> do not add new ones.
>>
>> You need to take direction for respective prep_ calls
>
> Ok, I already do that. IMHO I found it strange to have the same information
> repeated on two places. But now that you say that this is deprecated, it makes
> sense now.
>
>>
>>> + if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
>>> + dev_info(chan2dev(chan),
>>> + ": direction DMA_DEV_TO_MEM (EDMA_DIR_WRITE)\n");
>>> + chan->p_addr = config->src_addr;
>>> + } else if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
>>> + dev_info(chan2dev(chan),
>>> + ": direction DMA_MEM_TO_DEV (EDMA_DIR_READ)\n");
>>> + chan->p_addr = config->dst_addr;
>>> + } else {
>>> + dev_err(chan2dev(chan), ": invalid direction\n");
>>> + err = -EINVAL;
>>> + goto err_config;
>>> + }
>>
>> This should be removed
>
> Yeah, it was just for validation purposes. Now that direction is deprecated on
> the API, makes no sense to validate it.
>
>>
>>> +
>>> + dev_info(chan2dev(chan),
>>> + ": src_addr(physical) = 0x%.16x\n", config->src_addr);
>>> + dev_info(chan2dev(chan),
>>> + ": dst_addr(physical) = 0x%.16x\n", config->dst_addr);
>>
I've a doubt now. As you know, for a DMA transfer you need the source and
destination addresses, which in the limited can be swapped according to the
direction MEM_TO_DEV/DEV_TO_MEM case.
For the sake of simplicity, I'll just consider now the MEM_TO_DEV case, since
the other case is similar but the source and destination address are swapped.
In my code I can get some of the information that I need by using the
sg_dma_address() in the scatter-gather list (which gives me the source address).
The remaining information I got from here, using the direction to help me to
select which address I'll use later on the DMA transfer, in this case the
destination address.
Since this is deprecated how should I proceed? How can I get that information?
There is some similar function to sg_dma_address() that could give me the
destination address?
Regards,
Gustavo
^ permalink raw reply
* [RESEND,v1,1/3] dmaengine: stm32-dma: Add PM Runtime support
From: Pierre Yves MORDRET @ 2019-01-03 10:17 UTC (permalink / raw)
To: Dan Williams, Vinod Koul, Maxime Coquelin, Alexandre Torgue,
dmaengine, linux-stm32, linux-arm-kernel, linux-kernel
Cc: Pierre-Yves MORDRET
Use pm_runtime engine for clock management purpose.
Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>
---
Version history:
v1:
* Initial
---
---
drivers/dma/stm32-dma.c | 58 +++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c
index 48f7c0f..ba239b5 100644
--- a/drivers/dma/stm32-dma.c
+++ b/drivers/dma/stm32-dma.c
@@ -23,6 +23,7 @@
#include <linux/of_device.h>
#include <linux/of_dma.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/reset.h>
#include <linux/sched.h>
#include <linux/slab.h>
@@ -1115,15 +1116,14 @@ static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
int ret;
chan->config_init = false;
- ret = clk_prepare_enable(dmadev->clk);
- if (ret < 0) {
- dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n", ret);
+
+ ret = pm_runtime_get_sync(dmadev->ddev.dev);
+ if (ret < 0)
return ret;
- }
ret = stm32_dma_disable_chan(chan);
if (ret < 0)
- clk_disable_unprepare(dmadev->clk);
+ pm_runtime_put(dmadev->ddev.dev);
return ret;
}
@@ -1143,7 +1143,7 @@ static void stm32_dma_free_chan_resources(struct dma_chan *c)
spin_unlock_irqrestore(&chan->vchan.lock, flags);
}
- clk_disable_unprepare(dmadev->clk);
+ pm_runtime_put(dmadev->ddev.dev);
vchan_free_chan_resources(to_virt_chan(c));
}
@@ -1243,6 +1243,12 @@ static int stm32_dma_probe(struct platform_device *pdev)
return PTR_ERR(dmadev->clk);
}
+ ret = clk_prepare_enable(dmadev->clk);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "clk_prep_enable error: %d\n", ret);
+ return ret;
+ }
+
dmadev->mem2mem = of_property_read_bool(pdev->dev.of_node,
"st,mem2mem");
@@ -1292,7 +1298,7 @@ static int stm32_dma_probe(struct platform_device *pdev)
ret = dma_async_device_register(dd);
if (ret)
- return ret;
+ goto clk_free;
for (i = 0; i < STM32_DMA_MAX_CHANNELS; i++) {
chan = &dmadev->chan[i];
@@ -1324,20 +1330,58 @@ static int stm32_dma_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, dmadev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_get_noresume(&pdev->dev);
+ pm_runtime_put(&pdev->dev);
+
dev_info(&pdev->dev, "STM32 DMA driver registered\n");
return 0;
err_unregister:
dma_async_device_unregister(dd);
+clk_free:
+ clk_disable_unprepare(dmadev->clk);
return ret;
}
+#ifdef CONFIG_PM
+static int stm32_dma_runtime_suspend(struct device *dev)
+{
+ struct stm32_dma_device *dmadev = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(dmadev->clk);
+
+ return 0;
+}
+
+static int stm32_dma_runtime_resume(struct device *dev)
+{
+ struct stm32_dma_device *dmadev = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(dmadev->clk);
+ if (ret) {
+ dev_err(dev, "failed to prepare_enable clock\n");
+ return ret;
+ }
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops stm32_dma_pm_ops = {
+ SET_RUNTIME_PM_OPS(stm32_dma_runtime_suspend,
+ stm32_dma_runtime_resume, NULL)
+};
+
static struct platform_driver stm32_dma_driver = {
.driver = {
.name = "stm32-dma",
.of_match_table = stm32_dma_of_match,
+ .pm = &stm32_dma_pm_ops,
},
};
^ permalink raw reply related
* [RESEND,v1,2/3] dmaengine: stm32-dmamux: Add PM Runtime support
From: Pierre Yves MORDRET @ 2019-01-03 10:17 UTC (permalink / raw)
To: Dan Williams, Vinod Koul, Maxime Coquelin, Alexandre Torgue,
dmaengine, linux-stm32, linux-arm-kernel, linux-kernel
Cc: Pierre-Yves MORDRET
Use pm_runtime engine for clock management purpose.
Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>
---
Version history:
v1:
* Initial
---
---
drivers/dma/stm32-dmamux.c | 58 +++++++++++++++++++++++++++++++++++++---------
1 file changed, 47 insertions(+), 11 deletions(-)
diff --git a/drivers/dma/stm32-dmamux.c b/drivers/dma/stm32-dmamux.c
index b922db9..a671191 100644
--- a/drivers/dma/stm32-dmamux.c
+++ b/drivers/dma/stm32-dmamux.c
@@ -28,6 +28,7 @@
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/of_dma.h>
+#include <linux/pm_runtime.h>
#include <linux/reset.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
@@ -79,8 +80,7 @@ static void stm32_dmamux_free(struct device *dev, void *route_data)
stm32_dmamux_write(dmamux->iomem, STM32_DMAMUX_CCR(mux->chan_id), 0);
clear_bit(mux->chan_id, dmamux->dma_inuse);
- if (!IS_ERR(dmamux->clk))
- clk_disable(dmamux->clk);
+ pm_runtime_put_sync(dev);
spin_unlock_irqrestore(&dmamux->lock, flags);
@@ -146,13 +146,10 @@ static void *stm32_dmamux_route_allocate(struct of_phandle_args *dma_spec,
/* Set dma request */
spin_lock_irqsave(&dmamux->lock, flags);
- if (!IS_ERR(dmamux->clk)) {
- ret = clk_enable(dmamux->clk);
- if (ret < 0) {
- spin_unlock_irqrestore(&dmamux->lock, flags);
- dev_err(&pdev->dev, "clk_prep_enable issue: %d\n", ret);
- goto error;
- }
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ spin_unlock_irqrestore(&dmamux->lock, flags);
+ goto error;
}
spin_unlock_irqrestore(&dmamux->lock, flags);
@@ -254,6 +251,7 @@ static int stm32_dmamux_probe(struct platform_device *pdev)
dev_warn(&pdev->dev, "DMAMUX defaulting on %u requests\n",
stm32_dmamux->dmamux_requests);
}
+ pm_runtime_get_noresume(&pdev->dev);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
iomem = devm_ioremap_resource(&pdev->dev, res);
@@ -282,6 +280,8 @@ static int stm32_dmamux_probe(struct platform_device *pdev)
stm32_dmamux->dmarouter.route_free = stm32_dmamux_free;
platform_set_drvdata(pdev, stm32_dmamux);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
if (!IS_ERR(stm32_dmamux->clk)) {
ret = clk_prepare_enable(stm32_dmamux->clk);
@@ -291,17 +291,52 @@ static int stm32_dmamux_probe(struct platform_device *pdev)
}
}
+ pm_runtime_get_noresume(&pdev->dev);
+
/* Reset the dmamux */
for (i = 0; i < stm32_dmamux->dma_requests; i++)
stm32_dmamux_write(stm32_dmamux->iomem, STM32_DMAMUX_CCR(i), 0);
- if (!IS_ERR(stm32_dmamux->clk))
- clk_disable(stm32_dmamux->clk);
+ pm_runtime_put(&pdev->dev);
return of_dma_router_register(node, stm32_dmamux_route_allocate,
&stm32_dmamux->dmarouter);
}
+#ifdef CONFIG_PM
+static int stm32_dmamux_runtime_suspend(struct device *dev)
+{
+ struct platform_device *pdev =
+ container_of(dev, struct platform_device, dev);
+ struct stm32_dmamux_data *stm32_dmamux = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(stm32_dmamux->clk);
+
+ return 0;
+}
+
+static int stm32_dmamux_runtime_resume(struct device *dev)
+{
+ struct platform_device *pdev =
+ container_of(dev, struct platform_device, dev);
+ struct stm32_dmamux_data *stm32_dmamux = platform_get_drvdata(pdev);
+ int ret;
+
+ ret = clk_prepare_enable(stm32_dmamux->clk);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to prepare_enable clock\n");
+ return ret;
+ }
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops stm32_dmamux_pm_ops = {
+ SET_RUNTIME_PM_OPS(stm32_dmamux_runtime_suspend,
+ stm32_dmamux_runtime_resume, NULL)
+};
+
static const struct of_device_id stm32_dmamux_match[] = {
{ .compatible = "st,stm32h7-dmamux" },
{},
@@ -312,6 +347,7 @@ static struct platform_driver stm32_dmamux_driver = {
.driver = {
.name = "stm32-dmamux",
.of_match_table = stm32_dmamux_match,
+ .pm = &stm32_dmamux_pm_ops,
},
};
^ permalink raw reply related
* [RESEND,v1,3/3] dmaengine: stm32-mdma: Add PM Runtime support
From: Pierre Yves MORDRET @ 2019-01-03 10:17 UTC (permalink / raw)
To: Dan Williams, Vinod Koul, Maxime Coquelin, Alexandre Torgue,
dmaengine, linux-stm32, linux-arm-kernel, linux-kernel
Cc: Pierre-Yves MORDRET
Use pm_runtime engine for clock management purpose
Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>
---
Version history:
v1:
* Initial
---
---
drivers/dma/stm32-mdma.c | 52 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 46 insertions(+), 6 deletions(-)
diff --git a/drivers/dma/stm32-mdma.c b/drivers/dma/stm32-mdma.c
index 390e4ca..ac0301b 100644
--- a/drivers/dma/stm32-mdma.c
+++ b/drivers/dma/stm32-mdma.c
@@ -37,6 +37,7 @@
#include <linux/of_device.h>
#include <linux/of_dma.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/reset.h>
#include <linux/slab.h>
@@ -1456,15 +1457,13 @@ static int stm32_mdma_alloc_chan_resources(struct dma_chan *c)
return -ENOMEM;
}
- ret = clk_prepare_enable(dmadev->clk);
- if (ret < 0) {
- dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n", ret);
+ ret = pm_runtime_get_sync(dmadev->ddev.dev);
+ if (ret < 0)
return ret;
- }
ret = stm32_mdma_disable_chan(chan);
if (ret < 0)
- clk_disable_unprepare(dmadev->clk);
+ pm_runtime_put(dmadev->ddev.dev);
return ret;
}
@@ -1484,7 +1483,7 @@ static void stm32_mdma_free_chan_resources(struct dma_chan *c)
spin_unlock_irqrestore(&chan->vchan.lock, flags);
}
- clk_disable_unprepare(dmadev->clk);
+ pm_runtime_put(dmadev->ddev.dev);
vchan_free_chan_resources(to_virt_chan(c));
dmam_pool_destroy(chan->desc_pool);
chan->desc_pool = NULL;
@@ -1597,6 +1596,12 @@ static int stm32_mdma_probe(struct platform_device *pdev)
return ret;
}
+ ret = clk_prepare_enable(dmadev->clk);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "clk_prep_enable error: %d\n", ret);
+ return ret;
+ }
+
dmadev->rst = devm_reset_control_get(&pdev->dev, NULL);
if (!IS_ERR(dmadev->rst)) {
reset_control_assert(dmadev->rst);
@@ -1668,6 +1673,10 @@ static int stm32_mdma_probe(struct platform_device *pdev)
}
platform_set_drvdata(pdev, dmadev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_get_noresume(&pdev->dev);
+ pm_runtime_put(&pdev->dev);
dev_info(&pdev->dev, "STM32 MDMA driver registered\n");
@@ -1677,11 +1686,42 @@ static int stm32_mdma_probe(struct platform_device *pdev)
return ret;
}
+#ifdef CONFIG_PM
+static int stm32_mdma_runtime_suspend(struct device *dev)
+{
+ struct stm32_mdma_device *dmadev = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(dmadev->clk);
+
+ return 0;
+}
+
+static int stm32_mdma_runtime_resume(struct device *dev)
+{
+ struct stm32_mdma_device *dmadev = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(dmadev->clk);
+ if (ret) {
+ dev_err(dev, "failed to prepare_enable clock\n");
+ return ret;
+ }
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops stm32_mdma_pm_ops = {
+ SET_RUNTIME_PM_OPS(stm32_mdma_runtime_suspend,
+ stm32_mdma_runtime_resume, NULL)
+};
+
static struct platform_driver stm32_mdma_driver = {
.probe = stm32_mdma_probe,
.driver = {
.name = "stm32-mdma",
.of_match_table = stm32_mdma_of_match,
+ .pm = &stm32_mdma_pm_ops,
},
};
^ permalink raw reply related
* [RESEND,v1] dmaengine: stm32-dma: check FIFO error interrupt enable
From: Pierre Yves MORDRET @ 2019-01-03 10:17 UTC (permalink / raw)
To: Dan Williams, Vinod Koul, Maxime Coquelin, Alexandre Torgue,
dmaengine, linux-stm32, linux-arm-kernel, linux-kernel
Cc: Pierre-Yves MORDRET
For avoiding false FIFO detection, check FIFO Error interrupt is
enabled prior raising any errors.
This will prevent having spurious FIFO error where it shouldn't.
Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>
---
Version history:
v1:
* Initial
---
---
drivers/dma/stm32-dma.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c
index 4903a40..48f7c0f 100644
--- a/drivers/dma/stm32-dma.c
+++ b/drivers/dma/stm32-dma.c
@@ -641,12 +641,13 @@ static irqreturn_t stm32_dma_chan_irq(int irq, void *devid)
{
struct stm32_dma_chan *chan = devid;
struct stm32_dma_device *dmadev = stm32_dma_get_dev(chan);
- u32 status, scr;
+ u32 status, scr, sfcr;
spin_lock(&chan->vchan.lock);
status = stm32_dma_irq_status(chan);
scr = stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id));
+ sfcr = stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id));
if (status & STM32_DMA_TCI) {
stm32_dma_irq_clear(chan, STM32_DMA_TCI);
@@ -661,10 +662,12 @@ static irqreturn_t stm32_dma_chan_irq(int irq, void *devid)
if (status & STM32_DMA_FEI) {
stm32_dma_irq_clear(chan, STM32_DMA_FEI);
status &= ~STM32_DMA_FEI;
- if (!(scr & STM32_DMA_SCR_EN))
- dev_err(chan2dev(chan), "FIFO Error\n");
- else
- dev_dbg(chan2dev(chan), "FIFO over/underrun\n");
+ if (sfcr & STM32_DMA_SFCR_FEIE) {
+ if (!(scr & STM32_DMA_SCR_EN))
+ dev_err(chan2dev(chan), "FIFO Error\n");
+ else
+ dev_dbg(chan2dev(chan), "FIFO over/underrun\n");
+ }
}
if (status) {
stm32_dma_irq_clear(chan, status);
^ permalink raw reply related
* [RFC,1/6] dma: Add Synopsys eDMA IP core driver
From: Vinod Koul @ 2019-01-03 12:45 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Eugeniy Paltsev, Andy Shevchenko, Joao Pinto
Hi Gustavo,
On 03-01-19, 09:53, Gustavo Pimentel wrote:
> Hi Vinod,
>
> (snipped)
>
> >>> +{
> >>> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> >>> + const struct dw_edma_core_ops *ops = chan2ops(chan);
> >>> + enum dma_transfer_direction dir;
> >>> + unsigned long flags;
> >>> + int err = 0;
> >>> +
> >>> + spin_lock_irqsave(&chan->vc.lock, flags);
> >>> +
> >>> + if (!config) {
> >>> + err = -EINVAL;
> >>> + goto err_config;
> >>> + }
> >>> +
> >>> + if (chan->configured) {
> >>> + dev_err(chan2dev(chan), ": channel already configured\n");
> >>> + err = -EPERM;
> >>> + goto err_config;
> >>> + }
> >>> +
> >>> + dir = config->direction;
> >>
> >> Direction is depreciated, I have already removed the usages, so please
> >> do not add new ones.
> >>
> >> You need to take direction for respective prep_ calls
> >
> > Ok, I already do that. IMHO I found it strange to have the same information
> > repeated on two places. But now that you say that this is deprecated, it makes
> > sense now.
> >
> >>
> >>> + if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
> >>> + dev_info(chan2dev(chan),
> >>> + ": direction DMA_DEV_TO_MEM (EDMA_DIR_WRITE)\n");
> >>> + chan->p_addr = config->src_addr;
> >>> + } else if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
> >>> + dev_info(chan2dev(chan),
> >>> + ": direction DMA_MEM_TO_DEV (EDMA_DIR_READ)\n");
> >>> + chan->p_addr = config->dst_addr;
> >>> + } else {
> >>> + dev_err(chan2dev(chan), ": invalid direction\n");
> >>> + err = -EINVAL;
> >>> + goto err_config;
> >>> + }
> >>
> >> This should be removed
> >
> > Yeah, it was just for validation purposes. Now that direction is deprecated on
> > the API, makes no sense to validate it.
> >
> >>
> >>> +
> >>> + dev_info(chan2dev(chan),
> >>> + ": src_addr(physical) = 0x%.16x\n", config->src_addr);
> >>> + dev_info(chan2dev(chan),
> >>> + ": dst_addr(physical) = 0x%.16x\n", config->dst_addr);
> >>
>
> I've a doubt now. As you know, for a DMA transfer you need the source and
> destination addresses, which in the limited can be swapped according to the
> direction MEM_TO_DEV/DEV_TO_MEM case.
>
> For the sake of simplicity, I'll just consider now the MEM_TO_DEV case, since
> the other case is similar but the source and destination address are swapped.
>
> In my code I can get some of the information that I need by using the
> sg_dma_address() in the scatter-gather list (which gives me the source address).
>
> The remaining information I got from here, using the direction to help me to
> select which address I'll use later on the DMA transfer, in this case the
> destination address.
>
> Since this is deprecated how should I proceed? How can I get that information?
> There is some similar function to sg_dma_address() that could give me the
> destination address?
So the direction field is deprecated but rest of the configuration comes
from from dma_slave_config. The user should set src_addr, dst_addr and
then based on direction passed in the .device_prep_dma_* call arguments
one can use one of these as peripheral address.
Also, please keep in mind that for memory to memory transfers you should
not expect the dma_slave_config to be used
Thanks
^ permalink raw reply
* [RFC,1/6] dma: Add Synopsys eDMA IP core driver
From: Gustavo Pimentel @ 2019-01-03 12:55 UTC (permalink / raw)
To: Vinod Koul, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Eugeniy Paltsev, Andy Shevchenko, Joao Pinto
Hi Vinod,
On 03/01/2019 12:45, Vinod Koul wrote:
> Hi Gustavo,
>
> On 03-01-19, 09:53, Gustavo Pimentel wrote:
>> Hi Vinod,
>>
>> (snipped)
>>
>>>>> +{
>>>>> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>>>>> + const struct dw_edma_core_ops *ops = chan2ops(chan);
>>>>> + enum dma_transfer_direction dir;
>>>>> + unsigned long flags;
>>>>> + int err = 0;
>>>>> +
>>>>> + spin_lock_irqsave(&chan->vc.lock, flags);
>>>>> +
>>>>> + if (!config) {
>>>>> + err = -EINVAL;
>>>>> + goto err_config;
>>>>> + }
>>>>> +
>>>>> + if (chan->configured) {
>>>>> + dev_err(chan2dev(chan), ": channel already configured\n");
>>>>> + err = -EPERM;
>>>>> + goto err_config;
>>>>> + }
>>>>> +
>>>>> + dir = config->direction;
>>>>
>>>> Direction is depreciated, I have already removed the usages, so please
>>>> do not add new ones.
>>>>
>>>> You need to take direction for respective prep_ calls
>>>
>>> Ok, I already do that. IMHO I found it strange to have the same information
>>> repeated on two places. But now that you say that this is deprecated, it makes
>>> sense now.
>>>
>>>>
>>>>> + if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
>>>>> + dev_info(chan2dev(chan),
>>>>> + ": direction DMA_DEV_TO_MEM (EDMA_DIR_WRITE)\n");
>>>>> + chan->p_addr = config->src_addr;
>>>>> + } else if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
>>>>> + dev_info(chan2dev(chan),
>>>>> + ": direction DMA_MEM_TO_DEV (EDMA_DIR_READ)\n");
>>>>> + chan->p_addr = config->dst_addr;
>>>>> + } else {
>>>>> + dev_err(chan2dev(chan), ": invalid direction\n");
>>>>> + err = -EINVAL;
>>>>> + goto err_config;
>>>>> + }
>>>>
>>>> This should be removed
>>>
>>> Yeah, it was just for validation purposes. Now that direction is deprecated on
>>> the API, makes no sense to validate it.
>>>
>>>>
>>>>> +
>>>>> + dev_info(chan2dev(chan),
>>>>> + ": src_addr(physical) = 0x%.16x\n", config->src_addr);
>>>>> + dev_info(chan2dev(chan),
>>>>> + ": dst_addr(physical) = 0x%.16x\n", config->dst_addr);
>>>>
>>
>> I've a doubt now. As you know, for a DMA transfer you need the source and
>> destination addresses, which in the limited can be swapped according to the
>> direction MEM_TO_DEV/DEV_TO_MEM case.
>>
>> For the sake of simplicity, I'll just consider now the MEM_TO_DEV case, since
>> the other case is similar but the source and destination address are swapped.
>>
>> In my code I can get some of the information that I need by using the
>> sg_dma_address() in the scatter-gather list (which gives me the source address).
>>
>> The remaining information I got from here, using the direction to help me to
>> select which address I'll use later on the DMA transfer, in this case the
>> destination address.
>>
>> Since this is deprecated how should I proceed? How can I get that information?
>> There is some similar function to sg_dma_address() that could give me the
>> destination address?
>
> So the direction field is deprecated but rest of the configuration comes
> from from dma_slave_config. The user should set src_addr, dst_addr and
> then based on direction passed in the .device_prep_dma_* call arguments
> one can use one of these as peripheral address.
Ok, and the address given by sg_dma_address()? Is redundant or not applicable
for this case?
>
> Also, please keep in mind that for memory to memory transfers you should
> not expect the dma_slave_config to be used
My DMA implementation is only MEM_TO_DEV and DEV_TO_MEM.
>
> Thanks
>
Thanks.
^ permalink raw reply
* [RFC,1/6] dma: Add Synopsys eDMA IP core driver
From: Vinod Koul @ 2019-01-03 14:53 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Eugeniy Paltsev, Andy Shevchenko, Joao Pinto
HI Gustavo,
On 03-01-19, 12:55, Gustavo Pimentel wrote:
> On 03/01/2019 12:45, Vinod Koul wrote:
> > On 03-01-19, 09:53, Gustavo Pimentel wrote:
> >> I've a doubt now. As you know, for a DMA transfer you need the source and
> >> destination addresses, which in the limited can be swapped according to the
> >> direction MEM_TO_DEV/DEV_TO_MEM case.
> >>
> >> For the sake of simplicity, I'll just consider now the MEM_TO_DEV case, since
> >> the other case is similar but the source and destination address are swapped.
> >>
> >> In my code I can get some of the information that I need by using the
> >> sg_dma_address() in the scatter-gather list (which gives me the source address).
> >>
> >> The remaining information I got from here, using the direction to help me to
> >> select which address I'll use later on the DMA transfer, in this case the
> >> destination address.
> >>
> >> Since this is deprecated how should I proceed? How can I get that information?
> >> There is some similar function to sg_dma_address() that could give me the
> >> destination address?
> >
> > So the direction field is deprecated but rest of the configuration comes
> > from from dma_slave_config. The user should set src_addr, dst_addr and
> > then based on direction passed in the .device_prep_dma_* call arguments
> > one can use one of these as peripheral address.
>
> Ok, and the address given by sg_dma_address()? Is redundant or not applicable
> for this case?
It is, it is the memory address you need to program. The device address
comes from dma_slave_config.
To elaborate, if you have MEM_TO_DEV use sg_dma_address() for src
address and dst_addr from dma_slave_config.
Similarly for DEV_TO_MEM, we use src_addr from dma_slave_config and
sg_dma_address()
The peripheral is static but memory buffer keeps changing wrt different
descriptors, so device address can be programmed once for multiple
descriptors to be transferred from a device.
HTH
^ permalink raw reply
* [RFC,4/6] dma: Add Synopsys eDMA IP PCIe glue-logic
From: Andy Shevchenko @ 2019-01-03 17:35 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Eugeniy Paltsev, Lorenzo Pieralisi, Joao Pinto
On Thu, Dec 13, 2018 at 02:40:40PM +0000, Gustavo Pimentel wrote:
> On 12/12/2018 13:25, Andy Shevchenko wrote:
> > On Wed, Dec 12, 2018 at 12:13:24PM +0100, Gustavo Pimentel wrote:
> > It seems this driver somehow written as a copy-paste of existing pieces w/o good reasons to do such.
>
(1)
> What do you mean? It's true I relied on existing drivers to develop this
> solution, but is not it supposed to be so?
See below.
> >> +enum dw_edma_pcie_bar {
> >> + BAR_0,
> >> + BAR_1,
> >> + BAR_2,
> >> + BAR_3,
> >> + BAR_4,
> >> + BAR_5
> >> +};
> >
> > Why do you need this at all?
>
> See my answer below.
>
> >
> >> +static const struct dw_edma_pcie_data snps_edda_data = {
> >> + // eDMA registers location
> >> + .regs_bar = BAR_0,
> >> + .regs_off = 0x1000, // 4 KBytes
> >> + // eDMA memory linked list location
> >> + .ll_bar = BAR_2,
> >> + .ll_off = 0, // 0 KBytes
> >> + .ll_sz = 0x20000, // 128 KBytes
> >> + // Other
> >> + .version = 0,
> >> + .mode = EDMA_MODE_UNROLL,
> >> +};
> >
> > Huh? Isn't this
>
> The eDMA is a hardware block (with requires some configuration) accessible
> through a PCIe EndPoint, unfortunately there isn't any way for now (at least)
> through an PCIe capability for instance to detect those configurations
> automatically, therefore the only way to pass that configuration is to associate
> it to PCIe Vendor(0x16c3) and Device (0xedda) Id.
>
> To interact with eDMA hardware block the driver needs to access the eDMA
> registers and the only way to do it is through PCIe mapping. In this those
> registers will be available on BAR 0, with a offset of 4KB from the start.
>
> The driver also requires a memory space (RAM) to create a linked list
> descriptors and pass the first descriptor address to the eDMA hardware block so
> that it can start performing the transfer autonomously. Once again this memory
> space is provide through PCIe on BAR 2. This linked list space is in the
> beginning and it's restricted to a size of 128KB.
>
> Since this eDMA hardware block (more specifically eDMA registers) can evolve in
> the future, I choosed to put here a version in order to have a driver that can
> be easily adaptable and reusable without much effort.
(2)
> >> + err = pci_try_set_mwi(pdev);
> >> + if (err) {
> >> + dev_err(dev, "%s DMA memory write invalidate\n",
> >> + pci_name(pdev));
> >> + return err;
> >> + }
> >
> > Are you sure you need this?
>
> I'm not sure, probably not. I saw using this being use on /dma/dw/pci.c driver.
> I'll test without it.
That's exactly the point what I referred to in (1) above.
Looks like here is cargo-cult programming done without understanding why.
I dunno how many other cases in the driver like this.
> >> + dev_info(dev, "Mode:\t%s\n",
> >> + dw->mode == EDMA_MODE_LEGACY ? "Legacy" : "Unroll");
> >> +
> >
> >
> >> + dev_info(dev, "Registers:\tBAR=%u, off=0x%.16llx B, addr=0x%.8lx\n",
> >> + pdata->regs_bar, pdata->regs_off,
> >> + (unsigned long) dw->regs);
> >
> > Oh, no, don't do casting when printing something. In only rare cases it's needed, not here.
>
> I've put the cast to remove the following warning:
>
> drivers/dma/dw-edma/dw-edma-pcie.c:138:15: warning: format ‘%lx’ expects
> argument of type ‘long unsigned int’, but argument 6 has type ‘void *’
...and as I said this is wrong. Please, find suitable specifiers for your case.
> >> + if (pdev->msi_cap && pdev->msi_enabled) {
> >> + cap_off = pdev->msi_cap + PCI_MSI_FLAGS;
> >> + pci_read_config_word(pdev, cap_off, &flags);
> >> + if (flags & PCI_MSI_FLAGS_ENABLE) {
> >> + cap_off = pdev->msi_cap + PCI_MSI_ADDRESS_LO;
> >> + pci_read_config_dword(pdev, cap_off, &addr_lo);
> >> +
> >> + if (flags & PCI_MSI_FLAGS_64BIT) {
> >> + cap_off = pdev->msi_cap + PCI_MSI_ADDRESS_HI;
> >> + pci_read_config_dword(pdev, cap_off, &addr_hi);
> >> + cap_off = pdev->msi_cap + PCI_MSI_DATA_64;
> >> + } else {
> >> + addr_hi = 0;
> >> + cap_off = pdev->msi_cap + PCI_MSI_DATA_32;
> >> + }
> >> +
> >> + dw->msi_addr = addr_hi;
> >> + dw->msi_addr <<= 32;
> >> + dw->msi_addr |= addr_lo;
> >> +
> >> + pci_read_config_dword(pdev, cap_off, &(dw->msi_data));
> >> + dw->msi_data &= 0xffff;
> >> +
> >> + dev_info(dev,
> >> + "MSI:\t\taddr=0x%.16llx, data=0x%.8x, nr=%d\n",
> >> + dw->msi_addr, dw->msi_data, pdev->irq);
> >> + }
> >> + }
> >> +
> >> + if (pdev->msix_cap && pdev->msix_enabled) {
> >> + u32 offset;
> >> + u8 bir;
> >> +
> >> + cap_off = pdev->msix_cap + PCI_MSIX_FLAGS;
> >> + pci_read_config_word(pdev, cap_off, &flags);
> >> +
> >> + if (flags & PCI_MSIX_FLAGS_ENABLE) {
> >> + cap_off = pdev->msix_cap + PCI_MSIX_TABLE;
> >> + pci_read_config_dword(pdev, cap_off, &offset);
> >> +
> >> + bir = offset & PCI_MSIX_TABLE_BIR;
> >> + offset &= PCI_MSIX_TABLE_OFFSET;
> >> +
> >> + reg = pcim_iomap_table(pdev)[bir];
> >> + reg += offset;
> >> +
> >> + addr_lo = readl(reg + PCI_MSIX_ENTRY_LOWER_ADDR);
> >> + addr_hi = readl(reg + PCI_MSIX_ENTRY_UPPER_ADDR);
> >> + dw->msi_addr = addr_hi;
> >> + dw->msi_addr <<= 32;
> >> + dw->msi_addr |= addr_lo;
> >> +
> >> + dw->msi_data = readl(reg + PCI_MSIX_ENTRY_DATA);
> >> +
> >> + dev_info(dev,
> >> + "MSI-X:\taddr=0x%.16llx, data=0x%.8x, nr=%d\n",
> >> + dw->msi_addr, dw->msi_data, pdev->irq);
> >> + }
> >> + }
> >
> > What is this? Why?
>
> I need to find the PCIe interrupt vector address and value (either for MSI or
> MSI-X, depends what the system has selected) in order to configure eDMA later to
> generate the (done or abort) interruptions.
It seems rather hackish way to do something which should be done by PCI/driver core.
But perhaps Bjorn and others familiar with PCI endpoint facility can share more.
Also this applies to (2) above.
> >> + if (!pdev->msi_enabled && !pdev->msix_enabled) {
>
> I'm not finding it. Can you tell me what it is?
>
> Note: pci_msi_enabled() on msi.c returns if MSI has not been disabled by the
> command-line option pci=nomsi
Can you look a bit more carefully? For example, assume that 'dev' (sub)prefix
in the name of function would tell something about scope of application.
pci_dev_...() ?
> > There is a helper from PCI core for this.
^ permalink raw reply
* [RFC,4/6] dma: Add Synopsys eDMA IP PCIe glue-logic
From: Gustavo Pimentel @ 2019-01-03 18:00 UTC (permalink / raw)
To: Andy Shevchenko, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Eugeniy Paltsev, Lorenzo Pieralisi, Joao Pinto
On 03/01/2019 17:35, Andy Shevchenko wrote:
> On Thu, Dec 13, 2018 at 02:40:40PM +0000, Gustavo Pimentel wrote:
>> On 12/12/2018 13:25, Andy Shevchenko wrote:
>>> On Wed, Dec 12, 2018 at 12:13:24PM +0100, Gustavo Pimentel wrote:
>
>>> It seems this driver somehow written as a copy-paste of existing pieces w/o good reasons to do such.
>>
>
> (1)
>
>> What do you mean? It's true I relied on existing drivers to develop this
>> solution, but is not it supposed to be so?
>
> See below.
>
>>>> +enum dw_edma_pcie_bar {
>>>> + BAR_0,
>>>> + BAR_1,
>>>> + BAR_2,
>>>> + BAR_3,
>>>> + BAR_4,
>>>> + BAR_5
>>>> +};
>>>
>>> Why do you need this at all?
>>
>> See my answer below.
>>
>>>
>>>> +static const struct dw_edma_pcie_data snps_edda_data = {
>>>> + // eDMA registers location
>>>> + .regs_bar = BAR_0,
>>>> + .regs_off = 0x1000, // 4 KBytes
>>>> + // eDMA memory linked list location
>>>> + .ll_bar = BAR_2,
>>>> + .ll_off = 0, // 0 KBytes
>>>> + .ll_sz = 0x20000, // 128 KBytes
>>>> + // Other
>>>> + .version = 0,
>>>> + .mode = EDMA_MODE_UNROLL,
>>>> +};
>>>
>>> Huh? Isn't this
>>
>> The eDMA is a hardware block (with requires some configuration) accessible
>> through a PCIe EndPoint, unfortunately there isn't any way for now (at least)
>> through an PCIe capability for instance to detect those configurations
>> automatically, therefore the only way to pass that configuration is to associate
>> it to PCIe Vendor(0x16c3) and Device (0xedda) Id.
>>
>> To interact with eDMA hardware block the driver needs to access the eDMA
>> registers and the only way to do it is through PCIe mapping. In this those
>> registers will be available on BAR 0, with a offset of 4KB from the start.
>>
>> The driver also requires a memory space (RAM) to create a linked list
>> descriptors and pass the first descriptor address to the eDMA hardware block so
>> that it can start performing the transfer autonomously. Once again this memory
>> space is provide through PCIe on BAR 2. This linked list space is in the
>> beginning and it's restricted to a size of 128KB.
>>
>> Since this eDMA hardware block (more specifically eDMA registers) can evolve in
>> the future, I choosed to put here a version in order to have a driver that can
>> be easily adaptable and reusable without much effort.
>
> (2)
>
>>>> + err = pci_try_set_mwi(pdev);
>>>> + if (err) {
>>>> + dev_err(dev, "%s DMA memory write invalidate\n",
>>>> + pci_name(pdev));
>>>> + return err;
>>>> + }
>>>
>>> Are you sure you need this?
>>
>> I'm not sure, probably not. I saw using this being use on /dma/dw/pci.c driver.
>> I'll test without it.
>
> That's exactly the point what I referred to in (1) above.
> Looks like here is cargo-cult programming done without understanding why.
> I dunno how many other cases in the driver like this.
I understand your point of view and I agree with it and that's one of several
reasons why I launched this patch series as a RFC, to acquire some feedback and
expertise from more seniors developers about this matter.
>
>>>> + dev_info(dev, "Mode:\t%s\n",
>>>> + dw->mode == EDMA_MODE_LEGACY ? "Legacy" : "Unroll");
>>>> +
>>>
>>>
>>>> + dev_info(dev, "Registers:\tBAR=%u, off=0x%.16llx B, addr=0x%.8lx\n",
>>>> + pdata->regs_bar, pdata->regs_off,
>>>> + (unsigned long) dw->regs);
>>>
>>> Oh, no, don't do casting when printing something. In only rare cases it's needed, not here.
>>
>> I've put the cast to remove the following warning:
>>
>> drivers/dma/dw-edma/dw-edma-pcie.c:138:15: warning: format ‘%lx’ expects
>> argument of type ‘long unsigned int’, but argument 6 has type ‘void *’
>
> ...and as I said this is wrong. Please, find suitable specifiers for your case.
You were right, I corrected that in the meantime (fixed on RFC v3)
>
>>>> + if (pdev->msi_cap && pdev->msi_enabled) {
>>>> + cap_off = pdev->msi_cap + PCI_MSI_FLAGS;
>>>> + pci_read_config_word(pdev, cap_off, &flags);
>>>> + if (flags & PCI_MSI_FLAGS_ENABLE) {
>>>> + cap_off = pdev->msi_cap + PCI_MSI_ADDRESS_LO;
>>>> + pci_read_config_dword(pdev, cap_off, &addr_lo);
>>>> +
>>>> + if (flags & PCI_MSI_FLAGS_64BIT) {
>>>> + cap_off = pdev->msi_cap + PCI_MSI_ADDRESS_HI;
>>>> + pci_read_config_dword(pdev, cap_off, &addr_hi);
>>>> + cap_off = pdev->msi_cap + PCI_MSI_DATA_64;
>>>> + } else {
>>>> + addr_hi = 0;
>>>> + cap_off = pdev->msi_cap + PCI_MSI_DATA_32;
>>>> + }
>>>> +
>>>> + dw->msi_addr = addr_hi;
>>>> + dw->msi_addr <<= 32;
>>>> + dw->msi_addr |= addr_lo;
>>>> +
>>>> + pci_read_config_dword(pdev, cap_off, &(dw->msi_data));
>>>> + dw->msi_data &= 0xffff;
>>>> +
>>>> + dev_info(dev,
>>>> + "MSI:\t\taddr=0x%.16llx, data=0x%.8x, nr=%d\n",
>>>> + dw->msi_addr, dw->msi_data, pdev->irq);
>>>> + }
>>>> + }
>>>> +
>>>> + if (pdev->msix_cap && pdev->msix_enabled) {
>>>> + u32 offset;
>>>> + u8 bir;
>>>> +
>>>> + cap_off = pdev->msix_cap + PCI_MSIX_FLAGS;
>>>> + pci_read_config_word(pdev, cap_off, &flags);
>>>> +
>>>> + if (flags & PCI_MSIX_FLAGS_ENABLE) {
>>>> + cap_off = pdev->msix_cap + PCI_MSIX_TABLE;
>>>> + pci_read_config_dword(pdev, cap_off, &offset);
>>>> +
>>>> + bir = offset & PCI_MSIX_TABLE_BIR;
>>>> + offset &= PCI_MSIX_TABLE_OFFSET;
>>>> +
>>>> + reg = pcim_iomap_table(pdev)[bir];
>>>> + reg += offset;
>>>> +
>>>> + addr_lo = readl(reg + PCI_MSIX_ENTRY_LOWER_ADDR);
>>>> + addr_hi = readl(reg + PCI_MSIX_ENTRY_UPPER_ADDR);
>>>> + dw->msi_addr = addr_hi;
>>>> + dw->msi_addr <<= 32;
>>>> + dw->msi_addr |= addr_lo;
>>>> +
>>>> + dw->msi_data = readl(reg + PCI_MSIX_ENTRY_DATA);
>>>> +
>>>> + dev_info(dev,
>>>> + "MSI-X:\taddr=0x%.16llx, data=0x%.8x, nr=%d\n",
>>>> + dw->msi_addr, dw->msi_data, pdev->irq);
>>>> + }
>>>> + }
>>>
>>> What is this? Why?
>>
>> I need to find the PCIe interrupt vector address and value (either for MSI or
>> MSI-X, depends what the system has selected) in order to configure eDMA later to
>> generate the (done or abort) interruptions.
>
> It seems rather hackish way to do something which should be done by PCI/driver core.
> But perhaps Bjorn and others familiar with PCI endpoint facility can share more.
> Also this applies to (2) above.
Hum, I didn't found anything till now. However I'll ask Bjorn.
>
>>>> + if (!pdev->msi_enabled && !pdev->msix_enabled) {
>>
>> I'm not finding it. Can you tell me what it is?
>>
>> Note: pci_msi_enabled() on msi.c returns if MSI has not been disabled by the
>> command-line option pci=nomsi
>
> Can you look a bit more carefully? For example, assume that 'dev' (sub)prefix
> in the name of function would tell something about scope of application.
>
> pci_dev_...() ?
The function is pci_dev_msi_enabled(), I've found it meanwhile (fixed on RFC
v3). Thanks for the input.
>
>>> There is a helper from PCI core for this.
>
Thanks for the feedback.
Gustavo
^ permalink raw reply
* [2/3] dmaengine: Extend the k3dma driver binding
From: Rob Herring @ 2019-01-03 23:21 UTC (permalink / raw)
To: h00249924
Cc: dmaengine, devicetree, linux-kernel, linux-arm-kernel,
suzhuangluan, kongfei, liyuequan, cash.qianli, huangli295,
hantanglei, wangyoulin1, ninggaoyu, hanxiaolong3, Youlin Wang,
Vinod Koul, Mark Rutland
On Fri, Dec 28, 2018 at 02:36:22PM +0800, h00249924 wrote:
> From: Youlin Wang <wwx575822@notesmail.huawei.com>
>
> Extend the k3dma driver binding to support hisi-asp hardware variants.
>
> Signed-off-by: Youlin Wang <wwx575822@notesmail.huawei.com>
> Signed-off-by: Tanglei Han <hantanglei@huawei.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
> Documentation/devicetree/bindings/dma/k3dma.txt | 33 ++++++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> index 4945aea..cd21b82 100644
> --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> @@ -3,7 +3,9 @@
> See dma.txt first
>
> Required properties:
> -- compatible: Should be "hisilicon,k3-dma-1.0"
> +- compatible: Must be one of
> +- "hisilicon,k3-dma-1.0"
> +- "hisilicon,hisi-pcm-asp-dma-1.0"
> - reg: Should contain DMA registers location and length.
> - interrupts: Should contain one interrupt shared by all channel
> - #dma-cells: see dma.txt, should be 1, para number
> @@ -43,3 +45,32 @@ For example, i2c0 read channel request line is 18, while write channel use 19
> dma-names = "rx", "tx";
> };
>
> +
> +
> +
> +Controller:
Why is a new example needed just for a new compatible string?
> + asp_dmac: asp_dmac@E804B000 {
> + compatible = "hisilicon,hisi-pcm-asp-dma-1.0";
> + reg = <0x0 0xe804b000 0x0 0x1000>;
> + #dma-cells = <1>;
> + dma-channels = <16>;
> + dma-requests = <32>;
> + dma-min-chan = <0>;
> + dma-used-chans = <0xFFFE>;
> + dma-share;
> + interrupts = <0 216 4>;
> + interrupt-names = "asp_dma_irq";
> + status = "ok";
> + };
> +
> +Client:
> + i2s2: hisi_i2s {
> + compatible = "hisilicon,hisi-i2s";
> + reg = <0x0 0xe804f800 0x0 0x400>,
> + <0x0 0xe804e000 0x0 0x400>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2s2_pmx_func &i2s2_cfg_func>;
> + dmas = <&asp_dmac 18 &asp_dmac 19>;
> + dma-names = "rx", "tx";
> + #sound-dai-cells = <0>;
> + };
> --
> 1.9.1
>
^ permalink raw reply
* [3/3] arm64: dts: hi3660: Add hisi asp dma device
From: Rob Herring @ 2019-01-04 2:44 UTC (permalink / raw)
To: h00249924
Cc: dmaengine, devicetree, linux-kernel, linux-arm-kernel,
suzhuangluan, kongfei, liyuequan, cash.qianli, huangli295,
hantanglei, wangyoulin1, ninggaoyu, hanxiaolong3, Youlin Wang,
John Stultz, Wei Xu, Mark Rutland
On Fri, Dec 28, 2018 at 02:36:23PM +0800, h00249924 wrote:
> From: Youlin Wang <wwx575822@notesmail.huawei.com>
>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Youlin Wang <wwx575822@notesmail.huawei.com>
> Signed-off-by: Tanglei Han <hantanglei@huawei.com>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
> arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index f432b0a..5223e36 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -1122,5 +1122,19 @@
> };
> };
> };
> +
> + asp_dmac: asp_dmac@E804B000 {
dma-controller@e804b000
> + compatible = "hisilicon,hisi-pcm-asp-dma-1.0";
> + reg = <0x0 0xe804b000 0x0 0x1000>;
> + #dma-cells = <1>;
> + dma-channels = <16>;
> + dma-requests = <32>;
> + dma-min-chan = <0>;
> + dma-used-chans = <0xFFFE>;
Use lowercase hex.
But more importantly, as John mentioned, this and other properties
aren't documented.
> + dma-share;
> + interrupts = <0 216 4>;
> + interrupt-names = "asp_dma_irq";
> + status = "ok";
Don't need this.
> + };
> };
> };
> --
> 1.9.1
>
^ permalink raw reply
* dmaengine: stm32-mdma: Add a check on read_u32_array
From: Pierre Yves MORDRET @ 2019-01-04 8:05 UTC (permalink / raw)
To: Aditya Pakki
Cc: Alexandre Torgue, kjlu, linux-kernel, Vinod Koul, Maxime Coquelin,
dmaengine, Dan Williams, linux-stm32, linux-arm-kernel
Hello
Acked-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>
Regards
On 12/28/18 8:26 PM, Aditya Pakki wrote:
> In stm32_mdma_probe, after reading the property "st,ahb-addr-masks", the
> second call is not checked for failure. This time of check to time of use
> case of "count" error is sent upstream.
>
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> ---
> drivers/dma/stm32-mdma.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/stm32-mdma.c b/drivers/dma/stm32-mdma.c
> index 390e4cae0e1a..485dea177704 100644
> --- a/drivers/dma/stm32-mdma.c
> +++ b/drivers/dma/stm32-mdma.c
> @@ -1579,9 +1579,11 @@ static int stm32_mdma_probe(struct platform_device *pdev)
>
> dmadev->nr_channels = nr_channels;
> dmadev->nr_requests = nr_requests;
> - device_property_read_u32_array(&pdev->dev, "st,ahb-addr-masks",
> + ret = device_property_read_u32_array(&pdev->dev, "st,ahb-addr-masks",
> dmadev->ahb_addr_masks,
> count);
> + if (ret)
> + return ret;
> dmadev->nr_ahb_addr_masks = count;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
^ permalink raw reply
* [2/2] dmaengine: qcom_hidma: assign channel cookie correctly
From: Yang Shunyong @ 2019-01-04 8:56 UTC (permalink / raw)
To: vkoul@kernel.org
Cc: andy.gross@linaro.org, david.brown@linaro.org, Sinan Kaya,
dan.j.williams@intel.com, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org, Zheng, Joey
Hi, Vinod,
Gentle ping. Would you please help to review these two patches and merge?
On 2018/12/14 9:03, Yang, Shunyong wrote:
> Hi, Sinan
>
> On 2018/12/9 4:28, Sinan Kaya wrote:
>> On 12/6/2018 11:29 PM, Shunyong Yang wrote:
>>> When dma_cookie_complete() is called in hidma_process_completed(),
>>> dma_cookie_status() will return DMA_COMPLETE in hidma_tx_status(). Then,
>>> hidma_txn_is_success() will be called to use channel cookie
>>> mchan->last_success to do additional DMA status check. Current code
>>> assigns mchan->last_success after dma_cookie_complete(). This causes
>>> a race condition of dma_cookie_status() returns DMA_COMPLETE before
>>> mchan->last_success is assigned correctly. The race will cause
>>> hidma_tx_status() return DMA_ERROR but the transaction is actually a
>>> success. Moreover, in async_tx case, it will cause a timeout panic
>>> in async_tx_quiesce().
>>>
>>> Kernel panic - not syncing: async_tx_quiesce: DMA error waiting for
>>> transaction
>>> ...
>>> Call trace:
>>> [<ffff000008089994>] dump_backtrace+0x0/0x1f4
>>> [<ffff000008089bac>] show_stack+0x24/0x2c
>>> [<ffff00000891e198>] dump_stack+0x84/0xa8
>>> [<ffff0000080da544>] panic+0x12c/0x29c
>>> [<ffff0000045d0334>] async_tx_quiesce+0xa4/0xc8 [async_tx]
>>> [<ffff0000045d03c8>] async_trigger_callback+0x70/0x1c0 [async_tx]
>>> [<ffff0000048b7d74>] raid_run_ops+0x86c/0x1540 [raid456]
>>> [<ffff0000048bd084>] handle_stripe+0x5e8/0x1c7c [raid456]
>>> [<ffff0000048be9ec>] handle_active_stripes.isra.45+0x2d4/0x550 [raid456]
>>> [<ffff0000048beff4>] raid5d+0x38c/0x5d0 [raid456]
>>> [<ffff000008736538>] md_thread+0x108/0x168
>>> [<ffff0000080fb1cc>] kthread+0x10c/0x138
>>> [<ffff000008084d34>] ret_from_fork+0x10/0x18
>>>
>>> Cc: Joey Zheng<yu.zheng@hxt-semitech.com>
>>> Signed-off-by: Shunyong Yang<shunyong.yang@hxt-semitech.com>
>>
>>
>> Acked-by: Sinan Kaya <okaya@kernel.org>
>>
>> to both patches 1/2 and 2/2.
>>
>>
> Thanks for the ACKs.
>
> Shunyong.
>
>
^ permalink raw reply
* [2/2] dmaengine: mediatek: Add MediaTek Command-Queue DMA controller for MT6765 SoC
From: Vinod Koul @ 2019-01-04 12:38 UTC (permalink / raw)
To: shun-chih.yu
Cc: Sean Wang, Rob Herring, Matthias Brugger, Dan Williams, dmaengine,
linux-arm-kernel, linux-mediatek, devicetree, linux-kernel,
srv_wsdupstream
On 27-12-18, 21:10, shun-chih.yu@mediatek.com wrote:
> From: Shun-Chih Yu <shun-chih.yu@mediatek.com>
>
This should be v4 of the patch series, why is it not tagged so?
> MediaTek Command-Queue DMA controller (CQDMA) on MT6765 SoC is dedicated
> to memory-to-memory transfer through queue based descriptor management.
Have you tested this with dmatest, if so can you provide results of the
test as well.
> 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
Please remove this from upstream, checkpatch would have warned that!
> Signed-off-by: Shun-Chih Yu <shun-chih.yu@mediatek.com>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
This is _WRONG_ I have never provided such tag, can you explain why this
was added without my approval?
> 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"
Am not sure if I asked this earlier but, what is difference with HSDMA?
> +/**
> + * 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
Please maintain alignment!
> + */
> +struct mtk_cqdma_pchan {
> + struct list_head queue;
> + void __iomem *base;
> + u32 irq;
> + refcount_t refcnt;
> +
> + /* lock to protect PC */
This is not required, you already have above !
> + 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
typo completited , am not sure why you need this
> +static void mtk_dma_write(struct mtk_cqdma_pchan *pc, u32 reg, u32 val)
> +{
> + writel_relaxed(val, pc->base + reg);
Why is it relaxed one?
> +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);
you can use vchan_next_desc() and also remove your own queue as
virt-desc already implements that logic!
> + 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;
> + }
why do you need your own completion?
> +
> + spin_unlock(&cvc->vc.lock);
> + }
> +
> + /* 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);
most of this logic looks reduandant to me. Virt-dma was designed to do
exactly this, have N physical channels and share with M virt channels.
Please reuse and remove code from this driver.
> +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;
vchan_find_desc() ??
> +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;
> + }
> +
> + dma_set_residue(txstate, bytes);
Have you tested this and are able to report residue properly?
> +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);
You missed completed and didnt use vchan_get_all_descriptors() ??
Looking at the driver, I feel things have been complicated a bit, you
can reuse more code and routines in vchan layer and remove driver
handling and make things with less bugs (used more tested generic code)
and simpler to review ..
Please do so in next rev and tag version properly!
^ permalink raw reply
* dmaengine: stm32-mdma: Add a check on read_u32_array
From: Vinod Koul @ 2019-01-04 14:33 UTC (permalink / raw)
To: Aditya Pakki
Cc: kjlu, Dan Williams, Maxime Coquelin, Alexandre Torgue, dmaengine,
linux-stm32, linux-arm-kernel, linux-kernel
On 28-12-18, 13:26, Aditya Pakki wrote:
> In stm32_mdma_probe, after reading the property "st,ahb-addr-masks", the
> second call is not checked for failure. This time of check to time of use
> case of "count" error is sent upstream.
Applied, thanks
^ permalink raw reply
* dma/mv_xor: Fix a missing check in mv_xor_channel_add
From: Vinod Koul @ 2019-01-04 14:36 UTC (permalink / raw)
To: Aditya Pakki; +Cc: kjlu, Dan Williams, dmaengine, linux-kernel
On 24-12-18, 11:41, Aditya Pakki wrote:
> dma_async_device_register() may fail and return an error. The capabilities
> checked in mv_xor_channel_add() are not complete. The fix handles the
> error by freeing the resources.
Applied after fixing subsystem tag, thanks
^ permalink raw reply
* dmaengine: qcom_hidma: Check for driver register failure
From: Vinod Koul @ 2019-01-04 14:38 UTC (permalink / raw)
To: Aditya Pakki
Cc: kjlu, Sinan Kaya, Andy Gross, David Brown, Dan Williams,
linux-arm-kernel, linux-arm-msm, dmaengine, linux-kernel
On 28-12-18, 14:11, Aditya Pakki wrote:
> While initializing the driver, the function platform_driver_register can
> fail and return an error. Consistent with other invocations, this patch
> returns the error upstream.
Applied, thanks
^ permalink raw reply
* dmaengine: fix dmaengine_desc_callback_valid() doesn't check for callback_result
From: Vinod Koul @ 2019-01-04 14:51 UTC (permalink / raw)
To: Andrea Merello
Cc: dan.j.williams, dmaengine, linux-kernel, radhey.shyam.pandey
On 16-11-18, 14:56, Andrea Merello wrote:
> There are two flavors of DMA completion callbacks: callback() and
> callback_result(); the latter takes an additional parameter that carries
> result information.
>
> Most dmaengine helper functions that work with callbacks take care of both
> flavors i.e. dmaengine_desc_get_callback_invoke() first checks for
> callback_result() to be not NULL, and eventually it calls this one;
> otherwise it goes on checking for callback().
>
> It seems however that dmaengine_desc_callback_valid() does not care about
> callback_result(), and it returns false in case callback() is NULL but
> callback_result() is not; unless there is a (hidden to me) reason for doing
> so then I'd say this is wrong.
>
> I've hit this by using a DMA controller driver (xilinx_dma) that doesn't
> trigger any callback invocation unless dmaengine_desc_callback_valid()
> returns true, while I had only callback_result() implemented in my client
> driver (which AFAICT is always fine since dmaengine documentation says that
> callback() will be deprecated).
>
> This patch fixes this by making dmaengine_desc_callback_valid() to return
> true in the said scenario.
>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
> drivers/dma/dmaengine.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
> index 501c0b063f85..0ba2c1f3c55d 100644
> --- a/drivers/dma/dmaengine.h
> +++ b/drivers/dma/dmaengine.h
> @@ -168,7 +168,7 @@ dmaengine_desc_get_callback_invoke(struct dma_async_tx_descriptor *tx,
> static inline bool
> dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb)
> {
> - return (cb->callback) ? true : false;
> + return (cb->callback || cb->callback_result);
So I do not think this one should take care of callback_result, it is
supposed to check if the callback is valid or not.. Nothing more.
Ofcourse usage of this maybe incorrect which should be fixed. We do have
dmaengine_desc_callback_invoke() which propagates the callback_result to
user
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox