* [RFC,1/6] dma: Add Synopsys eDMA IP core driver
From: Vinod Koul @ 2018-12-17 7:23 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Gustavo Pimentel, linux-pci, dmaengine, Eugeniy Paltsev,
Andy Shevchenko, Joao Pinto
On 12-12-18, 17:00, Bjorn Helgaas wrote:
> On Wed, Dec 12, 2018 at 12:13:21PM +0100, Gustavo Pimentel wrote:
> > Add Synopsys eDMA IP core driver to kernel.
> >
> > This core driver, initializes and configures the eDMA IP using vma-helpers
> > functions and dma-engine subsystem.
> >
> > Also creates an abstration layer through callbacks allowing different
> > registers mappings in the future, organized in to versions.
> >
> > This driver can be compile as built-in or external module in kernel.
> >
> > To enable this driver just select DW_EDMA option in kernel configuration,
> > however it requires and selects automatically DMA_ENGINE and
> > DMA_VIRTUAL_CHANNELS option too.
> >
> > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: Eugeniy Paltsev <paltsev@synopsys.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Joao Pinto <jpinto@synopsys.com>
>
> > --- /dev/null
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -0,0 +1,925 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> > +// Synopsys DesignWare eDMA core driver
>
> The SPDX line in .c files needs the // comment for some obscure reason
> I can't remember, but based on the other drivers/dma/*.c files, it
> looks like the convention is the usual /* .. */ comments for the rest.
>
> I think the SPDX line is /* */ in .h files though. The rules are
> under Documentation/ somewhere.
Yes and documented in Documentation/process/license-rules.rst
"license identifier syntax" section
^ permalink raw reply
* [RFC,1/6] dma: Add Synopsys eDMA IP core driver
From: Vinod Koul @ 2018-12-17 6:51 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci, dmaengine, Eugeniy Paltsev, Andy Shevchenko,
Joao Pinto
On 12-12-18, 12:13, Gustavo Pimentel wrote:
> Add Synopsys eDMA IP core driver to kernel.
>
> This core driver, initializes and configures the eDMA IP using vma-helpers
> functions and dma-engine subsystem.
>
> Also creates an abstration layer through callbacks allowing different
> registers mappings in the future, organized in to versions.
>
> This driver can be compile as built-in or external module in kernel.
>
> To enable this driver just select DW_EDMA option in kernel configuration,
> however it requires and selects automatically DMA_ENGINE and
> DMA_VIRTUAL_CHANNELS option too.
The subsystem name is dmaengine: so please use that tag. If you are not
aware then git log for that subsystem helps you with the patterns
expected
I did a quick look at the patch, I have highlighted few concerns and
they repeat in similar code patterns in this patch
> +#include "dw-edma-core.h"
> +#include "../dmaengine.h"
> +#include "../virt-dma.h"
> +
> +#define DRV_CORE_NAME "dw-edma-core"
Why is this required?
> +
> +#define SET(reg, name, val) \
> + reg.name = val
> +
> +#define SET_BOTH_CH(name, value) \
> + do { \
> + SET(dw->wr_edma, name, value); \
> + SET(dw->rd_edma, name, value); \
> + } while (0)
I am not sure how this helps, makes things not explicit..
> +static struct dw_edma_burst *dw_edma_alloc_burst(struct dw_edma_chunk *chunk)
> +{
> + struct dw_edma_chan *chan = chunk->chan;
> + struct dw_edma_burst *burst;
> +
> + burst = kzalloc(sizeof(struct dw_edma_burst), GFP_NOWAIT);
> + if (unlikely(!burst)) {
> + dev_err(chan2dev(chan), ": fail to alloc new burst\n");
no need to log mem alloc failures
> + return NULL;
> + }
> +
> + INIT_LIST_HEAD(&burst->list);
> + burst->sar = 0;
> + burst->dar = 0;
> + burst->sz = 0;
you did kzalloc right?
> +
> + if (chunk->burst) {
> + atomic_inc(&chunk->bursts_alloc);
why does this need atomic variables?
> +static void dw_edma_free_burst(struct dw_edma_chunk *chunk)
> +{
> + struct dw_edma_burst *child, *_next;
> +
> + if (!chunk->burst)
> + return;
> +
> + // Remove all the list elements
We dont use C99 style comments, please use
/* single line */
and
/*
* multi
* line
*/
> +static void start_transfer(struct dw_edma_chan *chan)
> +{
> + struct virt_dma_desc *vd;
> + struct dw_edma_desc *desc;
> + struct dw_edma_chunk *child, *_next;
> + const struct dw_edma_core_ops *ops = chan2ops(chan);
> +
> + vd = vchan_next_desc(&chan->vc);
> + if (!vd)
> + return;
> +
> + desc = vd2dw_edma_desc(vd);
> + if (!desc)
> + return;
> +
> + list_for_each_entry_safe(child, _next, &desc->chunk->list, list) {
> + ops->start(child, !desc->xfer_sz);
> + desc->xfer_sz += child->sz;
> + dev_dbg(chan2dev(chan),
> + ": transfer of %u bytes started\n", child->sz);
> +
> + dw_edma_free_burst(child);
> + if (atomic_read(&child->bursts_alloc))
> + dev_dbg(chan2dev(chan),
> + ": %d bursts still allocated\n",
> + atomic_read(&child->bursts_alloc));
> + list_del(&child->list);
> + kfree(child);
> + atomic_dec(&desc->chunks_alloc);
> +
> + return;
> + }
> +}
> +
> +static int dw_edma_device_config(struct dma_chan *dchan,
> + struct dma_slave_config *config)
please align to preceding brace. Also running checkpatch with --strict
option helps, warning checkpatch is a guidebook and not a rule book!
> +{
> + 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
> + 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
> +
> + 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);
You have too many logs, it is good for bringup and initial work but not
suited for production.
> +
> + err = ops->device_config(dchan);
okay what does this callback do. You are already under and dmaengine fwk
so what is the need to add one more abstraction layer, can you explain
that in details please
> +static int dw_edma_device_pause(struct dma_chan *dchan)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> + unsigned long flags;
> + int err = 0;
> +
> + spin_lock_irqsave(&chan->vc.lock, flags);
> +
> + if (!chan->configured) {
> + dev_err(dchan2dev(dchan), ": channel not configured\n");
> + err = -EPERM;
> + goto err_pause;
> + }
> +
> + switch (chan->status) {
> + case EDMA_ST_IDLE:
> + dev_err(dchan2dev(dchan), ": channel is idle\n");
> + err = -EPERM;
> + goto err_pause;
> + case EDMA_ST_PAUSE:
> + dev_err(dchan2dev(dchan), ": channel is already paused\n");
> + err = -EPERM;
> + goto err_pause;
> + case EDMA_ST_BUSY:
> + // Only acceptable state
> + break;
Doesn't it look as overkill to use switch for single acceptable case.
Why not do
if (chan->status != EDMA_ST_BUSY) {
err = -EPERM;
...
}
> + default:
> + dev_err(dchan2dev(dchan), ": invalid status state\n");
> + err = -EINVAL;
> + goto err_pause;
> + }
> +
> + switch (chan->request) {
what is the need to track channel status and channel requests?
^ permalink raw reply
* [1/4] dmaengine: amba-pl08x: convert to DEFINE_SHOW_ATTRIBUTE
From: Vinod Koul @ 2018-12-17 6:19 UTC (permalink / raw)
To: Yangtao Li
Cc: dan.j.williams, sudeep.dutt, ashutosh.dixit, daniel,
haojian.zhuang, robert.jarzmik, okaya, andy.gross, david.brown,
dmaengine, linux-kernel, linux-arm-kernel, linux-arm-msm
On 05-12-18, 11:18, Yangtao Li wrote:
> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
Applied all 4, thanks
^ permalink raw reply
* dmaengine: fsl-qdma: add MODULE_LICENSE
From: Vinod Koul @ 2018-12-17 6:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Dan Williams, Wen He, Jiaheng Fan, Peng Ma, dmaengine,
linux-kernel
On 10-12-18, 21:55, Arnd Bergmann wrote:
> The newly added driver lacks a MODULE_LICENSE tag, which now produces
> a warning:
>
> WARNING: modpost: missing MODULE_LICENSE() in drivers/dma/fsl-qdma.o
>
> Add the license according to the SPDX specifier.
Applied, thanks
^ permalink raw reply
* [6/6] dmaengine: sh: rcar-dmac: Use device_iommu_mapped()
From: Vinod Koul @ 2018-12-17 6:11 UTC (permalink / raw)
To: Joerg Roedel
Cc: iommu, Russell Currey, Sam Bobroff, oohall,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Lorenzo Pieralisi, Hanjun Guo, Sudeep Holla, Dan Williams,
jroedel, Mathias Nyman, Greg Kroah-Hartman, linux-kernel,
linux-acpi, dmaengine, linux-usb
On 11-12-18, 14:43, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Use Use device_iommu_mapped() to check if the device is
> already mapped by an IOMMU.
Acked-by: Vinod Koul <vkoul@kernel.org>
^ permalink raw reply
* [1/6] driver core: Introduce device_iommu_mapped() function
From: Vinod Koul @ 2018-12-17 6:11 UTC (permalink / raw)
To: Joerg Roedel
Cc: iommu, Russell Currey, Sam Bobroff, oohall,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Lorenzo Pieralisi, Hanjun Guo, Sudeep Holla, Dan Williams,
jroedel, Mathias Nyman, Greg Kroah-Hartman, linux-kernel,
linux-acpi, dmaengine, linux-usb
On 11-12-18, 14:43, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Some places in the kernel check the iommu_group pointer in
> 'struct device' in order to find ot whether a device is
^^
Typo
^ permalink raw reply
* dt-bindings: dmaengine: usb-dmac: Add binding for r8a774c0
From: Vinod Koul @ 2018-12-17 5:31 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Rob Herring, Mark Rutland, dmaengine, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc
On 13-12-18, 20:21, Fabrizio Castro wrote:
> This patch adds bindings for the r8a774c0 (RZ/G2E).
Applied, thanks
^ permalink raw reply
* dmaengine: rcar-dmac: Document R8A774C0 bindings
From: Vinod Koul @ 2018-12-17 5:29 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Rob Herring, Mark Rutland, dmaengine, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc
On 13-12-18, 20:17, Fabrizio Castro wrote:
> Renesas' RZ/G2E (R8A774C0) SoC has DMA controllers compatible
> with this driver, therefore document RZ/G2E specific bindings.
Applied, thanks
^ permalink raw reply
* dmaengine: rcar-dmac: Document R8A774C0 bindings
From: Simon Horman @ 2018-12-15 13:50 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Vinod Koul, Rob Herring, Mark Rutland, dmaengine, devicetree,
linux-kernel, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc
On Thu, Dec 13, 2018 at 08:17:43PM +0000, Fabrizio Castro wrote:
> Renesas' RZ/G2E (R8A774C0) SoC has DMA controllers compatible
> with this driver, therefore document RZ/G2E specific bindings.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
^ permalink raw reply
* [v5,1/2] dmaengine: 8250_mtk_dma: add Mediatek uart DMA support
From: Sean Wang @ 2018-12-14 20:09 UTC (permalink / raw)
To: long.cheng
Cc: vkoul, robh+dt, mark.rutland,
Ryder Lee (李庚諺), Matthias Brugger,
dan.j.williams, gregkh, jslaby,
Sean Wang (王志亘), dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, yingjoe.chen, YT Shen
On Thu, Dec 13, 2018 at 3:36 AM Long Cheng <long.cheng@mediatek.com> wrote:
Hope those comments did not get a response that means they're fine with you.
< ... >
> > > +struct mtk_dmadev {
> > > + struct dma_device ddev;
> > > + void __iomem *mem_base[MTK_APDMA_CHANNELS];
> > > + spinlock_t lock; /* dma dev lock */
> > > + struct tasklet_struct task;
> >
> > we can drop tasklet and instead allows descriptors to be handled as
> > fast as possible.
> > similar suggestions have been made in the other dmaengine [1] and mtk-hsdma.c
> >
>
> OK, i will remove these, and improve it.
>
Thanks, that would be great.
> > [1] https://lkml.org/lkml/2018/11/11/146
> >
> > > + struct list_head pending;
> > > + struct clk *clk;
> > > + unsigned int dma_requests;
> > > + bool support_33bits;
> > > + unsigned int dma_irq[MTK_APDMA_CHANNELS];
> > > + struct mtk_chan *ch[MTK_APDMA_CHANNELS];
> > > +};
> > > +
< ... >
> > > +static struct dma_async_tx_descriptor *mtk_dma_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_dma_chan(chan);
> > > + struct scatterlist *sgent;
> > > + struct mtk_dma_desc *d;
> > > + struct mtk_dma_sg *sg;
> > > + unsigned int size, i, j, en;
> > > +
> > > + en = 1;
> > > +
> > > + if ((dir != DMA_DEV_TO_MEM) &&
> > > + (dir != DMA_MEM_TO_DEV)) {
> > > + dev_err(chan->device->dev, "bad direction\n");
> > > + return NULL;
> > > + }
> > > +
> > > + /* Now allocate and setup the descriptor. */
> > > + d = kzalloc(sizeof(*d) + sglen * sizeof(d->sg[0]), GFP_ATOMIC);
> > > + if (!d)
> > > + return NULL;
> > > +
> > > + d->dir = dir;
> > > +
> > > + j = 0;
> > > + for_each_sg(sgl, sgent, sglen, i) {
> > > + d->sg[j].addr = sg_dma_address(sgent);
> > > + d->sg[j].en = en;
> > > + d->sg[j].fn = sg_dma_len(sgent) / en;
> > > + j++;
> > > + }
> > > +
> > > + d->sglen = j;
> > > +
> > > + if (dir == DMA_MEM_TO_DEV) {
> > > + for (size = i = 0; i < d->sglen; i++) {
> > > + sg = &d->sg[i];
> > > + size += sg->en * sg->fn;
> > > + }
> > > + d->len = size;
> > > + }
> > > +
> >
> > The driver always only handles data move for the single contiguous
> > area, but it seems the callback must provide the scatter-gather
> > function to the dmaegine. otherwise, why is the callback be called
> > device_prep_slave_sg?
> >
>
> because in 8250 UART native code, call the device_prep_slave_sg. we just
> use one ring buffer.
>
If it really did specifically for UART, you should show the function
only can handle only one entry in sg for the DMA in a few comments and
a sanity check for these invalid requests (more one entries in sg).
Otherwise, the hardware will get a failure and even function doesn't
complain or warn anything if more one entries in sg requested in.
Additionally, the code can be simplified much if it's just for the
specific use case.
> > > + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> > > +}
> > > +
> > > +static void mtk_dma_issue_pending(struct dma_chan *chan)
> > > +{
> > > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > > + struct virt_dma_desc *vd;
> > > + struct mtk_dmadev *mtkd;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > > + mtkd = to_mtk_dma_dev(chan->device);
> >
> > mtkd can be dropped as it seems no users
> >
< ... >
> > > +static int mtk_dma_slave_config(struct dma_chan *chan,
> > > + struct dma_slave_config *cfg)
> > > +{
> > > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> > > + int ret;
> > > +
> > > + c->cfg = *cfg;
> > > +
> > > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > > + unsigned int rx_len = cfg->src_addr_width * 1024;
> >
> > it seems you should use cfg->src_port_window_size as the comments explains
> >
> > * @src_port_window_size: The length of the register area in words the data need
> > * to be accessed on the device side. It is only used for devices which is using
> > * an area instead of a single register to receive the data. Typically the DMA
> > * loops in this area in order to transfer the data.
> > * @dst_port_window_size: same as src_port_window_size but for the destination
> > * port.
> >
>
> in 8250 UART native code, will set the parameter. if want to modify
> this, i think that maybe at next kernel release, we can modify it. i
> suggest that not modify it at this patch. because it relate of 8250 uart
> code. thanks.
>
You can fix it in the next version and a separate follow-up patch for
the client driver.
> > > +
> > > + mtk_dma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > > + mtk_dma_chan_write(c, VFF_LEN, rx_len);
> > > + mtk_dma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > > + mtk_dma_chan_write(c,
> > > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > > + | VFF_RX_INT_EN1_B);
> > > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> >
> > I'd prefer to move those channel interrupt enablement to
> > .device_alloc_chan_resources
> > and related disablement to .device_free_chan_resources
> >
>
> i think that, first need set the config to HW, and the enable the DMA.
>
I've read through the client driver and the dmaengine, I probably know
how interaction they work and find out there is something you seem
completely make a wrong.
You can't do enable DMA with enabling VFF here. That causes a big
problem, DMA would self decide to move data and not managed and issued
by the dmaengine framework. For instance in DMA Tx path, because you
enable the DMA and DMA buffer (VFF) and UART Tx ring uses the same
memory area, DMA would self start to move data once data from
userland go in Tx ring even without being issued by dmaengine.
Instead, you should ensure all descriptors are being batched by
.device_prep_slave_sg and DMA does not start moving data until
.device_issue_pending done and once descriptors are issued, DMA
hardware or software have to do it as fast as possible.
> > > +
> > > + if (!c->requested) {
> > > + c->requested = true;
> > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > + mtk_dma_rx_interrupt,
> > > + IRQF_TRIGGER_NONE,
> > > + KBUILD_MODNAME, chan);
> >
> > ISR registration usually happens as the driver got probe, it can give
> > the system more flexibility to manage such IRQ affinity on the fly.
> >
>
> i will move the request irq to alloc channel.
>
why don't let it done in driver probe?
> > > + if (ret < 0) {
> > > + dev_err(chan->device->dev, "Can't request rx dma IRQ\n");
> > > + return -EINVAL;
> > > + }
> > > + }
> > > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > > + unsigned int tx_len = cfg->dst_addr_width * 1024;
> >
> > Ditto as above, it seems you should use cfg->dst_port_window_size
> >
> > > +
> > > + mtk_dma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > > + mtk_dma_chan_write(c, VFF_LEN, tx_len);
> > > + mtk_dma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> >
> > ditto, I'd prefer to move those channel interrupt enablement to
> > .device_alloc_chan_resources and related disablement to
> > .device_free_chan_resources
> >
> > > +
> > > + if (!c->requested) {
> > > + c->requested = true;
> > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > + mtk_dma_tx_interrupt,
> > > + IRQF_TRIGGER_NONE,
> > > + KBUILD_MODNAME, chan);
> >
> > ditto, we can request ISR with devm_request_irq in the driver got
> > probe and trim the c->request member
> >
> > > + if (ret < 0) {
> > > + dev_err(chan->device->dev, "Can't request tx dma IRQ\n");
> > > + return -EINVAL;
> > > + }
> > > + }
> > > + }
> > > +
> > > + if (mtkd->support_33bits)
> > > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > > +
> > > + if (mtk_dma_chan_read(c, VFF_EN) != VFF_EN_B) {
> > > + dev_err(chan->device->dev,
> > > + "config dma dir[%d] fail\n", cfg->direction);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_dma_terminate_all(struct dma_chan *chan)
> > > +{
> > > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > + list_del_init(&c->node);
> > > + mtk_dma_stop(c);
> > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_dma_device_pause(struct dma_chan *chan)
> > > +{
> > > + /* just for check caps pass */
> > > + return -EINVAL;
> >
> > always return error code seems not the client driver wants us to do.
> >
> > maybe if the hardware doesn't support pause, we can make a software
> > pause, that waits until all active descriptors in hardware done, then
> > disable interrupt and then stop handling the following vd in the
> > vchan.
> >
> > > +}
>
> the code can't run. just for 8250 native code to check the function
> pointer. in the future, if the function useful, we can realize. thanks.
>
Always return an error code looks like it's a faked function just to
pass the criteria check. It seems not a good idea.
> > > +
> > > +static int mtk_dma_device_resume(struct dma_chan *chan)
> > > +{
> > > + /* just for check caps pass */
> > > + return -EINVAL;
< ... >
> > > +static struct platform_driver mtk_dma_driver = {
> >
> > mtk_dma is much general and all functions and structures in the driver
> > should be all consistent. I'd prefer to have all naming starts with
> > mtk_uart_apdma.
> >
>
> the function name and parameters' name, i will modify it. but before the
> 8250_mtk.c and the Doc. are recorded. if modify file name and Kconfig,
> will bring about the disorder. i suggest that after the patch is
> recorded, modify this. thanks.
>
We can do it in separate patches and in a logical order for the
changes required across different subsystems.
> > > + .probe = mtk_apdma_probe,
> >
> > such as
> > mtk_uart_apdma_probe
> >
> > > + .remove = mtk_apdma_remove,
> >
> > mtk_uart_apdma_remove
> >
> > > + .driver = {
> > > + .name = KBUILD_MODNAME,
> > > + .pm = &mtk_dma_pm_ops,
> >
> > mtk_uart_apdma_pm_ops
> >
> > > + .of_match_table = of_match_ptr(mtk_uart_dma_match),
> >
> > mtk_uart_apdma_match
> >
> > > + },
> > > +};
> > > +
> > > +module_platform_driver(mtk_dma_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..d399624 100644
> > > --- a/drivers/dma/mediatek/Kconfig
> > > +++ b/drivers/dma/mediatek/Kconfig
> > > @@ -1,4 +1,15 @@
> > >
> > > +config DMA_MTK_UART
> >
> > MTK_UART_APDMA to align the other drivers
> >
> > > + 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 8250 mtk uart is enabled, and if you want to using DMA,
> >
> > 8250 mtk uart should be changed to SERIAL_8250_MT6577 to be more intuitive
> >
> > > + you can enable the config. the DMA engine just only be used
> > > + with MediaTek Socs.
> >
> > 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_UART_APDMA) += mtk-uart-apdma.o
> > to align the other dirvers
> >
> > > obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> > > --
> > > 1.7.9.5
> > >
>
>
^ permalink raw reply
* dt-bindings: dmaengine: usb-dmac: Add binding for r8a774c0
From: Simon Horman @ 2018-12-14 16:40 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Vinod Koul, Rob Herring, Mark Rutland, dmaengine, devicetree,
linux-kernel, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc
On Thu, Dec 13, 2018 at 08:21:31PM +0000, Fabrizio Castro wrote:
> This patch adds bindings for the r8a774c0 (RZ/G2E).
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
^ permalink raw reply
* dt-bindings: dmaengine: usb-dmac: Add binding for r8a774c0
From: Geert Uytterhoeven @ 2018-12-14 10:20 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Vinod, Rob Herring, Mark Rutland, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, Linux-Renesas
On Thu, Dec 13, 2018 at 9:21 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> This patch adds bindings for the r8a774c0 (RZ/G2E).
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
^ permalink raw reply
* dmaengine: rcar-dmac: Document R8A774C0 bindings
From: Geert Uytterhoeven @ 2018-12-14 10:03 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Vinod, Rob Herring, Mark Rutland, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, Linux-Renesas
On Thu, Dec 13, 2018 at 9:17 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> Renesas' RZ/G2E (R8A774C0) SoC has DMA controllers compatible
> with this driver, therefore document RZ/G2E specific bindings.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
^ permalink raw reply
* [2/2] dmaengine: qcom_hidma: assign channel cookie correctly
From: Yang Shunyong @ 2018-12-14 1:03 UTC (permalink / raw)
To: Sinan Kaya
Cc: andy.gross@linaro.org, david.brown@linaro.org, vkoul@kernel.org,
dan.j.williams@intel.com, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org, Zheng, Joey
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
* dt-bindings: dmaengine: usb-dmac: Add binding for r8a774c0
From: Fabrizio Castro @ 2018-12-13 20:21 UTC (permalink / raw)
To: Vinod Koul, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, dmaengine, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc
This patch adds bindings for the r8a774c0 (RZ/G2E).
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
Documentation/devicetree/bindings/dma/renesas,usb-dmac.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/dma/renesas,usb-dmac.txt b/Documentation/devicetree/bindings/dma/renesas,usb-dmac.txt
index 5e2c7e8..372f0ee 100644
--- a/Documentation/devicetree/bindings/dma/renesas,usb-dmac.txt
+++ b/Documentation/devicetree/bindings/dma/renesas,usb-dmac.txt
@@ -8,6 +8,7 @@ Required Properties:
- "renesas,r8a7745-usb-dmac" (RZ/G1E)
- "renesas,r8a77470-usb-dmac" (RZ/G1C)
- "renesas,r8a774a1-usb-dmac" (RZ/G2M)
+ - "renesas,r8a774c0-usb-dmac" (RZ/G2E)
- "renesas,r8a7790-usb-dmac" (R-Car H2)
- "renesas,r8a7791-usb-dmac" (R-Car M2-W)
- "renesas,r8a7793-usb-dmac" (R-Car M2-N)
^ permalink raw reply related
* dmaengine: rcar-dmac: Document R8A774C0 bindings
From: Fabrizio Castro @ 2018-12-13 20:17 UTC (permalink / raw)
To: Vinod Koul, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, dmaengine, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc
Renesas' RZ/G2E (R8A774C0) SoC has DMA controllers compatible
with this driver, therefore document RZ/G2E specific bindings.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt b/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt
index cdf32b2..5a512c5 100644
--- a/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt
+++ b/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt
@@ -21,6 +21,7 @@ Required Properties:
- "renesas,dmac-r8a7745" (RZ/G1E)
- "renesas,dmac-r8a77470" (RZ/G1C)
- "renesas,dmac-r8a774a1" (RZ/G2M)
+ - "renesas,dmac-r8a774c0" (RZ/G2E)
- "renesas,dmac-r8a7790" (R-Car H2)
- "renesas,dmac-r8a7791" (R-Car M2-W)
- "renesas,dmac-r8a7792" (R-Car V2H)
^ permalink raw reply related
* [RFC,4/6] dma: Add Synopsys eDMA IP PCIe glue-logic
From: Gustavo Pimentel @ 2018-12-13 14:40 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
Hi,
On 12/12/2018 13:25, Andy Shevchenko wrote:
> On Wed, Dec 12, 2018 at 12:13:24PM +0100, Gustavo Pimentel wrote:
>> Synopsys eDMA IP is normally distributed along with Synopsys PCIe
>> EndPoint IP (depends of the use and licensing agreement).
>>
>> This IP requires some basic configurations, such as:
>> - eDMA registers BAR
>> - eDMA registers offset
>> - eDMA linked list BAR
>> - eDMA linked list offset
>> - eDMA linked list size
>> - eDMA version
>> - eDMA mode
>>
>> As a working example, PCIe glue-logic will attach to a Synopsys PCIe
>> EndPoint IP prototype kit (Vendor ID = 0x16c3, Device ID = 0xedda),
>> which has built-in an eDMA IP with this default configuration:
>> - eDMA registers BAR = 0
>> - eDMA registers offset = 0x1000 (4 Kbytes)
>> - eDMA linked list BAR = 2
>> - eDMA linked list offset = 0x0 (0 Kbytes)
>> - eDMA linked list size = 0x20000 (128 Kbytes)
>> - eDMA version = 0
>> - eDMA mode = EDMA_MODE_UNROLL
>>
>> This driver can be compile as built-in or external module in kernel.
>>
>> To enable this driver just select DW_EDMA_PCIE option in kernel
>> configuration, however it requires and selects automatically DW_EDMA
>> option too.
>
> It seems this driver somehow written as a copy-paste of existing pieces w/o good reasons to do such.
What do you mean? It's true I relied on existing drivers to develop this
solution, but is not it supposed to be so?
>
>> +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.
>
>> +
>> +static int dw_edma_pcie_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *pid)
>> +{
>> + const struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
>> + struct device *dev = &pdev->dev;
>> + struct dw_edma_chip *chip;
>> + struct dw_edma *dw;
>> + void __iomem *reg;
>> + int err, irq = -1;
>> + u32 addr_hi, addr_lo;
>> + u16 flags;
>> + u8 cap_off;
>> +
>> + if (!pdata) {
>> + dev_err(dev, "%s missing data struture\n",
>> + pci_name(pdev));
>> + return -EFAULT;
>> + }
>> +
>> + err = pcim_enable_device(pdev);
>> + if (err) {
>> + dev_err(dev, "%s enabling device failed\n",
>> + pci_name(pdev));
>> + return err;
>> + }
>> +
>
>> + err = pcim_iomap_regions(pdev, 1 << pdata->regs_bar, pci_name(pdev));
>> + if (err) {
>> + dev_err(dev, "%s eDMA register BAR I/O memory remapping failed\n",
>> + pci_name(pdev));
>> + return err;
>> + }
>> +
>> + err = pcim_iomap_regions(pdev, 1 << pdata->ll_bar, pci_name(pdev));
>> + if (err) {
>> + dev_err(dev, "%s eDMA linked list BAR I/O remapping failed\n",
>> + pci_name(pdev));
>> + return err;
>> + }
>
> This could be done in one call.
Yes, that's true. I will change that.
>
>> +
>> + pci_set_master(pdev);
>> +
>
>> + 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.
>
>> +
>> + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
>> + if (err) {
>> + dev_err(dev, "%s DMA mask set failed\n",
>> + pci_name(pdev));
>> + return err;
>> + }
>> +
>> + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
>> + if (err) {
>> + dev_err(dev, "%s consistent DMA mask set failed\n",
>> + pci_name(pdev));
>> + return err;
>> + }
>> +
>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> + if (!chip)
>> + return -ENOMEM;
>> +
>> + dw = devm_kzalloc(&pdev->dev, sizeof(*dw), GFP_KERNEL);
>> + if (!dw)
>> + return -ENOMEM;
>> +
>> + irq = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI | PCI_IRQ_MSIX);
>> + if (irq < 0) {
>> + dev_err(dev, "%s failed to alloc IRQ vector\n",
>> + pci_name(pdev));
>> + return -EPERM;
>> + }
>> +
>> + chip->dw = dw;
>> + chip->dev = dev;
>> + chip->id = pdev->devfn;
>> + chip->irq = pdev->irq;
>> +
>> + dw->regs = pcim_iomap_table(pdev)[pdata->regs_bar];
>> + dw->regs += pdata->regs_off;
>> +
>> + dw->va_ll = pcim_iomap_table(pdev)[pdata->ll_bar];
>> + dw->va_ll += pdata->ll_off;
>> + dw->pa_ll = pdev->resource[pdata->ll_bar].start;
>> + dw->pa_ll += pdata->ll_off;
>> + dw->ll_sz = pdata->ll_sz;
>> +
>> + dw->msi_addr = 0;
>> + dw->msi_data = 0;
>> +
>> + dw->version = pdata->version;
>> + dw->mode = pdata->mode;
>> +
>> + dev_info(dev, "Version:\t%u\n", dw->version);
>> +
>> + 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 *’
>
>> +
>> + dev_info(dev,
>> + "L. List:\tBAR=%u, off=0x%.16llx B, sz=0x%.8x B, vaddr=0x%.8lx, paddr=0x%.8lx",
>> + pdata->ll_bar, pdata->ll_off, pdata->ll_sz,
>> + (unsigned long) dw->va_ll,
>> + (unsigned long) dw->pa_ll);
>
> This is noise, either remove or move to dbg level.
I'll move it to debug.
>
>> + 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.
>
>> +
>> + 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
>
> There is a helper from PCI core for this.
>
>> + dev_err(dev, "%s enable interrupt failed\n",
>> + pci_name(pdev));
>> + return -EPERM;
>> + }
>> +
>> + err = dw_edma_probe(chip);
>> + if (err) {
>> + dev_err(dev, "%s eDMA probe failed\n",
>> + pci_name(pdev));
>> + return err;
>> + }
>> +
>> + pci_set_drvdata(pdev, chip);
>> +
>> + dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
>> +
>> + return 0;
>> +}
>> +
>> +static void dw_edma_pcie_remove(struct pci_dev *pdev)
>> +{
>> + struct dw_edma_chip *chip = pci_get_drvdata(pdev);
>> + struct device *dev = &pdev->dev;
>> + int err;
>> +
>> + err = dw_edma_remove(chip);
>> + if (err) {
>> + dev_warn(dev, "%s can't remove device properly: %d\n",
>> + pci_name(pdev), err);
>
> dev_warn + dev_name ?! Have you tried to see what would be the output?
No, I didn't, lol. That would be fun at least.
>
>> + }
>> +
>> + pci_free_irq_vectors(pdev);
>> +
>> + dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
>> +}
>> +
>
>> +#ifdef CONFIG_PM_SLEEP
>
> You can use __maybe_unused instead of this.
I agree.
>
>> +#endif /* CONFIG_PM_SLEEP */
>
Thanks Andy!
^ permalink raw reply
* [RFC,2/6] dma: Add Synopsys eDMA IP version 0 support
From: Gustavo Pimentel @ 2018-12-13 11:51 UTC (permalink / raw)
To: Eugeniy Paltsev, dmaengine@vger.kernel.org,
linux-pci@vger.kernel.org, gustavo.pimentel@synopsys.com
Cc: vkoul@kernel.org, andriy.shevchenko@linux.intel.com, Joao Pinto,
Alexey Brodkin
Hi,
On 12/12/2018 13:39, Eugeniy Paltsev wrote:
> Hi Gustavo,
>
> On Wed, 2018-12-12 at 12:13 +0100, Gustavo Pimentel wrote:
>> Add support for the eDMA IP version 0 driver for both register maps (legacy
>> and unroll).
> [snip]
>> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
>> new file mode 100644
>> index 0000000..cc362b0
>> --- /dev/null
>> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
>> @@ -0,0 +1,346 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
>> +// Synopsys DesignWare eDMA v0 core
>> +
>> +#include "dw-edma-core.h"
>> +#include "dw-edma-v0-core.h"
>> +#include "dw-edma-v0-regs.h"
>> +#include "dw-edma-v0-debugfs.h"
>> +
>> +#define QWORD_HI(value) ((value & 0xFFFFFFFF00000000llu) >> 32)
> Use upper_32_bits() macros.
>
>> +#define QWORD_LO(value) (value & 0x00000000FFFFFFFFllu)
> Use lower_32_bits() macros.
Ok, makes sense.
>
>> +enum dw_edma_control {
>> + DW_EDMA_CB = BIT(0),
>> + DW_EDMA_TCB = BIT(1),
>> + DW_EDMA_LLP = BIT(2),
>> + DW_EDMA_LIE = BIT(3),
>> + DW_EDMA_RIE = BIT(4),
>
>
> [snip]
>> + viewport_sel = (ch & 0x00000007ul);
> [snip]
>> +void dw_edma_v0_core_off(struct dw_edma *dw)
>> +{
>> + SET_BOTH(dw, int_mask, 0x00FF00FFul);
>> + SET_BOTH(dw, int_clear, 0x00FF00FFul);
>> + SET_BOTH(dw, engine_en, 0x00000000ul);
>> +}
> [snip]
>> + num_ch &= 0x0000000Ful;
>> + } else {
>> + num_ch &= 0x000F0000ul;
>
> I guess it's better to name this magic numbers via defines.
>
Ok, also makes sense.
>
> [snip]
>
>> +
>> +bool dw_edma_v0_core_status_done_int(struct dw_edma_chan *chan)
>> +{
>> + struct dw_edma *dw = chan->chip->dw;
>> + u32 tmp;
>> +
>> + tmp = GET_RW(dw, chan->dir, int_status);
>> + tmp &= BIT(chan->id);
>> +
>> + return tmp ? true : false;
>
> return !!tmp;
Ok, I'll do it.
>
>> +}
>> +
>> +bool dw_edma_v0_core_status_abort_int(struct dw_edma_chan *chan)
>> +{
>> + struct dw_edma *dw = chan->chip->dw;
>> + u32 tmp;
>> +
>> + tmp = GET_RW(dw, chan->dir, int_status);
>> + tmp &= BIT(chan->id + 16);
>> +
>> + return tmp ? true : false;
> ditto.
>
Thanks!
^ permalink raw reply
* [RFC,6/6] pci: pci_ids: Add Synopsys device id 0xedda
From: Gustavo Pimentel @ 2018-12-13 11:49 UTC (permalink / raw)
To: Bjorn Helgaas, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Kishon Vijay Abraham I, Lorenzo Pieralisi, Joao Pinto
Hi,
On 12/12/2018 23:03, Bjorn Helgaas wrote:
> On Wed, Dec 12, 2018 at 12:13:26PM +0100, Gustavo Pimentel wrote:
>> Create and add Synopsys device id (0xedda) to pci id list, since this id
>> is now being use on two different drivers (pci_endpoint_test.ko and
>> dw-edma-pcie.ko).
>
> Please run "git log --oneline include/linux/pci_ids.h" and make your
> subject line match, i.e.,
>
> PCI: Add Synopsys endpoint EDDA Device ID
>
> Also, if you want, you can reorder this to add the ID first and use it
> in drivers/misc/pci_endpoint_test.c, and then just use it from the
> first appearance of drivers/dma/dw-edma/dw-edma-pcie.c. Either way is
> fine.
>
> For the include/linux/pci_ids.h change,
Ok, I'll change it.
Thanks.
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> ---
>> drivers/dma/dw-edma/dw-edma-pcie.c | 2 +-
>> drivers/misc/pci_endpoint_test.c | 2 +-
>> include/linux/pci_ids.h | 1 +
>> 3 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
>> index f29a861..50e0db4 100644
>> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
>> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
>> @@ -280,7 +280,7 @@ static const struct dev_pm_ops dw_edma_pcie_dev_pm_ops = {
>> };
>>
>> static const struct pci_device_id dw_edma_pcie_id_table[] = {
>> - { PCI_DEVICE_DATA(SYNOPSYS, 0xedda, &snps_edda_data) },
>> + { PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
>> { }
>> };
>> MODULE_DEVICE_TABLE(pci, dw_edma_pcie_id_table);
>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>> index 896e2df..d27efe838 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -788,7 +788,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
>> static const struct pci_device_id pci_endpoint_test_tbl[] = {
>> { PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
>> { PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
>> - { PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, 0xedda) },
>> + { PCI_DEVICE_DATA(SYNOPSYS, EDDA, NULL) },
>> { }
>> };
>> MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);
>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>> index 69f0abe..57f17dd 100644
>> --- a/include/linux/pci_ids.h
>> +++ b/include/linux/pci_ids.h
>> @@ -2358,6 +2358,7 @@
>> #define PCI_DEVICE_ID_CENATEK_IDE 0x0001
>>
>> #define PCI_VENDOR_ID_SYNOPSYS 0x16c3
>> +#define PCI_DEVICE_ID_SYNOPSYS_EDDA 0xedda
>>
>> #define PCI_VENDOR_ID_VITESSE 0x1725
>> #define PCI_DEVICE_ID_VITESSE_VSC7174 0x7174
>> --
>> 2.7.4
>>
^ permalink raw reply
* [v5,1/2] dmaengine: 8250_mtk_dma: add Mediatek uart DMA support
From: Long Cheng @ 2018-12-13 11:36 UTC (permalink / raw)
To: Sean Wang
Cc: vkoul, robh+dt, mark.rutland, ryder.lee, Matthias Brugger,
dan.j.williams, gregkh, jslaby,
Sean Wang (王志亘), dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, yingjoe.chen, YT Shen, Long Cheng
On Tue, 2018-12-11 at 15:12 -0800, Sean Wang wrote:
> Sorry for that I didn't have a full review at one time in the earlier version
>
> On Mon, Dec 10, 2018 at 9:37 PM Long Cheng
> <long.cheng@mediatek.com> wrote:
> >
> > In DMA engine framework, add 8250 mtk dma to support it.
>
> It looks like there are still many rooms to improve the description,
> especially it's a totally new driver.
>
> >
> > Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> > ---
> > drivers/dma/mediatek/8250_mtk_dma.c | 830 +++++++++++++++++++++++++++++++++++
> > drivers/dma/mediatek/Kconfig | 11 +
> > drivers/dma/mediatek/Makefile | 1 +
> > 3 files changed, 842 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..f79d180
> > --- /dev/null
> > +++ b/drivers/dma/mediatek/8250_mtk_dma.c
> > @@ -0,0 +1,830 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Mediatek 8250 DMA driver.
>
> MediaTek
>
> > + *
> > + * 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/list.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_dma.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/iopoll.h>
> > +
> > +#include "../virt-dma.h"
> > +
> > +#define MTK_APDMA_DEFAULT_REQUESTS 127
> > +#define MTK_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_FLUSH_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 MTK_DMA_RING_SIZE 0xffffU
> > +/* invert this bit when wrap ring head again*/
> > +#define MTK_DMA_RING_WRAP 0x10000U
> > +
> > +#define VFF_INT_FLAG 0x00
> > +#define VFF_INT_EN 0x04
> > +#define VFF_EN 0x08
> > +#define VFF_RST 0x0c
> > +#define VFF_STOP 0x10
> > +#define VFF_FLUSH 0x14
> > +#define VFF_ADDR 0x1c
> > +#define VFF_LEN 0x24
> > +#define VFF_THRE 0x28
> > +#define VFF_WPT 0x2c
> > +#define VFF_RPT 0x30
> > +/*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
> > +#define VFF_VALID_SIZE 0x3c
> > +/*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
> > +#define VFF_LEFT_SIZE 0x40
> > +#define VFF_DEBUG_STATUS 0x50
> > +#define VFF_4G_SUPPORT 0x54
> > +
> > +struct mtk_dmadev {
> > + struct dma_device ddev;
> > + void __iomem *mem_base[MTK_APDMA_CHANNELS];
> > + spinlock_t lock; /* dma dev lock */
> > + struct tasklet_struct task;
>
> we can drop tasklet and instead allows descriptors to be handled as
> fast as possible.
> similar suggestions have been made in the other dmaengine [1] and mtk-hsdma.c
>
OK, i will remove these, and improve it.
> [1] https://lkml.org/lkml/2018/11/11/146
>
> > + struct list_head pending;
> > + struct clk *clk;
> > + unsigned int dma_requests;
> > + bool support_33bits;
> > + unsigned int dma_irq[MTK_APDMA_CHANNELS];
> > + struct mtk_chan *ch[MTK_APDMA_CHANNELS];
> > +};
> > +
> > +struct mtk_chan {
> > + struct virt_dma_chan vc;
> > + struct list_head node;
> > + struct dma_slave_config cfg;
> > + void __iomem *base;
> > + struct mtk_dma_desc *desc;
> > +
> > + bool stop;
> > + bool requested;
> > +
> > + unsigned int rx_status;
> > +};
> > +
> > +struct mtk_dma_sg {
> > + dma_addr_t addr;
> > + unsigned int en; /* number of elements (24-bit) */
> > + unsigned int fn; /* number of frames (16-bit) */
> > +};
> > +
> > +struct mtk_dma_desc {
> > + struct virt_dma_desc vd;
> > + enum dma_transfer_direction dir;
> > +
> > + unsigned int sglen;
> > + struct mtk_dma_sg sg[0];
> > +
> > + unsigned int len;
> > +};
> > +
> > +static inline struct mtk_dmadev *to_mtk_dma_dev(struct dma_device *d)
> > +{
> > + return container_of(d, struct mtk_dmadev, ddev);
> > +}
> > +
> > +static inline struct mtk_chan *to_mtk_dma_chan(struct dma_chan *c)
> > +{
> > + return container_of(c, struct mtk_chan, vc.chan);
> > +}
> > +
> > +static inline struct mtk_dma_desc *to_mtk_dma_desc
> > + (struct dma_async_tx_descriptor *t)
> > +{
> > + return container_of(t, struct mtk_dma_desc, vd.tx);
> > +}
> > +
> > +static void mtk_dma_chan_write(struct mtk_chan *c,
> > + unsigned int reg, unsigned int val)
> > +{
> > + writel(val, c->base + reg);
> > +}
> > +
> > +static unsigned int mtk_dma_chan_read(struct mtk_chan *c, unsigned int reg)
> > +{
> > + return readl(c->base + reg);
> > +}
> > +
> > +static void mtk_dma_desc_free(struct virt_dma_desc *vd)
> > +{
> > + struct dma_chan *chan = vd->tx.chan;
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + kfree(c->desc);
> > + c->desc = NULL;
> > +}
> > +
> > +static void mtk_dma_tx_flush(struct dma_chan *chan)
> > +{
>
> If the user is only one, let's span the content into where the user is.
>
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + if (mtk_dma_chan_read(c, VFF_FLUSH) == 0U)
> > + mtk_dma_chan_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > +}
> > +
> > +static void mtk_dma_tx_write(struct dma_chan *chan)
> > +{
>
> If the user is only one, let's span the content into where the user is.
>
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + unsigned int txcount = c->desc->len;
> > + unsigned int len, send, left, wpt, wrap;
> > +
> > + len = mtk_dma_chan_read(c, VFF_LEN);
> > +
> > + while ((left = mtk_dma_chan_read(c, VFF_LEFT_SIZE)) > 0U) {
> > + if (c->desc->len == 0U)
>
> merge the condition back into the condition in while
>
> > + break;
> > + send = min_t(unsigned int, left, c->desc->len);
> > + wpt = mtk_dma_chan_read(c, VFF_WPT);
> > + wrap = wpt & MTK_DMA_RING_WRAP ? 0U : MTK_DMA_RING_WRAP;
> > +
> > + if ((wpt & (len - 1U)) + send < len)
> > + mtk_dma_chan_write(c, VFF_WPT, wpt + send);
> > + else
> > + mtk_dma_chan_write(c, VFF_WPT,
> > + ((wpt + send) & (len - 1U))
> > + | wrap);
> > +
> > + c->desc->len -= send;
>
> ->len can be renamed to ->avail_len to say it's variable during the work
>
> > + }
> > +
> > + if (txcount != c->desc->len) {
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > + mtk_dma_tx_flush(chan);
> > + }
> > +}
> > +
> > +static void mtk_dma_start_tx(struct mtk_chan *c)
> > +{
> > + if (mtk_dma_chan_read(c, VFF_LEFT_SIZE) == 0U)
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > + else
> > + mtk_dma_tx_write(&c->vc.chan);
> > +
> > + c->stop = false;
> > +}
> > +
> > +static void mtk_dma_get_rx_size(struct mtk_chan *c)
> > +{
>
> If the user is only one, let's span the content into where the user is.
>
> > + unsigned int rx_size = mtk_dma_chan_read(c, VFF_LEN);
> > + unsigned int rdptr, wrptr, wrreg, rdreg, count;
>
> too much variable seems a little lousy, two variables are enough
>
> unsigned int rd, wr;
>
> > +
> > + rdreg = mtk_dma_chan_read(c, VFF_RPT);
> > + wrreg = mtk_dma_chan_read(c, VFF_WPT);
> > + rdptr = rdreg & MTK_DMA_RING_SIZE;
> > + wrptr = wrreg & MTK_DMA_RING_SIZE;
>
> rd = mtk_dma_chan_read(c, VFF_RPT) & MTK_DMA_RING_SIZE;
> wr = mtk_dma_chan_read(c, VFF_WPT) & MTK_DMA_RING_SIZE
>
> > + count = ((rdreg ^ wrreg) & MTK_DMA_RING_WRAP) ?
> > + (wrptr + rx_size - rdptr) : (wrptr - rdptr);
> > +
> > + c->rx_status = count;
>
> drop the variable count and have a direct assignment
>
> > +
> > + mtk_dma_chan_write(c, VFF_RPT, wrreg);
> > +}
> > +
> > +static void mtk_dma_start_rx(struct mtk_chan *c)
> > +{
> > + struct dma_chan *chan = &c->vc.chan;
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_dma_desc *d = c->desc;
> > +
> > + if (mtk_dma_chan_read(c, VFF_VALID_SIZE) == 0U)
> > + return;
> > +
> > + if (d && vchan_next_desc(&c->vc)) {
> > + mtk_dma_get_rx_size(c);
> > + list_del(&d->vd.node);
> > + vchan_cookie_complete(&d->vd);
> > + } else {
> > + spin_lock(&mtkd->lock);
> > + if (list_empty(&mtkd->pending))
> > + list_add_tail(&c->node, &mtkd->pending);
> > + spin_unlock(&mtkd->lock);
> > + tasklet_schedule(&mtkd->task);
> > + }
> > +}
> > +
> > +static void mtk_dma_reset(struct mtk_chan *c)
> > +{
>
> If the user is only one, let's span the content into where the user is.
>
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> > + u32 status;
> > + int ret;
> > +
> > + mtk_dma_chan_write(c, VFF_ADDR, 0);
> > + mtk_dma_chan_write(c, VFF_THRE, 0);
> > + mtk_dma_chan_write(c, VFF_LEN, 0);
> > + mtk_dma_chan_write(c, VFF_RST, VFF_WARM_RST_B);
> > +
> > + ret = readx_poll_timeout(readl,
> > + c->base + VFF_EN,
> > + status, status == 0, 10, 100);
> > + if (ret) {
> > + dev_err(c->vc.chan.device->dev,
> > + "dma reset: fail, timeout\n");
> > + return;
> > + }
> > +
> > + if (c->cfg.direction == DMA_DEV_TO_MEM)
> > + mtk_dma_chan_write(c, VFF_RPT, 0);
> > + else if (c->cfg.direction == DMA_MEM_TO_DEV)
> > + mtk_dma_chan_write(c, VFF_WPT, 0);
>
> using switch and case statement
>
> > +
> > + if (mtkd->support_33bits)
> > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> > +}
> > +
> > +static void mtk_dma_stop(struct mtk_chan *c)
>
> If the user is only one, let's span the content into where the user is.
>
> > +{
> > + u32 status;
> > + int ret;
> > +
> > + mtk_dma_chan_write(c, VFF_FLUSH, VFF_FLUSH_CLR_B);
> > + /* Wait for flush */
> > + ret = readx_poll_timeout(readl,
> > + c->base + VFF_FLUSH,
> > + status,
> > + (status & VFF_FLUSH_B) != VFF_FLUSH_B,
> > + 10, 100);
> > + if (ret)
> > + dev_err(c->vc.chan.device->dev,
> > + "dma stop: polling FLUSH fail, DEBUG=0x%x\n",
> > + mtk_dma_chan_read(c, VFF_DEBUG_STATUS));
> > +
> > + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> > + mtk_dma_chan_write(c, VFF_STOP, VFF_STOP_B);
> > + ret = readx_poll_timeout(readl,
> > + c->base + VFF_EN,
> > + status, status == 0, 10, 100);
> > + if (ret)
> > + dev_err(c->vc.chan.device->dev,
> > + "dma stop: polling VFF_EN fail, DEBUG=0x%x\n",
> > + mtk_dma_chan_read(c, VFF_DEBUG_STATUS));
> > +
> > + mtk_dma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> > +
> > + if (c->cfg.direction == DMA_DEV_TO_MEM)
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + else
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
>
> using switch and case statement
>
> > +
> > + c->stop = true;
> > +}
> > +
> > +/*
> > + * This callback schedules all pending channels. We could be more
> > + * clever here by postponing allocation of the real DMA channels to
> > + * this point, and freeing them when our virtual channel becomes idle.
> > + *
> > + * We would then need to deal with 'all channels in-use'
> > + */
> > +static void mtk_dma_sched(unsigned long data)
> > +{
>
> As at the initial be said, try to make descriptors submit as fast as
> possible without involving in a tasklet. The same improvement had been
> done at mtk-hsdma.c so you could have a reference to it first if you
> have no much idea of how to begin to improve.
>
> > + struct mtk_dmadev *mtkd = (struct mtk_dmadev *)data;
> > + struct virt_dma_desc *vd;
> > + struct mtk_chan *c;
> > + unsigned long flags;
> > + LIST_HEAD(head);
> > +
> > + spin_lock_irq(&mtkd->lock);
> > + list_splice_tail_init(&mtkd->pending, &head);
> > + spin_unlock_irq(&mtkd->lock);
> > +
> > + if (!list_empty(&head)) {
> > + c = list_first_entry(&head, struct mtk_chan, node);
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > + list_del_init(&c->node);
> > + mtk_dma_start_rx(c);
> > + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> > + vd = vchan_next_desc(&c->vc);
> > + c->desc = to_mtk_dma_desc(&vd->tx);
> > + list_del_init(&c->node);
> > + mtk_dma_start_tx(c);
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > + }
> > +}
> > +
> > +static int mtk_dma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + int ret = -EBUSY;
> > +
> > + pm_runtime_get_sync(mtkd->ddev.dev);
> > +
> > + if (!mtkd->ch[chan->chan_id]) {
> > + c->base = mtkd->mem_base[chan->chan_id]
>
> mtkd->mem_base is unnecessary, we can directly decide c->base in the
> driver probe stage
>
> > + mtkd->ch[chan->chan_id] = c;
>
> mtkd->ch is also unnecessary, the core always pass struct dma_chan
> *chan to each callback function
>
> > + ret = 1;
>
> ret be 1 seems be wrong
>
> > + }
> > + c->requested = false;
> > + mtk_dma_reset(c);
> > +
> > + return ret;
> > +}
> > +
> > +static void mtk_dma_free_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + if (c->requested) {
> > + c->requested = false;
> > + free_irq(mtkd->dma_irq[chan->chan_id], chan);
>
> it makes not consistent because there are not request_irq present at
> mtk_dma_alloc_chan_resources. but I'd prefer a devm_request_irq is
> being done
> as the driver got probe.
>
> > + }
> > +
> > + tasklet_kill(&mtkd->task);
> > + tasklet_kill(&c->vc.task);
> > +
> > + c->base = NULL;
> > + mtkd->ch[chan->chan_id] = NULL;
> > + vchan_free_chan_resources(&c->vc);
> > +
> > + pm_runtime_put_sync(mtkd->ddev.dev);
> > +}
> > +
> > +static enum dma_status mtk_dma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_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_dma_chan_read(c, VFF_RPT)
> > + & MTK_DMA_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;
> > +}
> > +
> > +static struct dma_async_tx_descriptor *mtk_dma_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_dma_chan(chan);
> > + struct scatterlist *sgent;
> > + struct mtk_dma_desc *d;
> > + struct mtk_dma_sg *sg;
> > + unsigned int size, i, j, en;
> > +
> > + en = 1;
> > +
> > + if ((dir != DMA_DEV_TO_MEM) &&
> > + (dir != DMA_MEM_TO_DEV)) {
> > + dev_err(chan->device->dev, "bad direction\n");
> > + return NULL;
> > + }
> > +
> > + /* Now allocate and setup the descriptor. */
> > + d = kzalloc(sizeof(*d) + sglen * sizeof(d->sg[0]), GFP_ATOMIC);
> > + if (!d)
> > + return NULL;
> > +
> > + d->dir = dir;
> > +
> > + j = 0;
> > + for_each_sg(sgl, sgent, sglen, i) {
> > + d->sg[j].addr = sg_dma_address(sgent);
> > + d->sg[j].en = en;
> > + d->sg[j].fn = sg_dma_len(sgent) / en;
> > + j++;
> > + }
> > +
> > + d->sglen = j;
> > +
> > + if (dir == DMA_MEM_TO_DEV) {
> > + for (size = i = 0; i < d->sglen; i++) {
> > + sg = &d->sg[i];
> > + size += sg->en * sg->fn;
> > + }
> > + d->len = size;
> > + }
> > +
>
> The driver always only handles data move for the single contiguous
> area, but it seems the callback must provide the scatter-gather
> function to the dmaegine. otherwise, why is the callback be called
> device_prep_slave_sg?
>
because in 8250 UART native code, call the device_prep_slave_sg. we just
use one ring buffer.
> > + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> > +}
> > +
> > +static void mtk_dma_issue_pending(struct dma_chan *chan)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct virt_dma_desc *vd;
> > + struct mtk_dmadev *mtkd;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > + mtkd = to_mtk_dma_dev(chan->device);
>
> mtkd can be dropped as it seems no users
>
> > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > + vd = vchan_next_desc(&c->vc);
> > + c->desc = to_mtk_dma_desc(&vd->tx);
> > + }
> > + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > + vd = vchan_next_desc(&c->vc);
> > + c->desc = to_mtk_dma_desc(&vd->tx);
> > + mtk_dma_start_tx(c);
> > + }
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +}
> > +
> > +static irqreturn_t mtk_dma_rx_interrupt(int irq, void *dev_id)
> > +{
> > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > +
> > + mtk_dma_start_rx(c);
> > +
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t mtk_dma_tx_interrupt(int irq, void *dev_id)
> > +{
> > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct mtk_dma_desc *d = c->desc;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (d->len != 0U) {
> > + list_add_tail(&c->node, &mtkd->pending);
> > + tasklet_schedule(&mtkd->task);
> > + } else {
> > + list_del(&d->vd.node);
> > + vchan_cookie_complete(&d->vd);
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_dma_slave_config(struct dma_chan *chan,
> > + struct dma_slave_config *cfg)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> > + int ret;
> > +
> > + c->cfg = *cfg;
> > +
> > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > + unsigned int rx_len = cfg->src_addr_width * 1024;
>
> it seems you should use cfg->src_port_window_size as the comments explains
>
> * @src_port_window_size: The length of the register area in words the data need
> * to be accessed on the device side. It is only used for devices which is using
> * an area instead of a single register to receive the data. Typically the DMA
> * loops in this area in order to transfer the data.
> * @dst_port_window_size: same as src_port_window_size but for the destination
> * port.
>
in 8250 UART native code, will set the parameter. if want to modify
this, i think that maybe at next kernel release, we can modify it. i
suggest that not modify it at this patch. because it relate of 8250 uart
code. thanks.
> > +
> > + mtk_dma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > + mtk_dma_chan_write(c, VFF_LEN, rx_len);
> > + mtk_dma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > + mtk_dma_chan_write(c,
> > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > + | VFF_RX_INT_EN1_B);
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
>
> I'd prefer to move those channel interrupt enablement to
> .device_alloc_chan_resources
> and related disablement to .device_free_chan_resources
>
i think that, first need set the config to HW, and the enable the DMA.
> > +
> > + if (!c->requested) {
> > + c->requested = true;
> > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > + mtk_dma_rx_interrupt,
> > + IRQF_TRIGGER_NONE,
> > + KBUILD_MODNAME, chan);
>
> ISR registration usually happens as the driver got probe, it can give
> the system more flexibility to manage such IRQ affinity on the fly.
>
i will move the request irq to alloc channel.
> > + if (ret < 0) {
> > + dev_err(chan->device->dev, "Can't request rx dma IRQ\n");
> > + return -EINVAL;
> > + }
> > + }
> > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > + unsigned int tx_len = cfg->dst_addr_width * 1024;
>
> Ditto as above, it seems you should use cfg->dst_port_window_size
>
> > +
> > + mtk_dma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > + mtk_dma_chan_write(c, VFF_LEN, tx_len);
> > + mtk_dma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
>
> ditto, I'd prefer to move those channel interrupt enablement to
> .device_alloc_chan_resources and related disablement to
> .device_free_chan_resources
>
> > +
> > + if (!c->requested) {
> > + c->requested = true;
> > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > + mtk_dma_tx_interrupt,
> > + IRQF_TRIGGER_NONE,
> > + KBUILD_MODNAME, chan);
>
> ditto, we can request ISR with devm_request_irq in the driver got
> probe and trim the c->request member
>
> > + if (ret < 0) {
> > + dev_err(chan->device->dev, "Can't request tx dma IRQ\n");
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > +
> > + if (mtkd->support_33bits)
> > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > +
> > + if (mtk_dma_chan_read(c, VFF_EN) != VFF_EN_B) {
> > + dev_err(chan->device->dev,
> > + "config dma dir[%d] fail\n", cfg->direction);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_terminate_all(struct dma_chan *chan)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + list_del_init(&c->node);
> > + mtk_dma_stop(c);
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_device_pause(struct dma_chan *chan)
> > +{
> > + /* just for check caps pass */
> > + return -EINVAL;
>
> always return error code seems not the client driver wants us to do.
>
> maybe if the hardware doesn't support pause, we can make a software
> pause, that waits until all active descriptors in hardware done, then
> disable interrupt and then stop handling the following vd in the
> vchan.
>
> > +}
the code can't run. just for 8250 native code to check the function
pointer. in the future, if the function useful, we can realize. thanks.
> > +
> > +static int mtk_dma_device_resume(struct dma_chan *chan)
> > +{
> > + /* just for check caps pass */
> > + return -EINVAL;
>
> similar to the above
>
> > +}
> > +
> > +static void mtk_dma_free(struct mtk_dmadev *mtkd)
> > +{
> > + tasklet_kill(&mtkd->task);
> > + while (list_empty(&mtkd->ddev.channels) == 0) {
> !list_empty(&mtkd->ddev.channels)
> > + 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);
> > + devm_kfree(mtkd->ddev.dev, c);
>
> no need to call devm_kfree, the core would help do this
>
> > + }
> > +}
> > +
> > +static const struct of_device_id mtk_uart_dma_match[] = {
> > + { .compatible = "mediatek,mt6577-uart-dma", },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_uart_dma_match);
> > +
> > +static int mtk_apdma_probe(struct platform_device *pdev)
> > +{
> > + struct mtk_dmadev *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;
> > +
> > + for (i = 0; i < MTK_APDMA_CHANNELS; i++) {
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > + if (!res)
> > + return -ENODEV;
> > + mtkd->mem_base[i] = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(mtkd->mem_base[i]))
> > + return PTR_ERR(mtkd->mem_base[i]);
> > + }
> > +
> > + for (i = 0; i < MTK_APDMA_CHANNELS; i++) {
> > + 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);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(mtkd->clk)) {
> > + dev_err(&pdev->dev, "No clock specified\n");
> > + return PTR_ERR(mtkd->clk);
> > + }
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
> > + dev_info(&pdev->dev, "Support dma 33bits\n");
> > + mtkd->support_33bits = true;
> > + }
> > +
> > + if (mtkd->support_33bits)
> > + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(33));
> > + else
> > + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>
> rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32 |
> mtkd->support_33bits));
>
> > + if (rc)
> > + return rc;
> > +
> > + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> > + mtkd->ddev.device_alloc_chan_resources = mtk_dma_alloc_chan_resources;
> > + mtkd->ddev.device_free_chan_resources = mtk_dma_free_chan_resources;
> > + mtkd->ddev.device_tx_status = mtk_dma_tx_status;
> > + mtkd->ddev.device_issue_pending = mtk_dma_issue_pending;
> > + mtkd->ddev.device_prep_slave_sg = mtk_dma_prep_slave_sg;
> > + mtkd->ddev.device_config = mtk_dma_slave_config;
> > + mtkd->ddev.device_pause = mtk_dma_device_pause;
> > + mtkd->ddev.device_resume = mtk_dma_device_resume;
> > + mtkd->ddev.device_terminate_all = mtk_dma_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);
> > + INIT_LIST_HEAD(&mtkd->pending);
> > +
> > + spin_lock_init(&mtkd->lock);
> > + tasklet_init(&mtkd->task, mtk_dma_sched, (unsigned long)mtkd);
> > +
> > + mtkd->dma_requests = MTK_APDMA_DEFAULT_REQUESTS;
> > + if (of_property_read_u32(pdev->dev.of_node,
> > + "dma-requests", &mtkd->dma_requests)) {
> > + dev_info(&pdev->dev,
> > + "Missing dma-requests property, using %u.\n",
> > + MTK_APDMA_DEFAULT_REQUESTS);
> > + }
> > +
> > + for (i = 0; i < MTK_APDMA_CHANNELS; i++) {
> > + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> > + if (!c)
> > + goto err_no_dma;
> > +
> > + c->vc.desc_free = mtk_dma_desc_free;
> > + vchan_init(&c->vc, &mtkd->ddev);
> > + INIT_LIST_HEAD(&c->node);
> > + }
> > +
> > + 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_dma_free(mtkd);
> > + return rc;
> > +}
> > +
> > +static int mtk_apdma_remove(struct platform_device *pdev)
> > +{
> > + struct mtk_dmadev *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_dma_free(mtkd);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mtk_dma_suspend(struct device *dev)
> > +{
> > + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + if (!pm_runtime_suspended(dev))
> > + clk_disable_unprepare(mtkd->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_resume(struct device *dev)
> > +{
> > + int ret;
> > + struct mtk_dmadev *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_dma_runtime_suspend(struct device *dev)
> > +{
> > + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(mtkd->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_runtime_resume(struct device *dev)
> > +{
> > + int ret;
> > + struct mtk_dmadev *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_dma_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mtk_dma_suspend, mtk_dma_resume)
> > + SET_RUNTIME_PM_OPS(mtk_dma_runtime_suspend,
> > + mtk_dma_runtime_resume, NULL)
> > +};
> > +
> > +static struct platform_driver mtk_dma_driver = {
>
> mtk_dma is much general and all functions and structures in the driver
> should be all consistent. I'd prefer to have all naming starts with
> mtk_uart_apdma.
>
the function name and parameters' name, i will modify it. but before the
8250_mtk.c and the Doc. are recorded. if modify file name and Kconfig,
will bring about the disorder. i suggest that after the patch is
recorded, modify this. thanks.
> > + .probe = mtk_apdma_probe,
>
> such as
> mtk_uart_apdma_probe
>
> > + .remove = mtk_apdma_remove,
>
> mtk_uart_apdma_remove
>
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .pm = &mtk_dma_pm_ops,
>
> mtk_uart_apdma_pm_ops
>
> > + .of_match_table = of_match_ptr(mtk_uart_dma_match),
>
> mtk_uart_apdma_match
>
> > + },
> > +};
> > +
> > +module_platform_driver(mtk_dma_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..d399624 100644
> > --- a/drivers/dma/mediatek/Kconfig
> > +++ b/drivers/dma/mediatek/Kconfig
> > @@ -1,4 +1,15 @@
> >
> > +config DMA_MTK_UART
>
> MTK_UART_APDMA to align the other drivers
>
> > + 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 8250 mtk uart is enabled, and if you want to using DMA,
>
> 8250 mtk uart should be changed to SERIAL_8250_MT6577 to be more intuitive
>
> > + you can enable the config. the DMA engine just only be used
> > + with MediaTek Socs.
>
> 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_UART_APDMA) += mtk-uart-apdma.o
> to align the other dirvers
>
> > obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> > --
> > 1.7.9.5
> >
^ permalink raw reply
* [RFC,1/6] dma: Add Synopsys eDMA IP core driver
From: Gustavo Pimentel @ 2018-12-13 11:03 UTC (permalink / raw)
To: Bjorn Helgaas, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Eugeniy Paltsev, Andy Shevchenko, Joao Pinto
Hi,
On 12/12/2018 23:00, Bjorn Helgaas wrote:
> On Wed, Dec 12, 2018 at 12:13:21PM +0100, Gustavo Pimentel wrote:
>> Add Synopsys eDMA IP core driver to kernel.
>>
>> This core driver, initializes and configures the eDMA IP using vma-helpers
>> functions and dma-engine subsystem.
>>
>> Also creates an abstration layer through callbacks allowing different
>> registers mappings in the future, organized in to versions.
>>
>> This driver can be compile as built-in or external module in kernel.
>>
>> To enable this driver just select DW_EDMA option in kernel configuration,
>> however it requires and selects automatically DMA_ENGINE and
>> DMA_VIRTUAL_CHANNELS option too.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Cc: Vinod Koul <vkoul@kernel.org>
>> Cc: Eugeniy Paltsev <paltsev@synopsys.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>
>> --- /dev/null
>> +++ b/drivers/dma/dw-edma/dw-edma-core.c
>> @@ -0,0 +1,925 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
>> +// Synopsys DesignWare eDMA core driver
>
> The SPDX line in .c files needs the // comment for some obscure reason
Yes, I think it's correlated to some build breakage on linking stage.
> I can't remember, but based on the other drivers/dma/*.c files, it
> looks like the convention is the usual /* .. */ comments for the rest.
Ok, I'll change it.
>
> I think the SPDX line is /* */ in .h files though. The rules are
> under Documentation/ somewhere.
>
I found the info about this on [1], however I paste here a small extract to
future reference.
C source: // SPDX-License-Identifier: <SPDX License Expression>
C header: /* SPDX-License-Identifier: <SPDX License Expression> */
ASM: /* SPDX-License-Identifier: <SPDX License Expression> */
scripts: # SPDX-License-Identifier: <SPDX License Expression>
.rst: .. SPDX-License-Identifier: <SPDX License Expression>
.dts{i}: // SPDX-License-Identifier: <SPDX License Expression>
[1] https://www.kernel.org/doc/html/v4.18/process/license-rules.html
Thanks Bjorn.
^ permalink raw reply
* [v5,2/2] arm: dts: mt2712: add uart APDMA to device tree
From: Yingjoe Chen @ 2018-12-13 6:18 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Rob Herring, Mark Rutland, Ryder Lee,
Matthias Brugger, Dan Williams, Greg Kroah-Hartman, Jiri Slaby,
Sean Wang, Sean Wang, dmaengine, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, linux-serial, srv_heupstream,
YT Shen
On Tue, 2018-12-11 at 13:37 +0800, Long Cheng wrote:
> 1. add uart APDMA controller device node
> 2. add uart 0/1/2/3/4/5 DMA function
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 50 +++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> index 976d92a..a59728b 100644
> --- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> @@ -300,6 +300,9 @@
> interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW>;
> clocks = <&baud_clk>, <&sys_clk>;
> clock-names = "baud", "bus";
> + dmas = <&apdma 10
> + &apdma 11>;
> + dma-names = "tx", "rx";
> status = "disabled";
> };
>
> @@ -378,6 +381,38 @@
> status = "disabled";
> };
>
> + apdma: dma-controller@11000400 {
> + compatible = "mediatek,mt2712-uart-dma",
> + "mediatek,mt6577-uart-dma";
Sorting, please make sure this is before
auxadc: adc@11001000 {
> + reg = <0 0x11000400 0 0x80>,
> + <0 0x11000480 0 0x80>,
> + <0 0x11000500 0 0x80>,
> + <0 0x11000580 0 0x80>,
> + <0 0x11000600 0 0x80>,
> + <0 0x11000680 0 0x80>,
> + <0 0x11000700 0 0x80>,
> + <0 0x11000780 0 0x80>,
> + <0 0x11000800 0 0x80>,
> + <0 0x11000880 0 0x80>,
> + <0 0x11000900 0 0x80>,
> + <0 0x11000980 0 0x80>;
> + interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 104 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 105 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 106 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 107 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 108 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 109 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 110 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 111 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 112 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 113 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 114 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "apdma";
> + #dma-cells = <1>;
> + };
> +
> uart0: serial@11002000 {
> compatible = "mediatek,mt2712-uart",
> "mediatek,mt6577-uart";
> @@ -385,6 +420,9 @@
...deleted
^ permalink raw reply
* [RFC,6/6] pci: pci_ids: Add Synopsys device id 0xedda
From: Bjorn Helgaas @ 2018-12-12 23:03 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci, dmaengine, Kishon Vijay Abraham I, Lorenzo Pieralisi,
Joao Pinto
On Wed, Dec 12, 2018 at 12:13:26PM +0100, Gustavo Pimentel wrote:
> Create and add Synopsys device id (0xedda) to pci id list, since this id
> is now being use on two different drivers (pci_endpoint_test.ko and
> dw-edma-pcie.ko).
Please run "git log --oneline include/linux/pci_ids.h" and make your
subject line match, i.e.,
PCI: Add Synopsys endpoint EDDA Device ID
Also, if you want, you can reorder this to add the ID first and use it
in drivers/misc/pci_endpoint_test.c, and then just use it from the
first appearance of drivers/dma/dw-edma/dw-edma-pcie.c. Either way is
fine.
For the include/linux/pci_ids.h change,
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> ---
> drivers/dma/dw-edma/dw-edma-pcie.c | 2 +-
> drivers/misc/pci_endpoint_test.c | 2 +-
> include/linux/pci_ids.h | 1 +
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index f29a861..50e0db4 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> @@ -280,7 +280,7 @@ static const struct dev_pm_ops dw_edma_pcie_dev_pm_ops = {
> };
>
> static const struct pci_device_id dw_edma_pcie_id_table[] = {
> - { PCI_DEVICE_DATA(SYNOPSYS, 0xedda, &snps_edda_data) },
> + { PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
> { }
> };
> MODULE_DEVICE_TABLE(pci, dw_edma_pcie_id_table);
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 896e2df..d27efe838 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -788,7 +788,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
> static const struct pci_device_id pci_endpoint_test_tbl[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
> { PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
> - { PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, 0xedda) },
> + { PCI_DEVICE_DATA(SYNOPSYS, EDDA, NULL) },
> { }
> };
> MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 69f0abe..57f17dd 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2358,6 +2358,7 @@
> #define PCI_DEVICE_ID_CENATEK_IDE 0x0001
>
> #define PCI_VENDOR_ID_SYNOPSYS 0x16c3
> +#define PCI_DEVICE_ID_SYNOPSYS_EDDA 0xedda
>
> #define PCI_VENDOR_ID_VITESSE 0x1725
> #define PCI_DEVICE_ID_VITESSE_VSC7174 0x7174
> --
> 2.7.4
>
^ permalink raw reply
* [RFC,1/6] dma: Add Synopsys eDMA IP core driver
From: Bjorn Helgaas @ 2018-12-12 23:00 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci, dmaengine, Vinod Koul, Eugeniy Paltsev,
Andy Shevchenko, Joao Pinto
On Wed, Dec 12, 2018 at 12:13:21PM +0100, Gustavo Pimentel wrote:
> Add Synopsys eDMA IP core driver to kernel.
>
> This core driver, initializes and configures the eDMA IP using vma-helpers
> functions and dma-engine subsystem.
>
> Also creates an abstration layer through callbacks allowing different
> registers mappings in the future, organized in to versions.
>
> This driver can be compile as built-in or external module in kernel.
>
> To enable this driver just select DW_EDMA option in kernel configuration,
> however it requires and selects automatically DMA_ENGINE and
> DMA_VIRTUAL_CHANNELS option too.
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Eugeniy Paltsev <paltsev@synopsys.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> --- /dev/null
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -0,0 +1,925 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> +// Synopsys DesignWare eDMA core driver
The SPDX line in .c files needs the // comment for some obscure reason
I can't remember, but based on the other drivers/dma/*.c files, it
looks like the convention is the usual /* .. */ comments for the rest.
I think the SPDX line is /* */ in .h files though. The rules are
under Documentation/ somewhere.
^ permalink raw reply
* [RFC,2/6] dma: Add Synopsys eDMA IP version 0 support
From: Eugeniy Paltsev @ 2018-12-12 13:39 UTC (permalink / raw)
To: dmaengine@vger.kernel.org, linux-pci@vger.kernel.org,
gustavo.pimentel@synopsys.com
Cc: vkoul@kernel.org, andriy.shevchenko@linux.intel.com, Joao Pinto,
Alexey Brodkin
Hi Gustavo,
On Wed, 2018-12-12 at 12:13 +0100, Gustavo Pimentel wrote:
> Add support for the eDMA IP version 0 driver for both register maps (legacy
> and unroll).
[snip]
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> new file mode 100644
> index 0000000..cc362b0
> --- /dev/null
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> +// Synopsys DesignWare eDMA v0 core
> +
> +#include "dw-edma-core.h"
> +#include "dw-edma-v0-core.h"
> +#include "dw-edma-v0-regs.h"
> +#include "dw-edma-v0-debugfs.h"
> +
> +#define QWORD_HI(value) ((value & 0xFFFFFFFF00000000llu) >> 32)
Use upper_32_bits() macros.
> +#define QWORD_LO(value) (value & 0x00000000FFFFFFFFllu)
Use lower_32_bits() macros.
> +enum dw_edma_control {
> + DW_EDMA_CB = BIT(0),
> + DW_EDMA_TCB = BIT(1),
> + DW_EDMA_LLP = BIT(2),
> + DW_EDMA_LIE = BIT(3),
> + DW_EDMA_RIE = BIT(4),
[snip]
> + viewport_sel = (ch & 0x00000007ul);
[snip]
> +void dw_edma_v0_core_off(struct dw_edma *dw)
> +{
> + SET_BOTH(dw, int_mask, 0x00FF00FFul);
> + SET_BOTH(dw, int_clear, 0x00FF00FFul);
> + SET_BOTH(dw, engine_en, 0x00000000ul);
> +}
[snip]
> + num_ch &= 0x0000000Ful;
> + } else {
> + num_ch &= 0x000F0000ul;
I guess it's better to name this magic numbers via defines.
[snip]
> +
> +bool dw_edma_v0_core_status_done_int(struct dw_edma_chan *chan)
> +{
> + struct dw_edma *dw = chan->chip->dw;
> + u32 tmp;
> +
> + tmp = GET_RW(dw, chan->dir, int_status);
> + tmp &= BIT(chan->id);
> +
> + return tmp ? true : false;
return !!tmp;
> +}
> +
> +bool dw_edma_v0_core_status_abort_int(struct dw_edma_chan *chan)
> +{
> + struct dw_edma *dw = chan->chip->dw;
> + u32 tmp;
> +
> + tmp = GET_RW(dw, chan->dir, int_status);
> + tmp &= BIT(chan->id + 16);
> +
> + return tmp ? true : false;
ditto.
--
Eugeniy Paltsev
^ 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