* [v3,3/7] dmaengine: dw: Remove unused internal property
From: Andy Shevchenko @ 2019-01-07 11:07 UTC (permalink / raw)
To: Viresh Kumar, dmaengine, Vinod Koul; +Cc: Andy Shevchenko
All known devices, which use DT for configuration, support
memory-to-memory transfers. So enable it by default.
The rest two cases, i.e. Intel Quark and PPC460ex, instantiate DMA driver and
use its channels exclusively for hardware, which means there is no available
channel for any other purposes anyway.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/dma/dw/core.c | 4 +---
drivers/dma/dw/pci.c | 1 -
drivers/dma/dw/platform.c | 6 ------
include/linux/platform_data/dma-dw.h | 2 --
4 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index e25503986680..4982e443869c 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1227,7 +1227,6 @@ int dw_dma_probe(struct dw_dma_chip *chip)
pdata->block_size = dma_readl(dw, MAX_BLK_SIZE);
/* Fill platform data with the default values */
- pdata->is_memcpy = true;
pdata->chan_allocation_order = CHAN_ALLOCATION_ASCENDING;
pdata->chan_priority = CHAN_PRIORITY_ASCENDING;
} else if (chip->pdata->nr_channels > DW_DMA_MAX_NR_CHANNELS) {
@@ -1340,8 +1339,7 @@ int dw_dma_probe(struct dw_dma_chip *chip)
/* Set capabilities */
dma_cap_set(DMA_SLAVE, dw->dma.cap_mask);
dma_cap_set(DMA_PRIVATE, dw->dma.cap_mask);
- if (pdata->is_memcpy)
- dma_cap_set(DMA_MEMCPY, dw->dma.cap_mask);
+ dma_cap_set(DMA_MEMCPY, dw->dma.cap_mask);
dw->dma.dev = chip->dev;
dw->dma.device_alloc_chan_resources = dwc_alloc_chan_resources;
diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c
index 570498faadc3..66d98d7ccad0 100644
--- a/drivers/dma/dw/pci.c
+++ b/drivers/dma/dw/pci.c
@@ -17,7 +17,6 @@
static struct dw_dma_platform_data mrfld_pdata = {
.nr_channels = 8,
- .is_memcpy = true,
.is_idma32 = true,
.chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
.chan_priority = CHAN_PRIORITY_ASCENDING,
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 6dd8cd1820c1..58fc1ba02a1e 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -128,12 +128,6 @@ dw_dma_parse_dt(struct platform_device *pdev)
pdata->nr_masters = nr_masters;
pdata->nr_channels = nr_channels;
- /*
- * All known devices, which use DT for configuration, support
- * memory-to-memory transfers. So enable it by default.
- */
- pdata->is_memcpy = true;
-
if (!of_property_read_u32(np, "chan_allocation_order", &tmp))
pdata->chan_allocation_order = (unsigned char)tmp;
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index d443025c5c72..1c85eeee4171 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -38,7 +38,6 @@ struct dw_dma_slave {
/**
* struct dw_dma_platform_data - Controller configuration parameters
* @nr_channels: Number of channels supported by hardware (max 8)
- * @is_memcpy: The device channels do support memory-to-memory transfers.
* @is_idma32: The type of the DMA controller is iDMA32
* @chan_allocation_order: Allocate channels starting from 0 or 7
* @chan_priority: Set channel priority increasing from 0 to 7 or 7 to 0.
@@ -51,7 +50,6 @@ struct dw_dma_slave {
*/
struct dw_dma_platform_data {
unsigned int nr_channels;
- bool is_memcpy;
bool is_idma32;
#define CHAN_ALLOCATION_ASCENDING 0 /* zero to seven */
#define CHAN_ALLOCATION_DESCENDING 1 /* seven to zero */
^ permalink raw reply related
* [v3,2/7] dmaengine: dw: Remove misleading is_private property
From: Andy Shevchenko @ 2019-01-07 11:07 UTC (permalink / raw)
To: Viresh Kumar, dmaengine, Vinod Koul
Cc: Andy Shevchenko, Greg Kroah-Hartman, Dan Williams
The commit a9ddb575d6d6
("dmaengine: dw_dmac: Enhance device tree support")
introduces is_private property in uncertain understanding what does it mean.
First of all, documentation defines DMA_PRIVATE capability as
Documentation/crypto/async-tx-api.txt:
The DMA_PRIVATE capability flag is used to tag dma devices that should not be
used by the general-purpose allocator. It can be set at initialization time
if it is known that a channel will always be private. Alternatively,
it is set when dma_request_channel() finds an unused "public" channel.
A couple caveats to note when implementing a driver and consumer:
1/ Once a channel has been privately allocated it will no longer be
considered by the general-purpose allocator even after a call to
dma_release_channel().
2/ Since capabilities are specified at the device level a dma_device with
multiple channels will either have all channels public, or all channels
private.
Documentation/driver-api/dmaengine/provider.rst:
- DMA_PRIVATE
The devices only supports slave transfers, and as such isn't available
for async transfers.
The capability had been introduced by the commit 59b5ec21446b
("dmaengine: introduce dma_request_channel and private channels")
and some code didn't changed from that times ever.
Taking into consideration above and the fact that on all known platforms
Synopsys DesignWare DMA engine is attached to serve slave transfers,
the DMA_PRIVATE capability must be enabled for this device unconditionally.
Otherwise, as rightfully noticed in drivers/dma/at_xdmac.c:
/*
* Without DMA_PRIVATE the driver is not able to allocate more than
* one channel, second allocation fails in private_candidate.
*/
because of of a caveats mentioned in above documentation excerpts.
So, remove conditional around DMA_PRIVATE followed by removal leftovers.
If someone wonders, DMA_PRIVATE can be not used if and only if the all channels
of the DMA controller are supposed to serve memory-to-memory like operations.
For example, EP93xx has two controllers, one of which can only perform
memory-to-memory transfers
Note, this change doesn't affect dmatest to be able to test such controllers.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> (maintainer:SERIAL DRIVERS)
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/devicetree/bindings/dma/snps-dma.txt | 2 --
drivers/dma/dw/core.c | 4 +---
drivers/dma/dw/pci.c | 1 -
drivers/dma/dw/platform.c | 3 ---
drivers/tty/serial/8250/8250_lpss.c | 1 -
include/linux/platform_data/dma-dw.h | 3 ---
6 files changed, 1 insertion(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
index db757df7057d..0bedceed1963 100644
--- a/Documentation/devicetree/bindings/dma/snps-dma.txt
+++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
@@ -23,8 +23,6 @@ Deprecated properties:
Optional properties:
-- is_private: The device channels should be marked as private and not for by the
- general purpose DMA channel allocator. False if not passed.
- multi-block: Multi block transfers supported by hardware. Array property with
one cell per channel. 0: not supported, 1 (default): supported.
- snps,dma-protection-control: AHB HPROT[3:1] protection setting.
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index dc053e62f894..e25503986680 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1227,7 +1227,6 @@ int dw_dma_probe(struct dw_dma_chip *chip)
pdata->block_size = dma_readl(dw, MAX_BLK_SIZE);
/* Fill platform data with the default values */
- pdata->is_private = true;
pdata->is_memcpy = true;
pdata->chan_allocation_order = CHAN_ALLOCATION_ASCENDING;
pdata->chan_priority = CHAN_PRIORITY_ASCENDING;
@@ -1340,8 +1339,7 @@ int dw_dma_probe(struct dw_dma_chip *chip)
/* Set capabilities */
dma_cap_set(DMA_SLAVE, dw->dma.cap_mask);
- if (pdata->is_private)
- dma_cap_set(DMA_PRIVATE, dw->dma.cap_mask);
+ dma_cap_set(DMA_PRIVATE, dw->dma.cap_mask);
if (pdata->is_memcpy)
dma_cap_set(DMA_MEMCPY, dw->dma.cap_mask);
diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c
index 313ba10c6224..570498faadc3 100644
--- a/drivers/dma/dw/pci.c
+++ b/drivers/dma/dw/pci.c
@@ -17,7 +17,6 @@
static struct dw_dma_platform_data mrfld_pdata = {
.nr_channels = 8,
- .is_private = true,
.is_memcpy = true,
.is_idma32 = true,
.chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 31ff8113c3de..6dd8cd1820c1 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -128,9 +128,6 @@ dw_dma_parse_dt(struct platform_device *pdev)
pdata->nr_masters = nr_masters;
pdata->nr_channels = nr_channels;
- if (of_property_read_bool(np, "is_private"))
- pdata->is_private = true;
-
/*
* All known devices, which use DT for configuration, support
* memory-to-memory transfers. So enable it by default.
diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 2a71616890a4..1a6ee21cfcec 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -153,7 +153,6 @@ static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
#ifdef CONFIG_SERIAL_8250_DMA
static const struct dw_dma_platform_data qrk_serial_dma_pdata = {
.nr_channels = 2,
- .is_private = true,
.chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
.chan_priority = CHAN_PRIORITY_ASCENDING,
.block_size = 4095,
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 1a1d58ebffbf..d443025c5c72 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -38,8 +38,6 @@ struct dw_dma_slave {
/**
* struct dw_dma_platform_data - Controller configuration parameters
* @nr_channels: Number of channels supported by hardware (max 8)
- * @is_private: The device channels should be marked as private and not for
- * by the general purpose DMA channel allocator.
* @is_memcpy: The device channels do support memory-to-memory transfers.
* @is_idma32: The type of the DMA controller is iDMA32
* @chan_allocation_order: Allocate channels starting from 0 or 7
@@ -53,7 +51,6 @@ struct dw_dma_slave {
*/
struct dw_dma_platform_data {
unsigned int nr_channels;
- bool is_private;
bool is_memcpy;
bool is_idma32;
#define CHAN_ALLOCATION_ASCENDING 0 /* zero to seven */
^ permalink raw reply related
* [v3,1/7] dmaengine: dw: Add missed multi-block support for iDMA 32-bit
From: Andy Shevchenko @ 2019-01-07 11:07 UTC (permalink / raw)
To: Viresh Kumar, dmaengine, Vinod Koul; +Cc: Andy Shevchenko
Intel integrated DMA 32-bit support multi-block transfers.
Add missed setting to the platform data.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/dma/dw/pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c
index 7778ed705a1a..313ba10c6224 100644
--- a/drivers/dma/dw/pci.c
+++ b/drivers/dma/dw/pci.c
@@ -25,6 +25,7 @@ static struct dw_dma_platform_data mrfld_pdata = {
.block_size = 131071,
.nr_masters = 1,
.data_width = {4},
+ .multi_block = {1, 1, 1, 1, 1, 1, 1, 1},
};
static int dw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pid)
^ permalink raw reply related
* dmaengine: st_fdma: use struct_size() in kzalloc()
From: patrice.chotard @ 2019-01-07 8:50 UTC (permalink / raw)
To: Gustavo A. R. Silva, Vinod Koul, Dan Williams
Cc: linux-arm-kernel@lists.infradead.org, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Gustavo
On 1/4/19 7:43 PM, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding the
> size of a structure that has a zero-sized array at the end, along with memory
> for some number of elements for that array. For example:
>
> struct foo {
> int stuff;
> void *entry[];
> };
>
> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can now
> use the new struct_size() helper:
>
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>
> This code was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/dma/st_fdma.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
> index 07c20aa2e955..df6b73028ecb 100644
> --- a/drivers/dma/st_fdma.c
> +++ b/drivers/dma/st_fdma.c
> @@ -243,8 +243,7 @@ static struct st_fdma_desc *st_fdma_alloc_desc(struct st_fdma_chan *fchan,
> struct st_fdma_desc *fdesc;
> int i;
>
> - fdesc = kzalloc(sizeof(*fdesc) +
> - sizeof(struct st_fdma_sw_node) * sg_len, GFP_NOWAIT);
> + fdesc = kzalloc(struct_size(fdesc, node, sg_len), GFP_NOWAIT);
> if (!fdesc)
> return NULL;
>
>
Acked-by: Patrice Chotard <patrice.chotard@st.com>
Thanks
^ permalink raw reply
* [1/5] dmaengine: bcm2835: Fix interrupt race on RT
From: Lukas Wunner @ 2019-01-07 8:32 UTC (permalink / raw)
To: Vinod Koul
Cc: Eric Anholt, Stefan Wahren, Frank Pavlic, Martin Sperl,
Florian Meier, dmaengine, linux-rpi-kernel, Tiejun Chen,
linux-rt-users
On Mon, Jan 07, 2019 at 12:30:03PM +0530, Vinod Koul wrote:
> On 22-12-18, 08:28, Lukas Wunner wrote:
> > before the IRQ handler had a chance to run. In fact the IRQ handler may
> ^^^
> > miss an *arbitrary* number of descriptors. The result is the following
> ^^^^^
> this has double spaces in few places pls fix
That was intentional. Bjorn Helgaas has established double spaces to
separate sentences in the PCI subsystem, both in commit messages and
code comments. The rationale he has given:
"Only habit because my eighth-grade typing teacher in 1979 did it that
way, and (I think) vim does it that way by default."
https://patchwork.kernel.org/patch/8941601/
Rafael Wysocki, myself and others have adopted this style. However
I'll gladly omit the double spaces if that's the preferred style in
the DMA subsystem.
> > @@ -456,7 +456,8 @@ static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
> > c->desc = d = to_bcm2835_dma_desc(&vd->tx);
> >
> > writel(d->cb_list[0].paddr, c->chan_base + BCM2835_DMA_ADDR);
> > - writel(BCM2835_DMA_ACTIVE, c->chan_base + BCM2835_DMA_CS);
> > + writel(BCM2835_DMA_ACTIVE | BCM2835_DMA_END,
> > + c->chan_base + BCM2835_DMA_CS);
>
> can you explain this change please? so every descriptor is written with
> BCM2835_DMA_END?
The BCM2835_DMA_END flag is of type W1C (write 1 to clear). The above
change ensures that the END flag is cleared when a new descriptor is
started. Once the DMA engine has finished that descriptor, it will
automatically set the flag again.
This allows the IRQ handler to recognize that it has missed descriptors
and that a new descriptor is currently processed by the DMA engine.
The IRQ handler will then refrain from finalizing that in-flight
descriptor.
I did explain this change in the commit message:
"Therefore, only finalize a descriptor if the END flag is set in the CS
register. Clear the flag when starting a new descriptor. Perform both
operations under the vchan lock."
> > @@ -477,11 +479,17 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
> > spin_lock_irqsave(&c->vc.lock, flags);
> >
> > /* Acknowledge interrupt */
> > - writel(BCM2835_DMA_INT, c->chan_base + BCM2835_DMA_CS);
> > + cs = readl(c->chan_base + BCM2835_DMA_CS);
> > + writel(cs, c->chan_base + BCM2835_DMA_CS);
>
> So idea is to ack the interrupts asserted while this is running, right?
The BCM2835_DMA_INT flag is likewise of type W1C. By writing the
same value to the CS register that it currently has, the interrupt
is acknowledged (INT flag is cleared) but the ACTIVE flag is preserved.
That way, if a new descriptor is currently processed by the DMA engine,
that descriptor is left running.
(The BCM2835_DMA_END is also cleared, but that's not important.)
The above change is likewise explained in the commit message:
"Only minimal additional overhead is introduced as one
further MMIO read is necessary per interrupt.
That MMIO read is needed to write the same value to the CS register that
it currently has, thereby preserving the ACTIVE flag while clearing the
INT and END flags."
Thanks,
Lukas
^ permalink raw reply
* [5/5] dmaengine: bcm2835: Remove dead code
From: Vinod Koul @ 2019-01-07 8:28 UTC (permalink / raw)
To: Lukas Wunner
Cc: Eric Anholt, Stefan Wahren, Frank Pavlic, Martin Sperl,
Florian Meier, dmaengine, linux-rpi-kernel
On 22-12-18, 08:28, Lukas Wunner wrote:
> The BCM2835 DMA driver deletes a channel from a list upon termination
> without having added it to a list first. Moreover that operation is
> protected by a spinlock which isn't taken anywhere else. These appear
> to be remnants of an older version of the driver which accidentally
> got mainlined. Remove the dead code.
>
> While at it remove an outdated comment claiming the driver only supports
Ditto, whenever you use also, while at it... think if this is a
candidate for splitting up.
A patch should do one thing only..
> cyclic transactions. The driver has been supporting other transaction
> types for more than two years.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Frank Pavlic <f.pavlic@kunbus.de>
> Cc: Martin Sperl <kernel@martin.sperl.org>
> Cc: Florian Meier <florian.meier@koalo.de>
> ---
> drivers/dma/bcm2835-dma.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index e424e232a3d0..633be2ee7f33 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -2,9 +2,6 @@
> /*
> * BCM2835 DMA engine support
> *
> - * This driver only supports cyclic DMA transfers
> - * as needed for the I2S module.
> - *
> * Author: Florian Meier <florian.meier@koalo.de>
> * Copyright 2013
> *
> @@ -42,7 +39,6 @@
>
> struct bcm2835_dmadev {
> struct dma_device ddev;
> - spinlock_t lock;
> void __iomem *base;
> struct device_dma_parameters dma_parms;
> };
> @@ -64,7 +60,6 @@ struct bcm2835_cb_entry {
>
> struct bcm2835_chan {
> struct virt_dma_chan vc;
> - struct list_head node;
>
> struct dma_slave_config cfg;
> unsigned int dreq;
> @@ -772,17 +767,11 @@ static int bcm2835_dma_slave_config(struct dma_chan *chan,
> static int bcm2835_dma_terminate_all(struct dma_chan *chan)
> {
> struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> - struct bcm2835_dmadev *d = to_bcm2835_dma_dev(c->vc.chan.device);
> unsigned long flags;
> LIST_HEAD(head);
>
> spin_lock_irqsave(&c->vc.lock, flags);
>
> - /* Prevent this channel being scheduled */
> - spin_lock(&d->lock);
> - list_del_init(&c->node);
> - spin_unlock(&d->lock);
> -
> /* stop DMA activity */
> if (c->desc) {
> vchan_terminate_vdesc(&c->desc->vd);
> @@ -815,7 +804,6 @@ static int bcm2835_dma_chan_init(struct bcm2835_dmadev *d, int chan_id,
>
> c->vc.desc_free = bcm2835_dma_desc_free;
> vchan_init(&c->vc, &d->ddev);
> - INIT_LIST_HEAD(&c->node);
>
> c->chan_base = BCM2835_DMA_CHANIO(d->base, chan_id);
> c->ch = chan_id;
> @@ -918,7 +906,6 @@ static int bcm2835_dma_probe(struct platform_device *pdev)
> od->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
> od->ddev.dev = &pdev->dev;
> INIT_LIST_HEAD(&od->ddev.channels);
> - spin_lock_init(&od->lock);
>
> platform_set_drvdata(pdev, od);
>
> --
> 2.19.2
^ permalink raw reply
* [3/5] dmaengine: bcm2835: Clean up abort of transactions
From: Vinod Koul @ 2019-01-07 8:26 UTC (permalink / raw)
To: Lukas Wunner
Cc: Eric Anholt, Stefan Wahren, Frank Pavlic, Martin Sperl,
Florian Meier, dmaengine, linux-rpi-kernel
On 22-12-18, 08:28, Lukas Wunner wrote:
> bcm2835_dma_abort() returns an int but bcm2835_dma_terminate_all() (its
> sole caller) does not evaluate the return value. Change the return type
> to void.
>
> Also, the "cs" variable is dispensable, so remove it. Its type seems
Please make it two different commits...
> wrong anyway, "unsigned long" is 64-bit on arm64, yet the CS register is
> 32-bit.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Frank Pavlic <f.pavlic@kunbus.de>
> Cc: Martin Sperl <kernel@martin.sperl.org>
> Cc: Florian Meier <florian.meier@koalo.de>
> ---
> drivers/dma/bcm2835-dma.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index 17bc7304db3a..a3ecb7fd4fc2 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -406,14 +406,12 @@ static void bcm2835_dma_fill_cb_chain_with_sg(
> }
> }
>
> -static int bcm2835_dma_abort(void __iomem *chan_base)
> +static void bcm2835_dma_abort(void __iomem *chan_base)
> {
> - unsigned long cs;
> long int timeout = 10000;
>
> - cs = readl(chan_base + BCM2835_DMA_CS);
> - if (!(cs & BCM2835_DMA_ACTIVE))
> - return 0;
> + if (!(readl(chan_base + BCM2835_DMA_CS) & BCM2835_DMA_ACTIVE))
> + return;
>
> /* Write 0 to the active bit - Pause the DMA */
> writel(0, chan_base + BCM2835_DMA_CS);
> @@ -424,7 +422,6 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
> cpu_relax();
>
> writel(BCM2835_DMA_RESET, chan_base + BCM2835_DMA_CS);
> - return 0;
> }
>
> static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
> --
> 2.19.2
^ permalink raw reply
* [1/5] dmaengine: bcm2835: Fix interrupt race on RT
From: Vinod Koul @ 2019-01-07 7:00 UTC (permalink / raw)
To: Lukas Wunner
Cc: Eric Anholt, Stefan Wahren, Frank Pavlic, Martin Sperl,
Florian Meier, dmaengine, linux-rpi-kernel, Tiejun Chen,
linux-rt-users
Hi Lukas,
On 22-12-18, 08:28, Lukas Wunner wrote:
> If IRQ handlers are threaded (either because CONFIG_PREEMPT_RT_BASE is
> enabled or "threadirqs" was passed on the command line) and if system
> load is sufficiently high that wakeup latency of IRQ threads degrades,
> SPI DMA transactions on the BCM2835 occasionally break like this:
>
> ks8851 spi0.0: SPI transfer timed out
> bcm2835-dma 3f007000.dma: DMA transfer could not be terminated
> ks8851 spi0.0 eth2: ks8851_rdfifo: spi_sync() failed
>
> The root cause is an assumption made by the DMA driver which is
> documented in a code comment in bcm2835_dma_terminate_all():
>
> /*
> * Stop DMA activity: we assume the callback will not be called
> * after bcm_dma_abort() returns (even if it does, it will see
> * c->desc is NULL and exit.)
> */
>
> That assumption falls apart if the IRQ handler bcm2835_dma_callback() is
> threaded: A client may terminate a descriptor and issue a new one
> before the IRQ handler had a chance to run. In fact the IRQ handler may
^^^
> miss an *arbitrary* number of descriptors. The result is the following
^^^^^
this has double spaces in few places pls fix
> race condition:
>
> 1. A descriptor finishes, its interrupt is deferred to the IRQ thread.
> 2. A client calls dma_terminate_async() which sets channel->desc = NULL.
> 3. The client issues a new descriptor. Because channel->desc is NULL,
> bcm2835_dma_issue_pending() immediately starts the descriptor.
> 4. Finally the IRQ thread runs and writes BCM2835_DMA_INT to the CS
> register to acknowledge the interrupt. This clears the ACTIVE flag,
> so the newly issued descriptor is paused in the middle of the
> transaction. Because channel->desc is not NULL, the IRQ thread
> finalizes the descriptor and tries to start the next one.
>
> I see two possible solutions: The first is to call synchronize_irq()
> in bcm2835_dma_issue_pending() to wait until the IRQ thread has
> finished before issuing a new descriptor. The downside of this approach
> is unnecessary latency if clients desire rapidly terminating and
> re-issuing descriptors and don't have any use for an IRQ callback.
> (The SPI TX DMA channel is a case in point.)
>
> A better alternative is to make the IRQ thread recognize that it has
> missed descriptors and avoid finalizing the newly issued descriptor.
> Therefore, only finalize a descriptor if the END flag is set in the CS
> register. Clear the flag when starting a new descriptor. Perform both
> operations under the vchan lock. That way, there is practically no
> impact on latency and throughput if the client doesn't care for the
> interrupt: Only minimal additional overhead is introduced as one
> further MMIO read is necessary per interrupt.
>
> That MMIO read is needed to write the same value to the CS register that
> it currently has, thereby preserving the ACTIVE flag while clearing the
> INT and END flags. Note that the transaction may finish between reading
> and writing the CS register, and in that case the write changes the
> ACTIVE flag from 0 to 1. But that's harmless, the chip will notice that
> NEXTCONBK is 0 and self-clear the ACTIVE flag again.
>
> Fixes: 96286b576690 ("dmaengine: Add support for BCM2835")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v3.14+
> Cc: Frank Pavlic <f.pavlic@kunbus.de>
> Cc: Martin Sperl <kernel@martin.sperl.org>
> Cc: Florian Meier <florian.meier@koalo.de>
> ---
> drivers/dma/bcm2835-dma.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index 1a44c8086d77..e94f41c56975 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -456,7 +456,8 @@ static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
> c->desc = d = to_bcm2835_dma_desc(&vd->tx);
>
> writel(d->cb_list[0].paddr, c->chan_base + BCM2835_DMA_ADDR);
> - writel(BCM2835_DMA_ACTIVE, c->chan_base + BCM2835_DMA_CS);
> + writel(BCM2835_DMA_ACTIVE | BCM2835_DMA_END,
> + c->chan_base + BCM2835_DMA_CS);
can you explain this change please? so every descriptor is written with
BCM2835_DMA_END?
> }
>
> static irqreturn_t bcm2835_dma_callback(int irq, void *data)
> @@ -464,6 +465,7 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
> struct bcm2835_chan *c = data;
> struct bcm2835_desc *d;
> unsigned long flags;
> + u32 cs;
>
> /* check the shared interrupt */
> if (c->irq_flags & IRQF_SHARED) {
> @@ -477,11 +479,17 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
> spin_lock_irqsave(&c->vc.lock, flags);
>
> /* Acknowledge interrupt */
> - writel(BCM2835_DMA_INT, c->chan_base + BCM2835_DMA_CS);
> + cs = readl(c->chan_base + BCM2835_DMA_CS);
> + writel(cs, c->chan_base + BCM2835_DMA_CS);
So idea is to ack the interrupts asserted while this is running, right?
>
> d = c->desc;
>
> - if (d) {
> + /*
> + * If this IRQ handler is threaded, clients may terminate descriptors
> + * and issue new ones before the IRQ handler runs. Avoid finalizing
^^^^
this as well..
> + * such a newly issued descriptor by checking the END flag.
> + */
> + if (d && cs & BCM2835_DMA_END) {
> if (d->cyclic) {
> /* call the cyclic callback */
> vchan_cyclic_callback(&d->vd);
> @@ -789,11 +797,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
> list_del_init(&c->node);
> spin_unlock(&d->lock);
>
> - /*
> - * Stop DMA activity: we assume the callback will not be called
> - * after bcm_dma_abort() returns (even if it does, it will see
> - * c->desc is NULL and exit.)
> - */
> + /* stop DMA activity */
> if (c->desc) {
> vchan_terminate_vdesc(&c->desc->vd);
> c->desc = NULL;
> --
> 2.19.2
^ permalink raw reply
* [2/8,v2] Documentation: bindings: k3dma: Add binding for dma-avail-chan
From: Vinod Koul @ 2019-01-07 6:24 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: John Stultz, lkml, Rob Herring, Mark Rutland, Tanglei Han,
Zhuangluan Su, Ryan Grachek, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On 05-01-19, 19:38, Manivannan Sadhasivam wrote:
> Hi Vinod,
>
> On Sat, Jan 05, 2019 at 07:16:10PM +0530, Vinod Koul wrote:
> > On 05-01-19, 10:23, Manivannan Sadhasivam wrote:
> > > On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> > > > On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> > > > <manivannan.sadhasivam@linaro.org> wrote:
> > > > >
> > > > > Hi John,
> > > > >
> > > > > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > > > > Some dma channels can be reserved for secure mode or other
> > > > > > hardware on the SoC, so provide a binding for a bitmask
> > > > > > listing the available channels for the kernel to use.
> > > > > >
> > > > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > > > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > > > > Cc: Ryan Grachek <ryan@edited.us>
> > > > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > Cc: dmaengine@vger.kernel.org
> > > > > > Cc: devicetree@vger.kernel.org
> > > > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > > > ---
> > > > > > Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > > > > 1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > > index 10a2f15..1c466c1 100644
> > > > > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > > @@ -14,6 +14,9 @@ Required properties:
> > > > > > have specific request line
> > > > > > - clocks: clock required
> > > > > >
> > > > > > +Optional properties:
> > > > > > +- dma-avail-chan: Bitmask of available physical channels
> > > > > > +
> > > > >
> > > > > This property looks too generic. Since this is specific to HiSi SoCs,
> > > > > this could be "hisi-dma-avail-chan"?
> > > >
> > > > I'm fine to change it, but I'm not sure I fully understand the
> > > > rational. Can you help me understand?
> > > > Are device node-binding names supposed to have global scope? I assumed
> > > > the node property names are basically scoped to the entry?
> > >
> > > IIUC properties documented in subsystem binding (dma.txt in this case)
> > > will have global scope. Those which are not documented in this binding
> > > are specific to vendor IPs and should be prefixed with the vendor
> > > prefix (hisi in this case).
> > >
> > > > Further, having some dma channels be reserved doesn't seem to be too
> > > > unique a concept, so I'm not sure what we gain long term by prefixing
> > > > it?
> > > >
> > >
> > > Right, but this brings up the point of having this functionality in
> > > generic DMA engine so that the DMA controller drivers need not handle.
> > > So either we should move this available channel check to DMA Engine
> > > and document the property in dma.txt so that other IPs can also use it
> > > or keep the functionality in K3 driver and use HiSi prefix for the
> > > property.
> > >
> > > But I'd like to hear Vinod/Rob's opinion on this!
> >
> > So there are two parts, first is if this new property of using 'some'
> > channels of controller is generic enough, the answer is unfortunately
> > yes, so we should move this to dma.txt as a generic property
> >
> > But I don't agree the dmaengine core should handle it, we may add
> > helpers, but controllers registers N channels and they would do so, core
> > should not do filtering
> >
>
> Okay. But won't it create ambiguity? What if a new driver developer
> assmes that he can use this property to filter the channels for his own
> DMA controller? Since we are _explicitly_ stating that these channels
> should be filtered, why the dmaengine core can't handle it?
>
> If the property is generic, then it makes sense to keep the
> functionality also generic.
Core doesnt have a view of channels to be filtered, it looks at N
channels getting registered and works on those.
Till now folks do not create channels for 'filtered' ones and register
the ones kernel can use..
^ permalink raw reply
* [v2,2/2] dmaengine: qcom_hidma: assign channel cookie correctly
From: Yang Shunyong @ 2019-01-07 1:34 UTC (permalink / raw)
To: vkoul
Cc: okaya, andy.gross, david.brown, dan.j.williams, dmaengine,
linux-kernel, Shunyong Yang, Joey Zheng
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>
Reviewed-by: Sinan Kaya <okaya@kernel.org>
Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com>
---
v2:
fix missing brace according to Vinod's feedback.
add Reviewed-by: Sinan Kaya <okaya@kernel.org>.
---
drivers/dma/qcom/hidma.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index 9d639ed1955a..411f91fde734 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -138,24 +138,25 @@ static void hidma_process_completed(struct hidma_chan *mchan)
desc = &mdesc->desc;
last_cookie = desc->cookie;
+ llstat = hidma_ll_status(mdma->lldev, mdesc->tre_ch);
+
spin_lock_irqsave(&mchan->lock, irqflags);
+ if (llstat == DMA_COMPLETE) {
+ mchan->last_success = last_cookie;
+ result.result = DMA_TRANS_NOERROR;
+ } else {
+ result.result = DMA_TRANS_ABORTED;
+ }
+
dma_cookie_complete(desc);
spin_unlock_irqrestore(&mchan->lock, irqflags);
- llstat = hidma_ll_status(mdma->lldev, mdesc->tre_ch);
dmaengine_desc_get_callback(desc, &cb);
dma_run_dependencies(desc);
spin_lock_irqsave(&mchan->lock, irqflags);
list_move(&mdesc->node, &mchan->free);
-
- if (llstat == DMA_COMPLETE) {
- mchan->last_success = last_cookie;
- result.result = DMA_TRANS_NOERROR;
- } else
- result.result = DMA_TRANS_ABORTED;
-
spin_unlock_irqrestore(&mchan->lock, irqflags);
dmaengine_desc_callback_invoke(&cb, &result);
^ permalink raw reply related
* [v2,1/2] dmaengine: qcom_hidma: initialize tx flags in hidma_prep_dma_*
From: Yang Shunyong @ 2019-01-07 1:32 UTC (permalink / raw)
To: vkoul
Cc: okaya, andy.gross, david.brown, dan.j.williams, dmaengine,
linux-kernel, Shunyong Yang, Joey Zheng
In async_tx_test_ack(), it uses flags in struct dma_async_tx_descriptor
to check the ACK status. As hidma reuses the descriptor in a free list
when hidma_prep_dma_*(memcpy/memset) is called, the flag will keep ACKed
if the descriptor has been used before. This will cause a BUG_ON in
async_tx_quiesce().
kernel BUG at crypto/async_tx/async_tx.c:282!
Internal error: Oops - BUG: 0 1 SMP
...
task: ffff8017dd3ec000 task.stack: ffff8017dd3e8000
PC is at async_tx_quiesce+0x54/0x78 [async_tx]
LR is at async_trigger_callback+0x98/0x110 [async_tx]
This patch initializes flags in dma_async_tx_descriptor by the flags
passed from the caller when hidma_prep_dma_*(memcpy/memset) is called.
Cc: Joey Zheng <yu.zheng@hxt-semitech.com>
Reviewed-by: Sinan Kaya <okaya@kernel.org>
Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com>
---
v2: add Reviewed-by: Sinan Kaya <okaya@kernel.org>
---
drivers/dma/qcom/hidma.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index 43d4b00b8138..9d639ed1955a 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -415,6 +415,7 @@ static int hidma_alloc_chan_resources(struct dma_chan *dmach)
if (!mdesc)
return NULL;
+ mdesc->desc.flags = flags;
hidma_ll_set_transfer_params(mdma->lldev, mdesc->tre_ch,
src, dest, len, flags,
HIDMA_TRE_MEMCPY);
@@ -447,6 +448,7 @@ static int hidma_alloc_chan_resources(struct dma_chan *dmach)
if (!mdesc)
return NULL;
+ mdesc->desc.flags = flags;
hidma_ll_set_transfer_params(mdma->lldev, mdesc->tre_ch,
value, dest, len, flags,
HIDMA_TRE_MEMSET);
^ permalink raw reply related
* dmaengine: fsl-edma: use struct_size() in kzalloc()
From: Gustavo A. R. Silva @ 2019-01-06 20:33 UTC (permalink / raw)
To: Angelo Dureghello; +Cc: Dan Williams, Vinod Koul, dmaengine, linux-kernel
On 1/6/19 1:40 PM, Angelo Dureghello wrote:
> On Fri, Jan 04, 2019 at 03:25:45PM -0600, Gustavo A. R. Silva wrote:
>> One of the more common cases of allocation size calculations is finding the
>> size of a structure that has a zero-sized array at the end, along with memory
>> for some number of elements for that array. For example:
>>
>> struct foo {
>> int stuff;
>> void *entry[];
>> };
>>
>> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
>>
>> Instead of leaving these open-coded and prone to type mistakes, we can now
>> use the new struct_size() helper:
>>
>> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>>
>> This code was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>> drivers/dma/fsl-edma-common.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
>> index 8876c4c1bb2c..fe529100674f 100644
>> --- a/drivers/dma/fsl-edma-common.c
>> +++ b/drivers/dma/fsl-edma-common.c
>> @@ -339,9 +339,7 @@ static struct fsl_edma_desc *fsl_edma_alloc_desc(struct fsl_edma_chan *fsl_chan,
>> struct fsl_edma_desc *fsl_desc;
>> int i;
>>
>> - fsl_desc = kzalloc(sizeof(*fsl_desc) +
>> - sizeof(struct fsl_edma_sw_tcd) *
>> - sg_len, GFP_NOWAIT);
>> + fsl_desc = kzalloc(struct_size(fsl_desc, tcd, sg_len), GFP_NOWAIT);
>> if (!fsl_desc)
>> return NULL;
>>
>
> Tested-by: Angelo Dureghello <angelo@sysam.it>
>
Thanks, Angelo.
---
Gustavo
^ permalink raw reply
* dmaengine: fsl-edma: use struct_size() in kzalloc()
From: Angelo Dureghello @ 2019-01-06 19:40 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Dan Williams, Vinod Koul, dmaengine, linux-kernel
On Fri, Jan 04, 2019 at 03:25:45PM -0600, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding the
> size of a structure that has a zero-sized array at the end, along with memory
> for some number of elements for that array. For example:
>
> struct foo {
> int stuff;
> void *entry[];
> };
>
> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can now
> use the new struct_size() helper:
>
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>
> This code was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/dma/fsl-edma-common.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index 8876c4c1bb2c..fe529100674f 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
> @@ -339,9 +339,7 @@ static struct fsl_edma_desc *fsl_edma_alloc_desc(struct fsl_edma_chan *fsl_chan,
> struct fsl_edma_desc *fsl_desc;
> int i;
>
> - fsl_desc = kzalloc(sizeof(*fsl_desc) +
> - sizeof(struct fsl_edma_sw_tcd) *
> - sg_len, GFP_NOWAIT);
> + fsl_desc = kzalloc(struct_size(fsl_desc, tcd, sg_len), GFP_NOWAIT);
> if (!fsl_desc)
> return NULL;
>
Tested-by: Angelo Dureghello <angelo@sysam.it>
> --
> 2.20.1
>
^ permalink raw reply
* [2/5] dmaengine: bcm2835: Fix abort of transactions
From: Lukas Wunner @ 2019-01-06 8:38 UTC (permalink / raw)
To: Stefan Wahren
Cc: Frank Pavlic, Martin Sperl, Florian Meier, dmaengine, Eric Anholt,
linux-rpi-kernel, Vinod Koul
On Fri, Dec 28, 2018 at 02:20:42PM +0100, Stefan Wahren wrote:
> Lukas Wunner <lukas@wunner.de> hat am 22. Dezember 2018 um 08:28 geschrieben:
> > drivers/dma/bcm2835-dma.c | 33 +++------------------------------
> > 1 file changed, 3 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> > index e94f41c56975..17bc7304db3a 100644
> > --- a/drivers/dma/bcm2835-dma.c
> > +++ b/drivers/dma/bcm2835-dma.c
> > @@ -419,25 +419,11 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
> > writel(0, chan_base + BCM2835_DMA_CS);
> >
> > /* Wait for any current AXI transfer to complete */
> > - while ((cs & BCM2835_DMA_ISPAUSED) && --timeout) {
> > + while ((readl(chan_base + BCM2835_DMA_CS) &
> > + BCM2835_DMA_WAITING_FOR_WRITES) && --timeout)
> > cpu_relax();
> > - cs = readl(chan_base + BCM2835_DMA_CS);
> > - }
> > -
> > - /* We'll un-pause when we set of our next DMA */
> > - if (!timeout)
> > - return -ETIMEDOUT;
>
> i'm only sceptical about silently ignoring the timeout case. I prefer
> to have a comment explaining why we proceed with BCM2835_DMA_RESET in
> this case and some kind of error / debug message like below.
>
> > - if (!timeout)
> > - dev_err(d->ddev.dev, "DMA transfer could not be terminated\n");
The Foundation's downstream tree contains an additional DMA driver
called drivers/dma/bcm2708-dmaengine.c:
https://github.com/raspberrypi/linux/commit/5fa54ef4d495
There's a code comment in that driver which sheds some light on the
abort algorithm's rationale:
"This routine waits for the current AXI transfer to complete before
terminating the current DMA. If the current transfer is hung on a DREQ used
by an uncooperative peripheral the AXI transfer may never complete. In this
case the routine times out and return a non-zero error code."
The author's intention seems to have been that a peripheral may buffer
a write if it has deasserted its DREQ signal and won't send an AXI
write response until it can process the write. And if the peripheral
is somehow stuck, that write response may never occur.
I'm not sure if that can actually happen in the real world or if it's
a purely theoretical issue. But the additional cost (in terms of LoC
and performance) is small, so I left it in.
There's not much we can do if the peripheral doesn't acknowledge writes,
so I just carry on resetting the channel.
I intend to amend the commit like below to address your objection,
let me know if you do not consider this sufficient. Thanks.
-- >8 --
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index ceec110..c3d9a26 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -417,8 +417,9 @@ static void bcm2835_dma_fill_cb_chain_with_sg(
}
}
-static int bcm2835_dma_abort(void __iomem *chan_base)
+static int bcm2835_dma_abort(struct bcm2835_chan *c)
{
+ void __iomem *chan_base = c->chan_base;
unsigned long cs;
long int timeout = 10000;
@@ -434,6 +435,11 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
BCM2835_DMA_WAITING_FOR_WRITES) && --timeout)
cpu_relax();
+ /* Uncooperative peripherals may not signal AXI write responses */
+ if (!timeout)
+ dev_err(c->vc.chan.device->dev,
+ "failed to complete outstanding writes\n");
+
writel(BCM2835_DMA_RESET, chan_base + BCM2835_DMA_CS);
return 0;
}
@@ -797,7 +803,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
if (c->desc) {
vchan_terminate_vdesc(&c->desc->vd);
c->desc = NULL;
- bcm2835_dma_abort(c->chan_base);
+ bcm2835_dma_abort(c);
}
vchan_get_all_descriptors(&c->vc, &head);
^ permalink raw reply related
* [2/8,v2] Documentation: bindings: k3dma: Add binding for dma-avail-chan
From: Manivannan Sadhasivam @ 2019-01-05 14:08 UTC (permalink / raw)
To: Vinod Koul
Cc: John Stultz, lkml, Rob Herring, Mark Rutland, Tanglei Han,
Zhuangluan Su, Ryan Grachek, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
Hi Vinod,
On Sat, Jan 05, 2019 at 07:16:10PM +0530, Vinod Koul wrote:
> On 05-01-19, 10:23, Manivannan Sadhasivam wrote:
> > On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> > > On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > Hi John,
> > > >
> > > > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > > > Some dma channels can be reserved for secure mode or other
> > > > > hardware on the SoC, so provide a binding for a bitmask
> > > > > listing the available channels for the kernel to use.
> > > > >
> > > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > > > Cc: Ryan Grachek <ryan@edited.us>
> > > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > Cc: dmaengine@vger.kernel.org
> > > > > Cc: devicetree@vger.kernel.org
> > > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > > ---
> > > > > Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > index 10a2f15..1c466c1 100644
> > > > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > @@ -14,6 +14,9 @@ Required properties:
> > > > > have specific request line
> > > > > - clocks: clock required
> > > > >
> > > > > +Optional properties:
> > > > > +- dma-avail-chan: Bitmask of available physical channels
> > > > > +
> > > >
> > > > This property looks too generic. Since this is specific to HiSi SoCs,
> > > > this could be "hisi-dma-avail-chan"?
> > >
> > > I'm fine to change it, but I'm not sure I fully understand the
> > > rational. Can you help me understand?
> > > Are device node-binding names supposed to have global scope? I assumed
> > > the node property names are basically scoped to the entry?
> >
> > IIUC properties documented in subsystem binding (dma.txt in this case)
> > will have global scope. Those which are not documented in this binding
> > are specific to vendor IPs and should be prefixed with the vendor
> > prefix (hisi in this case).
> >
> > > Further, having some dma channels be reserved doesn't seem to be too
> > > unique a concept, so I'm not sure what we gain long term by prefixing
> > > it?
> > >
> >
> > Right, but this brings up the point of having this functionality in
> > generic DMA engine so that the DMA controller drivers need not handle.
> > So either we should move this available channel check to DMA Engine
> > and document the property in dma.txt so that other IPs can also use it
> > or keep the functionality in K3 driver and use HiSi prefix for the
> > property.
> >
> > But I'd like to hear Vinod/Rob's opinion on this!
>
> So there are two parts, first is if this new property of using 'some'
> channels of controller is generic enough, the answer is unfortunately
> yes, so we should move this to dma.txt as a generic property
>
> But I don't agree the dmaengine core should handle it, we may add
> helpers, but controllers registers N channels and they would do so, core
> should not do filtering
>
Okay. But won't it create ambiguity? What if a new driver developer
assmes that he can use this property to filter the channels for his own
DMA controller? Since we are _explicitly_ stating that these channels
should be filtered, why the dmaengine core can't handle it?
If the property is generic, then it makes sense to keep the
functionality also generic.
Thanks,
Mani
> --
> ~Vinod
^ permalink raw reply
* [2/8,v2] Documentation: bindings: k3dma: Add binding for dma-avail-chan
From: Vinod Koul @ 2019-01-05 13:46 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: John Stultz, lkml, Rob Herring, Mark Rutland, Tanglei Han,
Zhuangluan Su, Ryan Grachek, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On 05-01-19, 10:23, Manivannan Sadhasivam wrote:
> On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> > On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > Hi John,
> > >
> > > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > > Some dma channels can be reserved for secure mode or other
> > > > hardware on the SoC, so provide a binding for a bitmask
> > > > listing the available channels for the kernel to use.
> > > >
> > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > > Cc: Ryan Grachek <ryan@edited.us>
> > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Cc: dmaengine@vger.kernel.org
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > ---
> > > > Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > index 10a2f15..1c466c1 100644
> > > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > @@ -14,6 +14,9 @@ Required properties:
> > > > have specific request line
> > > > - clocks: clock required
> > > >
> > > > +Optional properties:
> > > > +- dma-avail-chan: Bitmask of available physical channels
> > > > +
> > >
> > > This property looks too generic. Since this is specific to HiSi SoCs,
> > > this could be "hisi-dma-avail-chan"?
> >
> > I'm fine to change it, but I'm not sure I fully understand the
> > rational. Can you help me understand?
> > Are device node-binding names supposed to have global scope? I assumed
> > the node property names are basically scoped to the entry?
>
> IIUC properties documented in subsystem binding (dma.txt in this case)
> will have global scope. Those which are not documented in this binding
> are specific to vendor IPs and should be prefixed with the vendor
> prefix (hisi in this case).
>
> > Further, having some dma channels be reserved doesn't seem to be too
> > unique a concept, so I'm not sure what we gain long term by prefixing
> > it?
> >
>
> Right, but this brings up the point of having this functionality in
> generic DMA engine so that the DMA controller drivers need not handle.
> So either we should move this available channel check to DMA Engine
> and document the property in dma.txt so that other IPs can also use it
> or keep the functionality in K3 driver and use HiSi prefix for the
> property.
>
> But I'd like to hear Vinod/Rob's opinion on this!
So there are two parts, first is if this new property of using 'some'
channels of controller is generic enough, the answer is unfortunately
yes, so we should move this to dma.txt as a generic property
But I don't agree the dmaengine core should handle it, we may add
helpers, but controllers registers N channels and they would do so, core
should not do filtering
^ permalink raw reply
* [3/8,v2] dma: k3dma: Upgrade k3dma driver to support hisi_asp_dma hardware
From: Manivannan Sadhasivam @ 2019-01-05 5:48 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Youlin Wang, Dan Williams, Vinod Koul, Zhuangluan Su,
Ryan Grachek, dmaengine, Tanglei Han
On Fri, Jan 04, 2019 at 09:44:17PM -0800, John Stultz wrote:
> On Fri, Jan 4, 2019 at 9:39 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Fri, Jan 04, 2019 at 09:22:46PM -0800, John Stultz wrote:
> > > On Fri, Jan 4, 2019 at 7:42 PM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > > On Fri, Jan 04, 2019 at 12:56:23PM -0800, John Stultz wrote:
> > > > > From: Youlin Wang <wwx575822@notesmail.huawei.com>
> > > > >
> > > > > There is an new "hisi-pcm-asp-dma-1.0" device added in
> > > > > "arch/arm64/boot/dts/hisilicon/hi3660.dtsi".
> > > > > So we have to add a matching id in the driver file:
> > > > > .compatible = "hisilicon,hisi-pcm-asp-dma-1.0"
> > > > >
> > > > > And also hisi-pcm-asp dma device needs no setting to the clock.
> > > > > So we skip this by adding and using soc data flags.
> > > > >
> > > > > After above this driver will support both k3 and hisi_asp dma hardware.
> > > > >
> > > >
> > > > Small description about the hardware (ASP DMAC) would be really helpful.
> > >
> > > I've taken a shot at this (along with integrating your other feedback
> > > - thanks again for the review!), though as I don't have direct
> > > documentation, my knowledge is a bit second hand.
> > >
> > > See here:
> > > https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey960-mainline-WIP&id=754a79facf1af0b59a7f8fd63050da12ebf5521e
> > >
> > > Let me know if you have suggestions for more specific changes!
> >
> > Description looks good to me. But looks like you are not protecting the
> > other clk APIs like clk_prepare_enable and clk_disable_unprepare. Don't
> > they fail when the relevant clk is not found?
>
> No, those calls just return 0 if null clock is passed
>
> int clk_prepare(struct clk *clk)
> {
> if (!clk)
> return 0;
> ...
Ah, okay. Didn't look into the definition.
So with the change in description,
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Thanks,
Mani
>
> thanks
> -john
^ permalink raw reply
* [3/8,v2] dma: k3dma: Upgrade k3dma driver to support hisi_asp_dma hardware
From: John Stultz @ 2019-01-05 5:44 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: lkml, Youlin Wang, Dan Williams, Vinod Koul, Zhuangluan Su,
Ryan Grachek, dmaengine, Tanglei Han
On Fri, Jan 4, 2019 at 9:39 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Fri, Jan 04, 2019 at 09:22:46PM -0800, John Stultz wrote:
> > On Fri, Jan 4, 2019 at 7:42 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > > On Fri, Jan 04, 2019 at 12:56:23PM -0800, John Stultz wrote:
> > > > From: Youlin Wang <wwx575822@notesmail.huawei.com>
> > > >
> > > > There is an new "hisi-pcm-asp-dma-1.0" device added in
> > > > "arch/arm64/boot/dts/hisilicon/hi3660.dtsi".
> > > > So we have to add a matching id in the driver file:
> > > > .compatible = "hisilicon,hisi-pcm-asp-dma-1.0"
> > > >
> > > > And also hisi-pcm-asp dma device needs no setting to the clock.
> > > > So we skip this by adding and using soc data flags.
> > > >
> > > > After above this driver will support both k3 and hisi_asp dma hardware.
> > > >
> > >
> > > Small description about the hardware (ASP DMAC) would be really helpful.
> >
> > I've taken a shot at this (along with integrating your other feedback
> > - thanks again for the review!), though as I don't have direct
> > documentation, my knowledge is a bit second hand.
> >
> > See here:
> > https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey960-mainline-WIP&id=754a79facf1af0b59a7f8fd63050da12ebf5521e
> >
> > Let me know if you have suggestions for more specific changes!
>
> Description looks good to me. But looks like you are not protecting the
> other clk APIs like clk_prepare_enable and clk_disable_unprepare. Don't
> they fail when the relevant clk is not found?
No, those calls just return 0 if null clock is passed
int clk_prepare(struct clk *clk)
{
if (!clk)
return 0;
...
thanks
-john
^ permalink raw reply
* [3/8,v2] dma: k3dma: Upgrade k3dma driver to support hisi_asp_dma hardware
From: Manivannan Sadhasivam @ 2019-01-05 5:39 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Youlin Wang, Dan Williams, Vinod Koul, Zhuangluan Su,
Ryan Grachek, dmaengine, Tanglei Han
On Fri, Jan 04, 2019 at 09:22:46PM -0800, John Stultz wrote:
> On Fri, Jan 4, 2019 at 7:42 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> > On Fri, Jan 04, 2019 at 12:56:23PM -0800, John Stultz wrote:
> > > From: Youlin Wang <wwx575822@notesmail.huawei.com>
> > >
> > > There is an new "hisi-pcm-asp-dma-1.0" device added in
> > > "arch/arm64/boot/dts/hisilicon/hi3660.dtsi".
> > > So we have to add a matching id in the driver file:
> > > .compatible = "hisilicon,hisi-pcm-asp-dma-1.0"
> > >
> > > And also hisi-pcm-asp dma device needs no setting to the clock.
> > > So we skip this by adding and using soc data flags.
> > >
> > > After above this driver will support both k3 and hisi_asp dma hardware.
> > >
> >
> > Small description about the hardware (ASP DMAC) would be really helpful.
>
> I've taken a shot at this (along with integrating your other feedback
> - thanks again for the review!), though as I don't have direct
> documentation, my knowledge is a bit second hand.
>
> See here:
> https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey960-mainline-WIP&id=754a79facf1af0b59a7f8fd63050da12ebf5521e
>
> Let me know if you have suggestions for more specific changes!
Description looks good to me. But looks like you are not protecting the
other clk APIs like clk_prepare_enable and clk_disable_unprepare. Don't
they fail when the relevant clk is not found?
Thanks,
Mani
> thanks
> -john
^ permalink raw reply
* [3/8,v2] dma: k3dma: Upgrade k3dma driver to support hisi_asp_dma hardware
From: John Stultz @ 2019-01-05 5:22 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: lkml, Youlin Wang, Dan Williams, Vinod Koul, Zhuangluan Su,
Ryan Grachek, dmaengine, Tanglei Han
On Fri, Jan 4, 2019 at 7:42 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
> On Fri, Jan 04, 2019 at 12:56:23PM -0800, John Stultz wrote:
> > From: Youlin Wang <wwx575822@notesmail.huawei.com>
> >
> > There is an new "hisi-pcm-asp-dma-1.0" device added in
> > "arch/arm64/boot/dts/hisilicon/hi3660.dtsi".
> > So we have to add a matching id in the driver file:
> > .compatible = "hisilicon,hisi-pcm-asp-dma-1.0"
> >
> > And also hisi-pcm-asp dma device needs no setting to the clock.
> > So we skip this by adding and using soc data flags.
> >
> > After above this driver will support both k3 and hisi_asp dma hardware.
> >
>
> Small description about the hardware (ASP DMAC) would be really helpful.
I've taken a shot at this (along with integrating your other feedback
- thanks again for the review!), though as I don't have direct
documentation, my knowledge is a bit second hand.
See here:
https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey960-mainline-WIP&id=754a79facf1af0b59a7f8fd63050da12ebf5521e
Let me know if you have suggestions for more specific changes!
thanks
-john
^ permalink raw reply
* [2/8,v2] Documentation: bindings: k3dma: Add binding for dma-avail-chan
From: John Stultz @ 2019-01-05 4:58 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: lkml, Vinod Koul, Rob Herring, Mark Rutland, Tanglei Han,
Zhuangluan Su, Ryan Grachek, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On Fri, Jan 4, 2019 at 8:53 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> > On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > Hi John,
> > >
> > > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > > Some dma channels can be reserved for secure mode or other
> > > > hardware on the SoC, so provide a binding for a bitmask
> > > > listing the available channels for the kernel to use.
> > > >
> > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > > Cc: Ryan Grachek <ryan@edited.us>
> > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Cc: dmaengine@vger.kernel.org
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > ---
> > > > Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > index 10a2f15..1c466c1 100644
> > > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > @@ -14,6 +14,9 @@ Required properties:
> > > > have specific request line
> > > > - clocks: clock required
> > > >
> > > > +Optional properties:
> > > > +- dma-avail-chan: Bitmask of available physical channels
> > > > +
> > >
> > > This property looks too generic. Since this is specific to HiSi SoCs,
> > > this could be "hisi-dma-avail-chan"?
> >
> > I'm fine to change it, but I'm not sure I fully understand the
> > rational. Can you help me understand?
> > Are device node-binding names supposed to have global scope? I assumed
> > the node property names are basically scoped to the entry?
>
> IIUC properties documented in subsystem binding (dma.txt in this case)
> will have global scope. Those which are not documented in this binding
> are specific to vendor IPs and should be prefixed with the vendor
> prefix (hisi in this case).
Thanks I appreciate the explanation here. I hadn't really understood
this point, and really haven't developed much "taste" in what makes a
good or bad binding.
> > Further, having some dma channels be reserved doesn't seem to be too
> > unique a concept, so I'm not sure what we gain long term by prefixing
> > it?
> >
>
> Right, but this brings up the point of having this functionality in
> generic DMA engine so that the DMA controller drivers need not handle.
> So either we should move this available channel check to DMA Engine
> and document the property in dma.txt so that other IPs can also use it
> or keep the functionality in K3 driver and use HiSi prefix for the
> property.
>
> But I'd like to hear Vinod/Rob's opinion on this!
Sure. Though for now I'll prefix it as the logic is handled at the
driver level.
thanks
-john
^ permalink raw reply
* [2/8,v2] Documentation: bindings: k3dma: Add binding for dma-avail-chan
From: Manivannan Sadhasivam @ 2019-01-05 4:53 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Vinod Koul, Rob Herring, Mark Rutland, Tanglei Han,
Zhuangluan Su, Ryan Grachek, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > Hi John,
> >
> > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > Some dma channels can be reserved for secure mode or other
> > > hardware on the SoC, so provide a binding for a bitmask
> > > listing the available channels for the kernel to use.
> > >
> > > Cc: Vinod Koul <vkoul@kernel.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > Cc: Ryan Grachek <ryan@edited.us>
> > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Cc: dmaengine@vger.kernel.org
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > ---
> > > Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > index 10a2f15..1c466c1 100644
> > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > @@ -14,6 +14,9 @@ Required properties:
> > > have specific request line
> > > - clocks: clock required
> > >
> > > +Optional properties:
> > > +- dma-avail-chan: Bitmask of available physical channels
> > > +
> >
> > This property looks too generic. Since this is specific to HiSi SoCs,
> > this could be "hisi-dma-avail-chan"?
>
> I'm fine to change it, but I'm not sure I fully understand the
> rational. Can you help me understand?
> Are device node-binding names supposed to have global scope? I assumed
> the node property names are basically scoped to the entry?
IIUC properties documented in subsystem binding (dma.txt in this case)
will have global scope. Those which are not documented in this binding
are specific to vendor IPs and should be prefixed with the vendor
prefix (hisi in this case).
> Further, having some dma channels be reserved doesn't seem to be too
> unique a concept, so I'm not sure what we gain long term by prefixing
> it?
>
Right, but this brings up the point of having this functionality in
generic DMA engine so that the DMA controller drivers need not handle.
So either we should move this available channel check to DMA Engine
and document the property in dma.txt so that other IPs can also use it
or keep the functionality in K3 driver and use HiSi prefix for the
property.
But I'd like to hear Vinod/Rob's opinion on this!
Thanks,
Mani
> thanks
> -john
^ permalink raw reply
* [2/8,v2] Documentation: bindings: k3dma: Add binding for dma-avail-chan
From: John Stultz @ 2019-01-05 4:39 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: lkml, Vinod Koul, Rob Herring, Mark Rutland, Tanglei Han,
Zhuangluan Su, Ryan Grachek, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Hi John,
>
> On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > Some dma channels can be reserved for secure mode or other
> > hardware on the SoC, so provide a binding for a bitmask
> > listing the available channels for the kernel to use.
> >
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Tanglei Han <hantanglei@huawei.com>
> > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > Cc: Ryan Grachek <ryan@edited.us>
> > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Cc: dmaengine@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> > Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > index 10a2f15..1c466c1 100644
> > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > @@ -14,6 +14,9 @@ Required properties:
> > have specific request line
> > - clocks: clock required
> >
> > +Optional properties:
> > +- dma-avail-chan: Bitmask of available physical channels
> > +
>
> This property looks too generic. Since this is specific to HiSi SoCs,
> this could be "hisi-dma-avail-chan"?
I'm fine to change it, but I'm not sure I fully understand the
rational. Can you help me understand?
Are device node-binding names supposed to have global scope? I assumed
the node property names are basically scoped to the entry?
Further, having some dma channels be reserved doesn't seem to be too
unique a concept, so I'm not sure what we gain long term by prefixing
it?
thanks
-john
^ permalink raw reply
* [2/8,v2] Documentation: bindings: k3dma: Add binding for dma-avail-chan
From: Manivannan Sadhasivam @ 2019-01-05 4:00 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Vinod Koul, Rob Herring, Mark Rutland, Tanglei Han,
Zhuangluan Su, Ryan Grachek, dmaengine, devicetree
Hi John,
On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> Some dma channels can be reserved for secure mode or other
> hardware on the SoC, so provide a binding for a bitmask
> listing the available channels for the kernel to use.
>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Tanglei Han <hantanglei@huawei.com>
> Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> Cc: Ryan Grachek <ryan@edited.us>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: dmaengine@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> index 10a2f15..1c466c1 100644
> --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> @@ -14,6 +14,9 @@ Required properties:
> have specific request line
> - clocks: clock required
>
> +Optional properties:
> +- dma-avail-chan: Bitmask of available physical channels
> +
This property looks too generic. Since this is specific to HiSi SoCs,
this could be "hisi-dma-avail-chan"?
Thanks,
Mani
> Example:
>
> Controller:
> --
> 2.7.4
>
^ permalink raw reply
* [4/8,v2] dma: k3dma: Delete axi_config
From: Manivannan Sadhasivam @ 2019-01-05 3:44 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Li Yu, Dan Williams, Vinod Koul, Tanglei Han, Zhuangluan Su,
Ryan Grachek, dmaengine, Guodong Xu
On Fri, Jan 04, 2019 at 12:56:24PM -0800, John Stultz wrote:
> From: Li Yu <liyu65@hisilicon.com>
>
> Axi_config controls whether DMA resources can be accessed in non-secure
> mode, such as linux kernel. The register should be set by the bootloader
> stage and depends on the device.
>
> Thus, this patch removes axi_config from k3dma driver.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Tanglei Han <hantanglei@huawei.com>
> Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> Cc: Ryan Grachek <ryan@edited.us>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: dmaengine@vger.kernel.org
> Signed-off-by: Li Yu <liyu65@hisilicon.com>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> [jstultz: Minor tweaks to commit message]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Thanks,
Mani
> ---
> drivers/dma/k3dma.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
> index df61406..b2060bf 100644
> --- a/drivers/dma/k3dma.c
> +++ b/drivers/dma/k3dma.c
> @@ -52,8 +52,6 @@
> #define CX_SRC 0x814
> #define CX_DST 0x818
> #define CX_CFG 0x81c
> -#define AXI_CFG 0x820
> -#define AXI_CFG_DEFAULT 0x201201
>
> #define CX_LLI_CHAIN_EN 0x2
> #define CX_CFG_EN 0x1
> @@ -168,7 +166,6 @@ static void k3_dma_set_desc(struct k3_dma_phy *phy, struct k3_desc_hw *hw)
> writel_relaxed(hw->count, phy->base + CX_CNT0);
> writel_relaxed(hw->saddr, phy->base + CX_SRC);
> writel_relaxed(hw->daddr, phy->base + CX_DST);
> - writel_relaxed(AXI_CFG_DEFAULT, phy->base + AXI_CFG);
> writel_relaxed(hw->config, phy->base + CX_CFG);
> }
>
> --
> 2.7.4
>
^ 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