* [v2,3/4] dmaengine: imx-sdma: implement channel termination via worker
From: Lucas Stach @ 2018-09-14 17:06 UTC (permalink / raw)
To: Vinod Koul
Cc: dmaengine, linux-kernel, Robin Gong, linux-imx, kernel,
patchwork-lst
The dmaengine documentation states that device_terminate_all may be
asynchronous and need not wait for the active transfers have stopped.
This allows us to move most of the functionality currently implemented
in the sdma channel termination function to run in a worker, outside
of any atomic context. Moving this out of atomic context has two
benefits: we can now sleep while waiting for the channel to terminate,
instead of busy waiting and the freeing of the dma descriptors happens
with IRQs enabled, getting rid of a warning in the dma mapping code.
As the termination is now asnc, we need to implement the
device_synchronize dma engine function, which simply waits for the
worker to finish its execution.
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
v2: Keep vchan_get_all_descriptors in the terminate_all call, so the
worker doesn't corrupt the next transfer if that's already in
the setup process.
---
drivers/dma/imx-sdma.c | 42 ++++++++++++++++++++++++++++++++----------
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 8d2fec8b16cc..da41e8fbf151 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -32,6 +32,7 @@
#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/of_dma.h>
+#include <linux/workqueue.h>
#include <asm/irq.h>
#include <linux/platform_data/dma-imx-sdma.h>
@@ -375,6 +376,8 @@ struct sdma_channel {
u32 shp_addr, per_addr;
enum dma_status status;
struct imx_dma_data data;
+ struct work_struct terminate_worker;
+ struct list_head deferred_desc_list;
};
#define IMX_DMA_SG_LOOP BIT(0)
@@ -1025,31 +1028,47 @@ static int sdma_disable_channel(struct dma_chan *chan)
return 0;
}
+static void sdma_channel_terminate(struct work_struct *work)
+{
+ struct sdma_channel *sdmac = container_of(work, struct sdma_channel,
+ terminate_worker);
+
+ /*
+ * According to NXP R&D team a delay of one BD SDMA cost time
+ * (maximum is 1ms) should be added after disable of the channel
+ * bit, to ensure SDMA core has really been stopped after SDMA
+ * clients call .device_terminate_all.
+ */
+ usleep_range(1000, 2000);
+
+ vchan_dma_desc_free_list(&sdmac->vc, &sdmac->deferred_desc_list);
+ INIT_LIST_HEAD(&sdmac->deferred_desc_list);
+}
static int sdma_disable_channel_with_delay(struct dma_chan *chan)
{
struct sdma_channel *sdmac = to_sdma_chan(chan);
unsigned long flags;
- LIST_HEAD(head);
sdma_disable_channel(chan);
+
spin_lock_irqsave(&sdmac->vc.lock, flags);
- vchan_get_all_descriptors(&sdmac->vc, &head);
+ vchan_get_all_descriptors(&sdmac->vc, &sdmac->deferred_desc_list);
sdmac->desc = NULL;
spin_unlock_irqrestore(&sdmac->vc.lock, flags);
- vchan_dma_desc_free_list(&sdmac->vc, &head);
- /*
- * According to NXP R&D team a delay of one BD SDMA cost time
- * (maximum is 1ms) should be added after disable of the channel
- * bit, to ensure SDMA core has really been stopped after SDMA
- * clients call .device_terminate_all.
- */
- mdelay(1);
+ schedule_work(&sdmac->terminate_worker);
return 0;
}
+static void sdma_channel_synchronize(struct dma_chan *chan)
+{
+ struct sdma_channel *sdmac = to_sdma_chan(chan);
+
+ flush_work(&sdmac->terminate_worker);
+}
+
static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
{
struct sdma_engine *sdma = sdmac->sdma;
@@ -1993,6 +2012,8 @@ static int sdma_probe(struct platform_device *pdev)
sdmac->channel = i;
sdmac->vc.desc_free = sdma_desc_free;
+ INIT_WORK(&sdmac->terminate_worker, sdma_channel_terminate);
+ INIT_LIST_HEAD(&sdmac->deferred_desc_list);
/*
* Add the channel to the DMAC list. Do not add channel 0 though
* because we need it internally in the SDMA driver. This also means
@@ -2045,6 +2066,7 @@ static int sdma_probe(struct platform_device *pdev)
sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
sdma->dma_device.device_config = sdma_config;
sdma->dma_device.device_terminate_all = sdma_disable_channel_with_delay;
+ sdma->dma_device.device_synchronize = sdma_channel_synchronize;
sdma->dma_device.src_addr_widths = SDMA_DMA_BUSWIDTHS;
sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
^ permalink raw reply related
* [v2,4/4] dmaengine: imx-sdma: use GFP_NOWAIT for dma allocations
From: Lucas Stach @ 2018-09-14 17:06 UTC (permalink / raw)
To: Vinod Koul
Cc: dmaengine, linux-kernel, Robin Gong, linux-imx, kernel,
patchwork-lst
DMA buffer descriptors aren't allocated from atomic context, so they
can use the less heavyweigth GFP_NOWAIT.
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
drivers/dma/imx-sdma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index da41e8fbf151..ac189d3d985c 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1213,7 +1213,7 @@ static int sdma_alloc_bd(struct sdma_desc *desc)
int ret = 0;
desc->bd = dma_zalloc_coherent(NULL, bd_size, &desc->bd_phys,
- GFP_ATOMIC);
+ GFP_NOWAIT);
if (!desc->bd) {
ret = -ENOMEM;
goto out;
^ permalink raw reply related
* [-next,v2] dmaengine: mcf-edma: fix x86_64 allmodconfig compilation warning
From: Angelo Dureghello @ 2018-09-14 18:51 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Vinod Koul, dmaengine, Stephen Rothwell
Hi Geert,
On Fri, Sep 14, 2018 at 02:13:50PM +0200, Angelo Dureghello wrote:
> Hi Geert,
>
> On Fri, Sep 14, 2018 at 01:35:22PM +0200, Geert Uytterhoeven wrote:
> > Hi Angelo,
> >
> > Thanks for your patch!
> >
> > On Fri, Sep 14, 2018 at 9:12 AM Angelo Dureghello <angelo@sysam.it> wrote:
> > > This patch fixes the compilation warning reported
> > > during x86_64 allmodconfig build.
> >
> > Please quote the warning, so people know what it is about:
> >
> > drivers/dma/mcf-edma.c: In function 'mcf_edma_filter_fn':
> > drivers/dma/mcf-edma.c:296:33: warning: cast from pointer to
> > integer of different size [-Wpointer-to-int-cast]
> > return (mcf_chan->slave_id == (u32)param);
> >
>
> Ok done.
>
> > > Reported-By: Stephen Rothwell <sfr@canb.auug.org.au>
> > > Signed-off-by: Angelo Dureghello <angelo@sysam.it>
> > > ---
> > > Changes for v2:
> > > - added Reported-By
> > > ---
> > > drivers/dma/mcf-edma.c | 3 ++-
> > > include/linux/platform_data/dma-mcf-edma.h | 2 +-
> > > 2 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/dma/mcf-edma.c b/drivers/dma/mcf-edma.c
> > > index 4d30d5302649..e08e2409a102 100644
> > > --- a/drivers/dma/mcf-edma.c
> > > +++ b/drivers/dma/mcf-edma.c
> > > @@ -292,8 +292,9 @@ bool mcf_edma_filter_fn(struct dma_chan *chan, void *param)
> > > {
> > > if (chan->device->dev->driver == &mcf_edma_driver.driver) {
> > > struct fsl_edma_chan *mcf_chan = to_fsl_edma_chan(chan);
> > > + unsigned int req = *(unsigned int *)param;
> >
> > This looks a bit hackish to me.
> > >
> > > - return (mcf_chan->slave_id == (u32)param);
> >
> > The recommended way to cast from pointers to integers is to
> > use uintptr_t, which always has the same size as a pointer, i.e.:
> >
>
> slave_id is acually an u32. So, if i understsand, when compiled
> on x86_64 using uintptr_t, the cast should trigger the same issue.
> I preferred, as i.e. ti/omap dma driver is doing, to simply
> use a pointer to an int (int[]) as passed type in param,
> so i can then indirectly access the integer. I tested
> proper functionality in ColdFire and that there are no warnings
> anymore in both ColdFire and x86_64 allmodconfig so the patch,
> hopefully, should be legal.
>
I misunderstood uintptr_t, verified now that it works fine
for this case, reducing the patch to a single line change.
So switching to it. Thanks !
> > return (mcf_chan->slave_id == (uintptr_t)param);
> >
> > > + return (mcf_chan->slave_id == req);
> > > }
> > >
> > > return false;
> > > diff --git a/include/linux/platform_data/dma-mcf-edma.h b/include/linux/platform_data/dma-mcf-edma.h
> > > index d718ccfa3421..97cb79bda646 100644
> > > --- a/include/linux/platform_data/dma-mcf-edma.h
> > > +++ b/include/linux/platform_data/dma-mcf-edma.h
> > > @@ -21,7 +21,7 @@ struct dma_slave_map;
> > >
> > > bool mcf_edma_filter_fn(struct dma_chan *chan, void *param);
> > >
> > > -#define MCF_EDMA_FILTER_PARAM(ch) ((void *)ch)
> > > +#define MCF_EDMA_FILTER_PARAM(ch) ((int[]) { (ch) })
> > >
> > > /**
> > > * struct mcf_edma_platform_data - platform specific data for eDMA engine
> >
> > Gr{oetje,eeting}s,
> >
> > Geert
> >
> Regards,
> Angelo
>
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> > -- Linus Torvalds
Regards,
Angelo
^ permalink raw reply
* driver/dma/ioat: Call del_timer_sync() without holding prep_lock
From: Waiman Long @ 2018-09-14 18:53 UTC (permalink / raw)
To: Vinod Koul
Cc: linux-kernel, dmaengine, Dan Williams, Dave Jiang, Kees Cook,
Christophe JAILLET, Waiman Long
The following lockdep splat was observed:
[ 1222.241750] ======================================================
[ 1222.271301] WARNING: possible circular locking dependency detected
[ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
[ 1222.326659] ------------------------------------------------------
[ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
[ 1222.382660] ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
[ 1222.422928]
[ 1222.422928] but task is already holding lock:
[ 1222.451743] (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
:
[ 1223.524987] Chain exists of:
[ 1223.524987] (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
[ 1223.524987]
[ 1223.594082] Possible unsafe locking scenario:
[ 1223.594082]
[ 1223.622630] CPU0 CPU1
[ 1223.645080] ---- ----
[ 1223.667404] lock(&(&ioat_chan->prep_lock)->rlock);
[ 1223.691535] lock(&(&ioat_chan->cleanup_lock)->rlock);
[ 1223.728657] lock(&(&ioat_chan->prep_lock)->rlock);
[ 1223.765122] lock((&ioat_chan->timer));
[ 1223.784095]
[ 1223.784095] *** DEADLOCK ***
[ 1223.784095]
[ 1223.813492] 4 locks held by systemd-shutdow/1:
[ 1223.834677] #0: (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
[ 1223.873310] #1: (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
[ 1223.913604] #2: (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
[ 1223.954000] #3: (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
In the ioat_shutdown() function:
spin_lock_bh(&ioat_chan->prep_lock);
set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
del_timer_sync(&ioat_chan->timer);
spin_unlock_bh(&ioat_chan->prep_lock);
According to the synchronization rule for the del_timer_sync() function,
the caller must not hold locks which would prevent completion of the
timer's handler.
The timer structure has its own lock that manages its synchronization.
Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
trying to use that device anyway, there is probably no need to call
del_timer_sync() while holding the prep_lock. So the del_timer_sync()
call is now moved outside of the prep_lock critical section to prevent
the circular lock dependency.
Signed-off-by: Waiman Long <longman@redhat.com>
---
drivers/dma/ioat/init.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 4fa4c06..21a5708 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1205,8 +1205,15 @@ static void ioat_shutdown(struct pci_dev *pdev)
spin_lock_bh(&ioat_chan->prep_lock);
set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
- del_timer_sync(&ioat_chan->timer);
spin_unlock_bh(&ioat_chan->prep_lock);
+ /*
+ * Synchronization rule for del_timer_sync():
+ * - The caller must not hold locks which would prevent
+ * completion of the timer's handler.
+ * So prep_lock cannot be held before calling it.
+ */
+ del_timer_sync(&ioat_chan->timer);
+
/* this should quiesce then reset */
ioat_reset_hw(ioat_chan);
}
^ permalink raw reply related
* [-next,v3] dmaengine: mcf-edma: avoid warning for wrong pointer cast
From: Angelo Dureghello @ 2018-09-14 19:13 UTC (permalink / raw)
To: vinod.koul; +Cc: dmaengine, sfr, Angelo Dureghello
This patch fixes the following compilation warning
reported during x86_64 allmodconfig build:
drivers/dma/mcf-edma.c: In function 'mcf_edma_filter_fn':
drivers/dma/mcf-edma.c:296:33: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
return (mcf_chan->slave_id == (u32)param);
Reported-By: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Angelo Dureghello <angelo@sysam.it>
---
Changes for v2:
- added Reported-By
Changes for v3:
- added more details about the patch
- changed channel request cast using uintptr_t
---
drivers/dma/mcf-edma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/mcf-edma.c b/drivers/dma/mcf-edma.c
index 4d30d5302649..5de1b07eddff 100644
--- a/drivers/dma/mcf-edma.c
+++ b/drivers/dma/mcf-edma.c
@@ -293,7 +293,7 @@ bool mcf_edma_filter_fn(struct dma_chan *chan, void *param)
if (chan->device->dev->driver == &mcf_edma_driver.driver) {
struct fsl_edma_chan *mcf_chan = to_fsl_edma_chan(chan);
- return (mcf_chan->slave_id == (u32)param);
+ return (mcf_chan->slave_id == (uintptr_t)param);
}
return false;
^ permalink raw reply related
* driver/dma/ioat: Call del_timer_sync() without holding prep_lock
From: Dave Jiang @ 2018-09-14 19:35 UTC (permalink / raw)
To: Waiman Long, Vinod Koul
Cc: linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org,
Williams, Dan J, Kees Cook, Christophe JAILLET
On 09/14/2018 11:53 AM, Waiman Long wrote:
> The following lockdep splat was observed:
>
> [ 1222.241750] ======================================================
> [ 1222.271301] WARNING: possible circular locking dependency detected
> [ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
> [ 1222.326659] ------------------------------------------------------
> [ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
> [ 1222.382660] ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
> [ 1222.422928]
> [ 1222.422928] but task is already holding lock:
> [ 1222.451743] (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
> :
> [ 1223.524987] Chain exists of:
> [ 1223.524987] (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
> [ 1223.524987]
> [ 1223.594082] Possible unsafe locking scenario:
> [ 1223.594082]
> [ 1223.622630] CPU0 CPU1
> [ 1223.645080] ---- ----
> [ 1223.667404] lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.691535] lock(&(&ioat_chan->cleanup_lock)->rlock);
> [ 1223.728657] lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.765122] lock((&ioat_chan->timer));
> [ 1223.784095]
> [ 1223.784095] *** DEADLOCK ***
> [ 1223.784095]
> [ 1223.813492] 4 locks held by systemd-shutdow/1:
> [ 1223.834677] #0: (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
> [ 1223.873310] #1: (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
> [ 1223.913604] #2: (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
> [ 1223.954000] #3: (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
>
> In the ioat_shutdown() function:
>
> spin_lock_bh(&ioat_chan->prep_lock);
> set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> del_timer_sync(&ioat_chan->timer);
> spin_unlock_bh(&ioat_chan->prep_lock);
>
> According to the synchronization rule for the del_timer_sync() function,
> the caller must not hold locks which would prevent completion of the
> timer's handler.
>
> The timer structure has its own lock that manages its synchronization.
> Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
> trying to use that device anyway, there is probably no need to call
> del_timer_sync() while holding the prep_lock. So the del_timer_sync()
> call is now moved outside of the prep_lock critical section to prevent
> the circular lock dependency.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
Thanks!
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/dma/ioat/init.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 4fa4c06..21a5708 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -1205,8 +1205,15 @@ static void ioat_shutdown(struct pci_dev *pdev)
>
> spin_lock_bh(&ioat_chan->prep_lock);
> set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> - del_timer_sync(&ioat_chan->timer);
> spin_unlock_bh(&ioat_chan->prep_lock);
> + /*
> + * Synchronization rule for del_timer_sync():
> + * - The caller must not hold locks which would prevent
> + * completion of the timer's handler.
> + * So prep_lock cannot be held before calling it.
> + */
> + del_timer_sync(&ioat_chan->timer);
> +
> /* this should quiesce then reset */
> ioat_reset_hw(ioat_chan);
> }
>
^ permalink raw reply
* [v2,3/4] dmaengine: imx-sdma: implement channel termination via worker
From: Robin Gong @ 2018-09-17 7:51 UTC (permalink / raw)
To: Lucas Stach, Vinod Koul
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
dl-linux-imx, kernel@pengutronix.de, patchwork-lst@pengutronix.de
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2018年9月15日 1:06
> To: Vinod Koul <vkoul@kernel.org>
> Cc: dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; Robin Gong
> <yibin.gong@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; patchwork-lst@pengutronix.de
> Subject: [PATCH v2 3/4] dmaengine: imx-sdma: implement channel termination
> via worker
>
> The dmaengine documentation states that device_terminate_all may be
> asynchronous and need not wait for the active transfers have stopped.
>
> This allows us to move most of the functionality currently implemented in the
> sdma channel termination function to run in a worker, outside of any atomic
> context. Moving this out of atomic context has two
> benefits: we can now sleep while waiting for the channel to terminate, instead
> of busy waiting and the freeing of the dma descriptors happens with IRQs
> enabled, getting rid of a warning in the dma mapping code.
>
> As the termination is now asnc, we need to implement the device_synchronize
> dma engine function, which simply waits for the worker to finish its execution.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> v2: Keep vchan_get_all_descriptors in the terminate_all call, so the
> worker doesn't corrupt the next transfer if that's already in
> the setup process.
> ---
> drivers/dma/imx-sdma.c | 42 ++++++++++++++++++++++++++++++++----------
> 1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> 8d2fec8b16cc..da41e8fbf151 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -32,6 +32,7 @@
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> #include <linux/of_dma.h>
> +#include <linux/workqueue.h>
>
> #include <asm/irq.h>
> #include <linux/platform_data/dma-imx-sdma.h>
> @@ -375,6 +376,8 @@ struct sdma_channel {
> u32 shp_addr, per_addr;
> enum dma_status status;
> struct imx_dma_data data;
> + struct work_struct terminate_worker;
> + struct list_head deferred_desc_list;
> };
>
> #define IMX_DMA_SG_LOOP BIT(0)
> @@ -1025,31 +1028,47 @@ static int sdma_disable_channel(struct dma_chan
> *chan)
>
> return 0;
> }
> +static void sdma_channel_terminate(struct work_struct *work) {
> + struct sdma_channel *sdmac = container_of(work, struct sdma_channel,
> + terminate_worker);
> +
> + /*
> + * According to NXP R&D team a delay of one BD SDMA cost time
> + * (maximum is 1ms) should be added after disable of the channel
> + * bit, to ensure SDMA core has really been stopped after SDMA
> + * clients call .device_terminate_all.
> + */
> + usleep_range(1000, 2000);
> +
> + vchan_dma_desc_free_list(&sdmac->vc, &sdmac->deferred_desc_list);
> + INIT_LIST_HEAD(&sdmac->deferred_desc_list);
> +}
>
> static int sdma_disable_channel_with_delay(struct dma_chan *chan) {
> struct sdma_channel *sdmac = to_sdma_chan(chan);
> unsigned long flags;
> - LIST_HEAD(head);
>
> sdma_disable_channel(chan);
> +
> spin_lock_irqsave(&sdmac->vc.lock, flags);
> - vchan_get_all_descriptors(&sdmac->vc, &head);
> + vchan_get_all_descriptors(&sdmac->vc, &sdmac->deferred_desc_list);
> sdmac->desc = NULL;
> spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> - vchan_dma_desc_free_list(&sdmac->vc, &head);
>
> - /*
> - * According to NXP R&D team a delay of one BD SDMA cost time
> - * (maximum is 1ms) should be added after disable of the channel
> - * bit, to ensure SDMA core has really been stopped after SDMA
> - * clients call .device_terminate_all.
> - */
> - mdelay(1);
> + schedule_work(&sdmac->terminate_worker);
>
> return 0;
> }
>
> +static void sdma_channel_synchronize(struct dma_chan *chan) {
> + struct sdma_channel *sdmac = to_sdma_chan(chan);
> +
Please add the below to make sure internal virt dma tasklet killed too.
tasklet_kill(&sdmac->vc.task);
> + flush_work(&sdmac->terminate_worker);
> +}
> +
> static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
> {
> struct sdma_engine *sdma = sdmac->sdma; @@ -1993,6 +2012,8 @@
> static int sdma_probe(struct platform_device *pdev)
>
> sdmac->channel = i;
> sdmac->vc.desc_free = sdma_desc_free;
> + INIT_WORK(&sdmac->terminate_worker,
> sdma_channel_terminate);
> + INIT_LIST_HEAD(&sdmac->deferred_desc_list);
> /*
> * Add the channel to the DMAC list. Do not add channel 0 though
> * because we need it internally in the SDMA driver. This also means
> @@ -2045,6 +2066,7 @@ static int sdma_probe(struct platform_device
> *pdev)
> sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
> sdma->dma_device.device_config = sdma_config;
> sdma->dma_device.device_terminate_all =
> sdma_disable_channel_with_delay;
> + sdma->dma_device.device_synchronize = sdma_channel_synchronize;
> sdma->dma_device.src_addr_widths = SDMA_DMA_BUSWIDTHS;
> sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
> sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
> --
> 2.19.0
^ permalink raw reply
* [v2,3/4] dmaengine: imx-sdma: implement channel termination via worker
From: Lucas Stach @ 2018-09-17 11:03 UTC (permalink / raw)
To: Robin Gong, Vinod Koul
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
dl-linux-imx, kernel@pengutronix.de, patchwork-lst@pengutronix.de
Hi Robin,
Am Montag, den 17.09.2018, 07:51 +0000 schrieb Robin Gong:
> > -----Original Message-----
> > > > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: 2018年9月15日 1:06
> > > > To: Vinod Koul <vkoul@kernel.org>
> > > > > > Cc: dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; Robin Gong
> > > > > > <yibin.gong@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; patchwork-lst@pengutronix.de
> > Subject: [PATCH v2 3/4] dmaengine: imx-sdma: implement channel termination
> > via worker
> >
> > The dmaengine documentation states that device_terminate_all may be
> > asynchronous and need not wait for the active transfers have stopped.
> >
> > This allows us to move most of the functionality currently implemented in the
> > sdma channel termination function to run in a worker, outside of any atomic
> > context. Moving this out of atomic context has two
> > benefits: we can now sleep while waiting for the channel to terminate, instead
> > of busy waiting and the freeing of the dma descriptors happens with IRQs
> > enabled, getting rid of a warning in the dma mapping code.
> >
> > As the termination is now asnc, we need to implement the device_synchronize
> > dma engine function, which simply waits for the worker to finish its execution.
> >
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> > v2: Keep vchan_get_all_descriptors in the terminate_all call, so the
> > worker doesn't corrupt the next transfer if that's already in
> > the setup process.
> > ---
> > drivers/dma/imx-sdma.c | 42 ++++++++++++++++++++++++++++++++----------
> > 1 file changed, 32 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > 8d2fec8b16cc..da41e8fbf151 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -32,6 +32,7 @@
> > #include <linux/of_address.h>
> > #include <linux/of_device.h>
> > #include <linux/of_dma.h>
> > +#include <linux/workqueue.h>
> >
> > #include <asm/irq.h>
> > #include <linux/platform_data/dma-imx-sdma.h>
> > @@ -375,6 +376,8 @@ struct sdma_channel {
> > > > > > u32 shp_addr, per_addr;
> > > > > > enum dma_status status;
> > > > > > struct imx_dma_data data;
> > > > > > + struct work_struct terminate_worker;
> > > > > > + struct list_head deferred_desc_list;
> > };
> >
> > > > #define IMX_DMA_SG_LOOP BIT(0)
> > @@ -1025,31 +1028,47 @@ static int sdma_disable_channel(struct dma_chan
> > *chan)
> >
> > > > return 0;
> > }
> > +static void sdma_channel_terminate(struct work_struct *work) {
> > > > + struct sdma_channel *sdmac = container_of(work, struct sdma_channel,
> > > > + terminate_worker);
> > +
> > > > + /*
> > > > + * According to NXP R&D team a delay of one BD SDMA cost time
> > > > + * (maximum is 1ms) should be added after disable of the channel
> > > > + * bit, to ensure SDMA core has really been stopped after SDMA
> > > > + * clients call .device_terminate_all.
> > > > + */
> > > > + usleep_range(1000, 2000);
> > +
> > > > + vchan_dma_desc_free_list(&sdmac->vc, &sdmac->deferred_desc_list);
> > > > + INIT_LIST_HEAD(&sdmac->deferred_desc_list);
> > +}
> >
> > static int sdma_disable_channel_with_delay(struct dma_chan *chan) {
> > > > struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > > unsigned long flags;
> > > > - LIST_HEAD(head);
> >
> > > > sdma_disable_channel(chan);
> > +
> > > > spin_lock_irqsave(&sdmac->vc.lock, flags);
> > > > - vchan_get_all_descriptors(&sdmac->vc, &head);
> > > > + vchan_get_all_descriptors(&sdmac->vc, &sdmac->deferred_desc_list);
> > > > sdmac->desc = NULL;
> > > > spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> > > > - vchan_dma_desc_free_list(&sdmac->vc, &head);
> >
> > > > - /*
> > > > - * According to NXP R&D team a delay of one BD SDMA cost time
> > > > - * (maximum is 1ms) should be added after disable of the channel
> > > > - * bit, to ensure SDMA core has really been stopped after SDMA
> > > > - * clients call .device_terminate_all.
> > > > - */
> > > > - mdelay(1);
> > > > + schedule_work(&sdmac->terminate_worker);
> >
> > > > return 0;
> > }
> >
> > +static void sdma_channel_synchronize(struct dma_chan *chan) {
> > > > + struct sdma_channel *sdmac = to_sdma_chan(chan);
> > +
>
> Please add the below to make sure internal virt dma tasklet killed too.
> tasklet_kill(&sdmac->vc.task);
This is not possible. We can not do anything that alters the current
state of the VC in the synchronize call. Otherwise we could be running
into the the same class of issues as the v1 had, with corrupting the
state of the next set up transfer.
Regards,
Lucas
^ permalink raw reply
* dt-bindings: rcar-dmac: Document r8a7744 support
From: Biju Das @ 2018-09-17 15:18 UTC (permalink / raw)
To: Vinod Koul, Rob Herring, Mark Rutland
Cc: Biju Das, dmaengine, devicetree, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Fabrizio Castro, linux-renesas-soc
Renesas RZ/G SoC also have the R-Car gen2/3 compatible DMA controllers.
Document RZ/G1N (also known as R8A7744) SoC bindings.
Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-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 946229c..a5a7c3f 100644
--- a/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt
+++ b/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt
@@ -17,6 +17,7 @@ Required Properties:
- compatible: "renesas,dmac-<soctype>", "renesas,rcar-dmac" as fallback.
Examples with soctypes are:
- "renesas,dmac-r8a7743" (RZ/G1M)
+ - "renesas,dmac-r8a7744" (RZ/G1N)
- "renesas,dmac-r8a7745" (RZ/G1E)
- "renesas,dmac-r8a77470" (RZ/G1C)
- "renesas,dmac-r8a7790" (R-Car H2)
^ permalink raw reply related
* [v5,2/7] dmaengine: xilinx_dma: in axidma slave_sg and dma_cyclic mode align split descriptors
From: Vinod Koul @ 2018-09-18 16:21 UTC (permalink / raw)
To: Andrea Merello
Cc: dan.j.williams, michal.simek, appana.durga.rao, dmaengine,
linux-arm-kernel, linux-kernel, robh+dt, mark.rutland, devicetree,
radhey.shyam.pandey
On 07-09-18, 08:24, Andrea Merello wrote:
> Whenever a single or cyclic transaction is prepared, the driver
> could eventually split it over several SG descriptors in order
> to deal with the HW maximum transfer length.
>
> This could end up in DMA operations starting from a misaligned
> address. This seems fatal for the HW if DRE (Data Realignment Engine)
> is not enabled.
>
> This patch eventually adjusts the transfer size in order to make sure
> all operations start from an aligned address.
>
> Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
> Changes in v2:
> - don't introduce copy_mask field, rather rely on already-esistent
> copy_align field. Suggested by Radhey Shyam Pandey
> - reword title
> Changes in v3:
> - fix bug introduced in v2: wrong copy size when DRE is enabled
> - use implementation suggested by Radhey Shyam Pandey
> Changes in v4:
> - rework on the top of 1/6
> Changes in v5:
> - fix typo in commit title
> - add hint about "DRE" meaning in commit message
> ---
> drivers/dma/xilinx/xilinx_dma.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index a3aaa0e34cc7..aaa6de8a70e4 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -954,15 +954,28 @@ static int xilinx_dma_alloc_chan_resources(struct dma_chan *dchan)
>
> /**
> * xilinx_dma_calc_copysize - Calculate the amount of data to copy
> + * @chan: Driver specific DMA channel
> * @size: Total data that needs to be copied
> * @done: Amount of data that has been already copied
> *
> * Return: Amount of data that has to be copied
> */
> -static int xilinx_dma_calc_copysize(int size, int done)
> +static int xilinx_dma_calc_copysize(struct xilinx_dma_chan *chan,
> + int size, int done)
align to preceeding line opening brace please
> {
> - return min_t(size_t, size - done,
> + size_t copy = min_t(size_t, size - done,
> XILINX_DMA_MAX_TRANS_LEN);
so we can do this way in patch 1:
size t copy;
copy = min_t(size_t, size - done,
XILINX_DMA_MAX_TRANS_LEN);
return copy;
and then add these here, feels like we are redoing change introduced in
patch 1..
> + if ((copy + done < size) &&
> + chan->xdev->common.copy_align) {
> + /*
> + * If this is not the last descriptor, make sure
> + * the next one will be properly aligned
> + */
> + copy = rounddown(copy,
> + (1 << chan->xdev->common.copy_align));
> + }
> + return copy;
> }
>
> /**
> @@ -1804,7 +1817,7 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
> * Calculate the maximum number of bytes to transfer,
> * making sure it is less than the hw limit
> */
> - copy = xilinx_dma_calc_copysize(sg_dma_len(sg),
> + copy = xilinx_dma_calc_copysize(chan, sg_dma_len(sg),
why not keep chan in patch 1 and add only handling in patch 2, seems
less churn to me..
^ permalink raw reply
* [v5,4/7] dmaengine: xilinx_dma: program hardware supported buffer length
From: Vinod Koul @ 2018-09-18 16:25 UTC (permalink / raw)
To: Andrea Merello
Cc: dan.j.williams, michal.simek, appana.durga.rao, dmaengine,
linux-arm-kernel, linux-kernel, robh+dt, mark.rutland, devicetree,
radhey.shyam.pandey
On 07-09-18, 08:24, Andrea Merello wrote:
> From: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
>
> AXI-DMA IP supports configurable (c_sg_length_width) buffer length
> register width, hence read buffer length (xlnx,sg-length-width) DT
> property and ensure that driver doesn't program buffer length
> exceeding the supported limit. For VDMA and CDMA there is no change.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com> [rebase, reword]
> ---
> Changes in v2:
> - drop original patch and replace with the one in Xilinx tree
> Changes in v3:
> - cc DT maintainers/ML
> Changes in v4:
> - upper bound for the property should be 26, not 23
> - add warn for width > 23 as per xilinx original patch
> - rework due to changes introduced in 1/6
> Changes in v5:
> None
> ---
> drivers/dma/xilinx/xilinx_dma.c | 36 +++++++++++++++++++++++++--------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index aaa6de8a70e4..b17f24e4ec35 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -158,7 +158,9 @@
> #define XILINX_DMA_REG_BTT 0x28
>
> /* AXI DMA Specific Masks/Bit fields */
> -#define XILINX_DMA_MAX_TRANS_LEN GENMASK(22, 0)
> +#define XILINX_DMA_MAX_TRANS_LEN_MIN 8
> +#define XILINX_DMA_MAX_TRANS_LEN_MAX 23
> +#define XILINX_DMA_V2_MAX_TRANS_LEN_MAX 26
> #define XILINX_DMA_CR_COALESCE_MAX GENMASK(23, 16)
> #define XILINX_DMA_CR_CYCLIC_BD_EN_MASK BIT(4)
> #define XILINX_DMA_CR_COALESCE_SHIFT 16
> @@ -418,6 +420,7 @@ struct xilinx_dma_config {
> * @rxs_clk: DMA s2mm stream clock
> * @nr_channels: Number of channels DMA device supports
> * @chan_id: DMA channel identifier
> + * @max_buffer_len: Max buffer length
> */
> struct xilinx_dma_device {
> void __iomem *regs;
> @@ -437,6 +440,7 @@ struct xilinx_dma_device {
> struct clk *rxs_clk;
> u32 nr_channels;
> u32 chan_id;
> + u32 max_buffer_len;
> };
>
> /* Macros */
> @@ -964,7 +968,7 @@ static int xilinx_dma_calc_copysize(struct xilinx_dma_chan *chan,
> int size, int done)
> {
> size_t copy = min_t(size_t, size - done,
> - XILINX_DMA_MAX_TRANS_LEN);
> + chan->xdev->max_buffer_len);
hmm why not add max_buffer_len in patch 1 again, and then use default
len as XILINX_DMA_MAX_TRANS_LEN and add multiple lengths here :)
-
~Vinod
^ permalink raw reply
* [2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Vinod Koul @ 2018-09-18 16:35 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
> @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> OWL_DMA_MODE_DAM_INC;
>
> + break;
> + case DMA_MEM_TO_DEV:
> + mode |= OWL_DMA_MODE_TS(vchan->drq)
> + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> +
> + /* Handle bus width for UART */
> + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> + mode |= OWL_DMA_MODE_NDDBW_8BIT;
this is fine per se, but not correct way to handle in dmaengine driver.
You should be agnostic to user of dmaengine, so handle all the buswidths
the IP block supports and update the values accordingly. That way new
uses can be added w/o requiring change in dmaengine driver
^ permalink raw reply
* [-next,v3] dmaengine: mcf-edma: avoid warning for wrong pointer cast
From: Vinod Koul @ 2018-09-18 19:16 UTC (permalink / raw)
To: Angelo Dureghello; +Cc: dmaengine, sfr
On 14-09-18, 21:13, Angelo Dureghello wrote:
> This patch fixes the following compilation warning
> reported during x86_64 allmodconfig build:
>
> drivers/dma/mcf-edma.c: In function 'mcf_edma_filter_fn':
> drivers/dma/mcf-edma.c:296:33: warning: cast from pointer to
> integer of different size [-Wpointer-to-int-cast]
> return (mcf_chan->slave_id == (u32)param);
Applied, thanks
^ permalink raw reply
* driver/dma/ioat: Call del_timer_sync() without holding prep_lock
From: Vinod Koul @ 2018-09-18 19:20 UTC (permalink / raw)
To: Waiman Long
Cc: linux-kernel, dmaengine, Dan Williams, Dave Jiang, Kees Cook,
Christophe JAILLET
On 14-09-18, 14:53, Waiman Long wrote:
> The following lockdep splat was observed:
>
> [ 1222.241750] ======================================================
> [ 1222.271301] WARNING: possible circular locking dependency detected
> [ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
> [ 1222.326659] ------------------------------------------------------
> [ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
> [ 1222.382660] ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
> [ 1222.422928]
> [ 1222.422928] but task is already holding lock:
> [ 1222.451743] (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
> :
> [ 1223.524987] Chain exists of:
> [ 1223.524987] (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
> [ 1223.524987]
> [ 1223.594082] Possible unsafe locking scenario:
> [ 1223.594082]
> [ 1223.622630] CPU0 CPU1
> [ 1223.645080] ---- ----
> [ 1223.667404] lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.691535] lock(&(&ioat_chan->cleanup_lock)->rlock);
> [ 1223.728657] lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.765122] lock((&ioat_chan->timer));
> [ 1223.784095]
> [ 1223.784095] *** DEADLOCK ***
> [ 1223.784095]
> [ 1223.813492] 4 locks held by systemd-shutdow/1:
> [ 1223.834677] #0: (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
> [ 1223.873310] #1: (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
> [ 1223.913604] #2: (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
> [ 1223.954000] #3: (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
>
> In the ioat_shutdown() function:
>
> spin_lock_bh(&ioat_chan->prep_lock);
> set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> del_timer_sync(&ioat_chan->timer);
> spin_unlock_bh(&ioat_chan->prep_lock);
>
> According to the synchronization rule for the del_timer_sync() function,
> the caller must not hold locks which would prevent completion of the
> timer's handler.
>
> The timer structure has its own lock that manages its synchronization.
> Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
> trying to use that device anyway, there is probably no need to call
> del_timer_sync() while holding the prep_lock. So the del_timer_sync()
> call is now moved outside of the prep_lock critical section to prevent
> the circular lock dependency.
Applied, thanks
^ permalink raw reply
* [03/12] dmaengine: coh901318: remove dma_slave_config direction usage
From: Linus Walleij @ 2018-09-18 22:32 UTC (permalink / raw)
To: vkoul; +Cc: dmaengine, Eric Anholt
On Tue, Sep 11, 2018 at 1:36 AM Vinod Koul <vkoul@kernel.org> wrote:
> dma_slave_config direction was marked as deprecated quite some
> time back, remove the usage from this driver so that the field
> can be removed
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* [2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Manivannan Sadhasivam @ 2018-09-18 22:52 UTC (permalink / raw)
To: Vinod
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
On Tue, Sep 18, 2018 at 09:35:12AM -0700, Vinod wrote:
> On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
>
> > @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> > OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> > OWL_DMA_MODE_DAM_INC;
> >
> > + break;
> > + case DMA_MEM_TO_DEV:
> > + mode |= OWL_DMA_MODE_TS(vchan->drq)
> > + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> > + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> > +
> > + /* Handle bus width for UART */
> > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> > + mode |= OWL_DMA_MODE_NDDBW_8BIT;
>
> this is fine per se, but not correct way to handle in dmaengine driver.
> You should be agnostic to user of dmaengine, so handle all the buswidths
> the IP block supports and update the values accordingly. That way new
> uses can be added w/o requiring change in dmaengine driver
>
Hi Vinod,
Currently, all members of Owl family supports only 32bit and 8bit
bus widths. 32bit is common for all peripherals and 8bit applies to only
UART since the internal buffer is 8bit wide. So, this makes sense to me!
Thanks,
Mani
> --
> ~Vinod
^ permalink raw reply
* [2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Vinod Koul @ 2018-09-18 23:32 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
Hi Mani,
On 18-09-18, 15:52, Manivannan Sadhasivam wrote:
> On Tue, Sep 18, 2018 at 09:35:12AM -0700, Vinod wrote:
> > On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
> >
> > > @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> > > OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> > > OWL_DMA_MODE_DAM_INC;
> > >
> > > + break;
> > > + case DMA_MEM_TO_DEV:
> > > + mode |= OWL_DMA_MODE_TS(vchan->drq)
> > > + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> > > + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> > > +
> > > + /* Handle bus width for UART */
> > > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> > > + mode |= OWL_DMA_MODE_NDDBW_8BIT;
> >
> > this is fine per se, but not correct way to handle in dmaengine driver.
> > You should be agnostic to user of dmaengine, so handle all the buswidths
> > the IP block supports and update the values accordingly. That way new
> > uses can be added w/o requiring change in dmaengine driver
>
> Currently, all members of Owl family supports only 32bit and 8bit
> bus widths. 32bit is common for all peripherals and 8bit applies to only
> UART since the internal buffer is 8bit wide. So, this makes sense to me!
Above you are onky handing DMA_SLAVE_BUSWIDTH_1_BYTE and not 32bit which
this IP supports.. You should handle all widths supported vt hardware..
^ permalink raw reply
* [2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Manivannan Sadhasivam @ 2018-09-18 23:34 UTC (permalink / raw)
To: Vinod
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
On Tue, Sep 18, 2018 at 04:32:00PM -0700, Vinod wrote:
> Hi Mani,
>
> On 18-09-18, 15:52, Manivannan Sadhasivam wrote:
> > On Tue, Sep 18, 2018 at 09:35:12AM -0700, Vinod wrote:
> > > On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
> > >
> > > > @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> > > > OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> > > > OWL_DMA_MODE_DAM_INC;
> > > >
> > > > + break;
> > > > + case DMA_MEM_TO_DEV:
> > > > + mode |= OWL_DMA_MODE_TS(vchan->drq)
> > > > + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> > > > + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> > > > +
> > > > + /* Handle bus width for UART */
> > > > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> > > > + mode |= OWL_DMA_MODE_NDDBW_8BIT;
> > >
> > > this is fine per se, but not correct way to handle in dmaengine driver.
> > > You should be agnostic to user of dmaengine, so handle all the buswidths
> > > the IP block supports and update the values accordingly. That way new
> > > uses can be added w/o requiring change in dmaengine driver
> >
> > Currently, all members of Owl family supports only 32bit and 8bit
> > bus widths. 32bit is common for all peripherals and 8bit applies to only
> > UART since the internal buffer is 8bit wide. So, this makes sense to me!
>
> Above you are onky handing DMA_SLAVE_BUSWIDTH_1_BYTE and not 32bit which
> this IP supports.. You should handle all widths supported vt hardware..
>
Hi Vinod,
Default width is 32bit and we will only override it for UART... Should I
add a comment stating this?
Thanks,
Mani
> --
> ~Vinod
^ permalink raw reply
* [2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Manivannan Sadhasivam @ 2018-09-18 23:56 UTC (permalink / raw)
To: Vinod
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
On Tue, Sep 18, 2018 at 04:34:14PM -0700, Manivannan Sadhasivam wrote:
> On Tue, Sep 18, 2018 at 04:32:00PM -0700, Vinod wrote:
> > Hi Mani,
> >
> > On 18-09-18, 15:52, Manivannan Sadhasivam wrote:
> > > On Tue, Sep 18, 2018 at 09:35:12AM -0700, Vinod wrote:
> > > > On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
> > > >
> > > > > @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> > > > > OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> > > > > OWL_DMA_MODE_DAM_INC;
> > > > >
> > > > > + break;
> > > > > + case DMA_MEM_TO_DEV:
> > > > > + mode |= OWL_DMA_MODE_TS(vchan->drq)
> > > > > + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> > > > > + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> > > > > +
> > > > > + /* Handle bus width for UART */
> > > > > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> > > > > + mode |= OWL_DMA_MODE_NDDBW_8BIT;
> > > >
> > > > this is fine per se, but not correct way to handle in dmaengine driver.
> > > > You should be agnostic to user of dmaengine, so handle all the buswidths
> > > > the IP block supports and update the values accordingly. That way new
> > > > uses can be added w/o requiring change in dmaengine driver
> > >
> > > Currently, all members of Owl family supports only 32bit and 8bit
> > > bus widths. 32bit is common for all peripherals and 8bit applies to only
> > > UART since the internal buffer is 8bit wide. So, this makes sense to me!
> >
> > Above you are onky handing DMA_SLAVE_BUSWIDTH_1_BYTE and not 32bit which
> > this IP supports.. You should handle all widths supported vt hardware..
> >
>
> Hi Vinod,
>
> Default width is 32bit and we will only override it for UART... Should I
> add a comment stating this?
>
I think it is better to select 32bit mode eventhough it is the default one.
Will update it in next revision.
Thanks,
Mani
> Thanks,
> Mani
>
> > --
> > ~Vinod
^ permalink raw reply
* [v2,3/4] dmaengine: imx-sdma: implement channel termination via worker
From: Robin Gong @ 2018-09-19 9:20 UTC (permalink / raw)
To: Lucas Stach, Vinod Koul
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
dl-linux-imx, kernel@pengutronix.de, patchwork-lst@pengutronix.de
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2018年9月17日 19:04
> To: Robin Gong <yibin.gong@nxp.com>; Vinod Koul <vkoul@kernel.org>
> Cc: dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de;
> patchwork-lst@pengutronix.de
> Subject: Re: [PATCH v2 3/4] dmaengine: imx-sdma: implement channel
> termination via worker
>
> Hi Robin,
>
> Am Montag, den 17.09.2018, 07:51 +0000 schrieb Robin Gong:
> > > -----Original Message-----
> > > > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2018年9月15日 1:06
> > > > > To: Vinod Koul <vkoul@kernel.org>
> > > > > > > Cc: dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > > > Robin Gong <yibin.gong@nxp.com>; dl-linux-imx
> > > > > > > <linux-imx@nxp.com>;
> > > kernel@pengutronix.de; patchwork-lst@pengutronix.de
> > > Subject: [PATCH v2 3/4] dmaengine: imx-sdma: implement channel
> > > termination via worker
> > >
> > > The dmaengine documentation states that device_terminate_all may be
> > > asynchronous and need not wait for the active transfers have stopped.
> > >
> > > This allows us to move most of the functionality currently
> > > implemented in the sdma channel termination function to run in a
> > > worker, outside of any atomic context. Moving this out of atomic
> > > context has two
> > > benefits: we can now sleep while waiting for the channel to
> > > terminate, instead of busy waiting and the freeing of the dma
> > > descriptors happens with IRQs enabled, getting rid of a warning in the dma
> mapping code.
> > >
> > > As the termination is now asnc, we need to implement the
> > > device_synchronize dma engine function, which simply waits for the worker
> to finish its execution.
> > >
> > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > > v2: Keep vchan_get_all_descriptors in the terminate_all call, so the
> > > worker doesn't corrupt the next transfer if that's already in
> > > the setup process.
> > > ---
> > > drivers/dma/imx-sdma.c | 42
> > > ++++++++++++++++++++++++++++++++----------
> > > 1 file changed, 32 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > > 8d2fec8b16cc..da41e8fbf151 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -32,6 +32,7 @@
> > > #include <linux/of_address.h>
> > > #include <linux/of_device.h>
> > > #include <linux/of_dma.h>
> > > +#include <linux/workqueue.h>
> > >
> > > #include <asm/irq.h>
> > > #include <linux/platform_data/dma-imx-sdma.h>
> > > @@ -375,6 +376,8 @@ struct sdma_channel {
> > > > > > > u32 shp_addr, per_addr;
> > > > > > > enum dma_status status;
> > > > > > > struct imx_dma_data data;
> > > > > > > + struct work_struct terminate_worker;
> > > > > > > + struct list_head deferred_desc_list;
> > > };
> > >
> > > > > #define IMX_DMA_SG_LOOP BIT(0)
> > > @@ -1025,31 +1028,47 @@ static int sdma_disable_channel(struct
> > > dma_chan
> > > *chan)
> > >
> > > > > return 0;
> > > }
> > > +static void sdma_channel_terminate(struct work_struct *work) {
> > > > > + struct sdma_channel *sdmac = container_of(work, struct
> sdma_channel,
> > > > > + terminate_worker);
> > > +
> > > > > + /*
> > > > > + * According to NXP R&D team a delay of one BD SDMA cost time
> > > > > + * (maximum is 1ms) should be added after disable of the channel
> > > > > + * bit, to ensure SDMA core has really been stopped after SDMA
> > > > > + * clients call .device_terminate_all.
> > > > > + */
> > > > > + usleep_range(1000, 2000);
> > > +
> > > > > + vchan_dma_desc_free_list(&sdmac->vc,
> &sdmac->deferred_desc_list);
> > > > > + INIT_LIST_HEAD(&sdmac->deferred_desc_list);
> > > +}
> > >
> > > static int sdma_disable_channel_with_delay(struct dma_chan *chan)
> > > {
> > > > > struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > > > unsigned long flags;
> > > > > - LIST_HEAD(head);
> > >
> > > > > sdma_disable_channel(chan);
> > > +
> > > > > spin_lock_irqsave(&sdmac->vc.lock, flags);
> > > > > - vchan_get_all_descriptors(&sdmac->vc, &head);
> > > > > + vchan_get_all_descriptors(&sdmac->vc,
> > > > > +&sdmac->deferred_desc_list);
> > > > > sdmac->desc = NULL;
> > > > > spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> > > > > - vchan_dma_desc_free_list(&sdmac->vc, &head);
> > >
> > > > > - /*
> > > > > - * According to NXP R&D team a delay of one BD SDMA cost time
> > > > > - * (maximum is 1ms) should be added after disable of the channel
> > > > > - * bit, to ensure SDMA core has really been stopped after SDMA
> > > > > - * clients call .device_terminate_all.
> > > > > - */
> > > > > - mdelay(1);
> > > > > + schedule_work(&sdmac->terminate_worker);
> > >
> > > > > return 0;
> > > }
> > >
> > > +static void sdma_channel_synchronize(struct dma_chan *chan) {
> > > > > + struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > +
> >
> > Please add the below to make sure internal virt dma tasklet killed too.
> > tasklet_kill(&sdmac->vc.task);
>
> This is not possible. We can not do anything that alters the current state of the
> VC in the synchronize call. Otherwise we could be running into the the same
> class of issues as the v1 had, with corrupting the state of the next set up
> transfer.
tasklet_kill never alter the current state, just wait for the tasklet running over.
But here 'vchan_synchronize()' maybe better than 'tasklet_kill' directly.
For the v1 Issue, I investigate it today and found that caused by uart driver call flush_buffer before
TX start and then terminate dma channel directly which is not setup dma channel yet. But in v1,
dma channel terminated in worker instead, thus, when tx dma finish setup transfer and waiting
for the dma done interrupt, the worker scheduled and set 'sdmac->desc=NULL', which cause
dma channel stopped forever once the dma done interrupt coming. Only one line code as below
could fix your V1 issue:
> Regards,
> Lucas
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 2f2e5af..56d542b 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1054,7 +1054,9 @@ static int sdma_disable_channel_with_delay(struct dma_chan *chan)
struct sdma_channel *sdmac = to_sdma_chan(chan);
sdma_disable_channel(chan);
- schedule_work(&sdmac->terminate_worker);
+
+ if (sdmac->desc)
+ schedule_work(&sdmac->terminate_worker);
return 0;
>
^ permalink raw reply related
* dt-bindings: rcar-dmac: Document r8a7744 support
From: Simon Horman @ 2018-09-19 9:38 UTC (permalink / raw)
To: Biju Das
Cc: Vinod Koul, Rob Herring, Mark Rutland, dmaengine, devicetree,
Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
linux-renesas-soc
On Mon, Sep 17, 2018 at 04:18:16PM +0100, Biju Das wrote:
> Renesas RZ/G SoC also have the R-Car gen2/3 compatible DMA controllers.
> Document RZ/G1N (also known as R8A7744) SoC bindings.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> 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 946229c..a5a7c3f 100644
> --- a/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt
> +++ b/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt
> @@ -17,6 +17,7 @@ Required Properties:
> - compatible: "renesas,dmac-<soctype>", "renesas,rcar-dmac" as fallback.
> Examples with soctypes are:
> - "renesas,dmac-r8a7743" (RZ/G1M)
> + - "renesas,dmac-r8a7744" (RZ/G1N)
> - "renesas,dmac-r8a7745" (RZ/G1E)
> - "renesas,dmac-r8a77470" (RZ/G1C)
> - "renesas,dmac-r8a7790" (R-Car H2)
> --
> 2.7.4
>
^ permalink raw reply
* [V3,1/5] dmaengine: dmatest: Add support for multi channel testing
From: Seraj Alijan @ 2018-09-19 19:52 UTC (permalink / raw)
To: vkoul
Cc: dmaengine, dan.j.williams, james.hartley, sifan.naeem, ed.blake,
Seraj Alijan
Add support for running tests on multiple channels simultaneously as the
driver currently limits to 1 channel per test run. This will add support
for stress testing DMA controllers with multi channel capabilities.
This is done by adding a callback function to the "channel" parameter
that registers the requested channel prior to the "run" parameter being
set to 1. Each time the "channel" parameter is populated with a new
dma channel, a new test is appended to the thread queue. Once the "run"
parameter is set to 1, the test will kick start all pending threads.
Signed-off-by: Seraj Alijan <seraj.alijan@sondrel.com>
---
drivers/dma/dmatest.c | 196 +++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 177 insertions(+), 19 deletions(-)
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index aa1712b..ea69033 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -27,11 +27,6 @@ static unsigned int test_buf_size = 16384;
module_param(test_buf_size, uint, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(test_buf_size, "Size of the memcpy test buffer");
-static char test_channel[20];
-module_param_string(channel, test_channel, sizeof(test_channel),
- S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(channel, "Bus ID of the channel to test (default: any)");
-
static char test_device[32];
module_param_string(device, test_device, sizeof(test_device),
S_IRUGO | S_IWUSR);
@@ -139,6 +134,28 @@ static bool dmatest_run;
module_param_cb(run, &run_ops, &dmatest_run, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(run, "Run the test (default: false)");
+static int dmatest_chan_set(const char *val, const struct kernel_param *kp);
+static int dmatest_chan_get(char *val, const struct kernel_param *kp);
+static const struct kernel_param_ops multi_chan_ops = {
+ .set = dmatest_chan_set,
+ .get = dmatest_chan_get,
+};
+
+static char test_channel[20];
+static struct kparam_string newchan_kps = {
+ .string = test_channel,
+ .maxlen = 20,
+};
+module_param_cb(channel, &multi_chan_ops, &newchan_kps, 0644);
+MODULE_PARM_DESC(channel, "Bus ID of the channel to test (default: any)");
+
+static int dmatest_test_list_get(char *val, const struct kernel_param *kp);
+static const struct kernel_param_ops test_list_ops = {
+ .get = dmatest_test_list_get,
+};
+module_param_cb(test_list, &test_list_ops, NULL, 0444);
+MODULE_PARM_DESC(test_list, "Print current test list");
+
/* Maximum amount of mismatched bytes in buffer to print */
#define MAX_ERROR_COUNT 32
@@ -179,6 +196,7 @@ struct dmatest_thread {
wait_queue_head_t done_wait;
struct dmatest_done test_done;
bool done;
+ bool pending;
};
struct dmatest_chan {
@@ -206,6 +224,22 @@ static bool is_threaded_test_run(struct dmatest_info *info)
return false;
}
+static bool is_threaded_test_pending(struct dmatest_info *info)
+{
+ struct dmatest_chan *dtc;
+
+ list_for_each_entry(dtc, &info->channels, node) {
+ struct dmatest_thread *thread;
+
+ list_for_each_entry(thread, &dtc->threads, node) {
+ if (thread->pending)
+ return true;
+ }
+ }
+
+ return false;
+}
+
static int dmatest_wait_get(char *val, const struct kernel_param *kp)
{
struct dmatest_info *info = &test_info;
@@ -476,6 +510,7 @@ static int dmatest_func(void *data)
ret = -ENOMEM;
smp_rmb();
+ thread->pending = false;
info = thread->info;
params = &info->params;
chan = thread->chan;
@@ -886,7 +921,7 @@ static int dmatest_add_threads(struct dmatest_info *info,
/* srcbuf and dstbuf are allocated by the thread itself */
get_task_struct(thread->task);
list_add_tail(&thread->node, &dtc->threads);
- wake_up_process(thread->task);
+ thread->pending = true;
}
return i;
@@ -932,7 +967,7 @@ static int dmatest_add_channel(struct dmatest_info *info,
thread_count += cnt > 0 ? cnt : 0;
}
- pr_info("Started %u threads using %s\n",
+ pr_info("Added %u threads using %s\n",
thread_count, dma_chan_name(chan));
list_add_tail(&dtc->node, &info->channels);
@@ -977,7 +1012,7 @@ static void request_channels(struct dmatest_info *info,
}
}
-static void run_threaded_test(struct dmatest_info *info)
+static void add_threaded_test(struct dmatest_info *info)
{
struct dmatest_params *params = &info->params;
@@ -1000,6 +1035,24 @@ static void run_threaded_test(struct dmatest_info *info)
request_channels(info, DMA_PQ);
}
+static void run_pending_tests(struct dmatest_info *info)
+{
+ struct dmatest_chan *dtc;
+ unsigned int thread_count = 0;
+
+ list_for_each_entry(dtc, &info->channels, node) {
+ struct dmatest_thread *thread;
+
+ thread_count = 0;
+ list_for_each_entry(thread, &dtc->threads, node) {
+ wake_up_process(thread->task);
+ thread_count++;
+ }
+ pr_info("Started %u threads using %s\n",
+ thread_count, dma_chan_name(dtc->chan));
+ }
+}
+
static void stop_threaded_test(struct dmatest_info *info)
{
struct dmatest_chan *dtc, *_dtc;
@@ -1016,7 +1069,7 @@ static void stop_threaded_test(struct dmatest_info *info)
info->nr_channels = 0;
}
-static void restart_threaded_test(struct dmatest_info *info, bool run)
+static void start_threaded_tests(struct dmatest_info *info)
{
/* we might be called early to set run=, defer running until all
* parameters have been evaluated
@@ -1024,11 +1077,7 @@ static void restart_threaded_test(struct dmatest_info *info, bool run)
if (!info->did_init)
return;
- /* Stop any running test first */
- stop_threaded_test(info);
-
- /* Run test with new parameters */
- run_threaded_test(info);
+ run_pending_tests(info);
}
static int dmatest_run_get(char *val, const struct kernel_param *kp)
@@ -1039,7 +1088,8 @@ static int dmatest_run_get(char *val, const struct kernel_param *kp)
if (is_threaded_test_run(info)) {
dmatest_run = true;
} else {
- stop_threaded_test(info);
+ if (!is_threaded_test_pending(info))
+ stop_threaded_test(info);
dmatest_run = false;
}
mutex_unlock(&info->lock);
@@ -1057,18 +1107,125 @@ static int dmatest_run_set(const char *val, const struct kernel_param *kp)
if (ret) {
mutex_unlock(&info->lock);
return ret;
+ } else if (dmatest_run) {
+ if (is_threaded_test_pending(info))
+ start_threaded_tests(info);
+ else
+ pr_info("Could not start test, no channels configured\n");
+ } else {
+ stop_threaded_test(info);
+ }
+
+ mutex_unlock(&info->lock);
+
+ return ret;
+}
+
+static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
+{
+ struct dmatest_info *info = &test_info;
+ struct dmatest_chan *dtc;
+ char chan_reset_val[20];
+ int ret = 0;
+
+ mutex_lock(&info->lock);
+ ret = param_set_copystring(val, kp);
+ if (ret) {
+ mutex_unlock(&info->lock);
+ return ret;
+ }
+ /*Clear any previously run threads */
+ if (!is_threaded_test_run(info) && !is_threaded_test_pending(info))
+ stop_threaded_test(info);
+ /* Reject channels that are already registered */
+ if (is_threaded_test_pending(info)) {
+ list_for_each_entry(dtc, &info->channels, node) {
+ if (strcmp(dma_chan_name(dtc->chan),
+ strim(test_channel)) == 0) {
+ dtc = list_last_entry(&info->channels,
+ struct dmatest_chan,
+ node);
+ strlcpy(chan_reset_val,
+ dma_chan_name(dtc->chan),
+ sizeof(chan_reset_val));
+ ret = -EBUSY;
+ goto add_chan_err;
+ }
+ }
}
- if (is_threaded_test_run(info))
+ add_threaded_test(info);
+
+ /* Check if channel was added successfully */
+ dtc = list_last_entry(&info->channels, struct dmatest_chan, node);
+
+ if (dtc->chan) {
+ /*
+ * if new channel was not successfully added, revert the the
+ * "test_channel" string to the name of the last successfully
+ * added channel. exception for when users issues empty string
+ * to channel parameter.
+ */
+ if ((strcmp(dma_chan_name(dtc->chan), strim(test_channel)) != 0)
+ && (strcmp("", strim(test_channel)) != 0)) {
+ ret = -EINVAL;
+ strlcpy(chan_reset_val, dma_chan_name(dtc->chan),
+ sizeof(chan_reset_val));
+ goto add_chan_err;
+ }
+
+ } else {
+ /* Clear test_channel if no channels were added successfully */
+ strlcpy(chan_reset_val, "", sizeof(chan_reset_val));
ret = -EBUSY;
- else if (dmatest_run)
- restart_threaded_test(info, dmatest_run);
+ goto add_chan_err;
+ }
+
+ mutex_unlock(&info->lock);
+
+ return ret;
+add_chan_err:
+ param_set_copystring(chan_reset_val, kp);
mutex_unlock(&info->lock);
return ret;
}
+static int dmatest_chan_get(char *val, const struct kernel_param *kp)
+{
+ struct dmatest_info *info = &test_info;
+
+ mutex_lock(&info->lock);
+ if (!is_threaded_test_run(info) && !is_threaded_test_pending(info)) {
+ stop_threaded_test(info);
+ strlcpy(test_channel, "", sizeof(test_channel));
+ }
+ mutex_unlock(&info->lock);
+
+ return param_get_string(val, kp);
+}
+
+static int dmatest_test_list_get(char *val, const struct kernel_param *kp)
+{
+ struct dmatest_info *info = &test_info;
+ struct dmatest_chan *dtc;
+ unsigned int thread_count = 0;
+
+ list_for_each_entry(dtc, &info->channels, node) {
+ struct dmatest_thread *thread;
+
+ thread_count = 0;
+ list_for_each_entry(thread, &dtc->threads, node) {
+ thread_count++;
+ }
+ pr_info("%u threads using %s\n",
+ thread_count, dma_chan_name(dtc->chan));
+ }
+
+ return 0;
+}
+
static int __init dmatest_init(void)
{
struct dmatest_info *info = &test_info;
@@ -1076,7 +1233,8 @@ static int __init dmatest_init(void)
if (dmatest_run) {
mutex_lock(&info->lock);
- run_threaded_test(info);
+ add_threaded_test(info);
+ run_pending_tests(info);
mutex_unlock(&info->lock);
}
^ permalink raw reply related
* [V3,2/5] dmaengine: dmatest: Use fixed point div to calculate iops
From: Seraj Alijan @ 2018-09-19 19:52 UTC (permalink / raw)
To: vkoul
Cc: dmaengine, dan.j.williams, james.hartley, sifan.naeem, ed.blake,
Seraj Alijan
Use fixed point division to calculate iops to prevent reporting 0 iops
when operations last for longer than a second.
Signed-off-by: Seraj Alijan <seraj.alijan@sondrel.com>
---
drivers/dma/dmatest.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index ea69033..22f0e4eb 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -177,6 +177,13 @@ MODULE_PARM_DESC(test_list, "Print current test list");
#define PATTERN_COUNT_MASK 0x1f
#define PATTERN_MEMSET_IDX 0x01
+/* Fixed point arithmetic ops */
+#define FIXPT_SHIFT 8
+#define FIXPNT_MASK 0xFF
+#define FIXPT_TO_INT(a) ((a) >> FIXPT_SHIFT)
+#define INT_TO_FIXPT(a) ((a) << FIXPT_SHIFT)
+#define FIXPT_GET_FRAC(a) ((((a) & FIXPNT_MASK) * 100) >> FIXPT_SHIFT)
+
/* poor man's completion - we want to use wait_event_freezable() on it */
struct dmatest_done {
bool done;
@@ -453,13 +460,15 @@ static unsigned long long dmatest_persec(s64 runtime, unsigned int val)
}
per_sec *= val;
+ per_sec = INT_TO_FIXPT(per_sec);
do_div(per_sec, runtime);
+
return per_sec;
}
static unsigned long long dmatest_KBs(s64 runtime, unsigned long long len)
{
- return dmatest_persec(runtime, len >> 10);
+ return FIXPT_TO_INT(dmatest_persec(runtime, len >> 10));
}
/*
@@ -500,6 +509,7 @@ static int dmatest_func(void *data)
ktime_t comparetime = 0;
s64 runtime = 0;
unsigned long long total_len = 0;
+ unsigned long long iops = 0;
u8 align = 0;
bool is_memset = false;
dma_addr_t *srcs;
@@ -840,9 +850,10 @@ static int dmatest_func(void *data)
err_srcs:
kfree(pq_coefs);
err_thread_type:
- pr_info("%s: summary %u tests, %u failures %llu iops %llu KB/s (%d)\n",
+ iops = dmatest_persec(runtime, total_tests);
+ pr_info("%s: summary %u tests, %u failures %llu.%02llu iops %llu KB/s (%d)\n",
current->comm, total_tests, failed_tests,
- dmatest_persec(runtime, total_tests),
+ FIXPT_TO_INT(iops), FIXPT_GET_FRAC(iops),
dmatest_KBs(runtime, total_len), ret);
/* terminate all transfers on specified channels */
^ permalink raw reply related
* [V3,3/5] dmaengine: dmatest: Add alignment parameter
From: Seraj Alijan @ 2018-09-19 19:52 UTC (permalink / raw)
To: vkoul
Cc: dmaengine, dan.j.williams, james.hartley, sifan.naeem, ed.blake,
Seraj Alijan
Add parameter "alignment" to allow setting the address alignment
manually. Having the ability to configure address alignment from
user space adds new testing capabilities where different alignments can
be configured for testing without having to modify the dma device
alignment properties.
If configured, the alignment value will override the device alignment
property of the target device.
Signed-off-by: Seraj Alijan <seraj.alijan@sondrel.com>
---
drivers/dma/dmatest.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 22f0e4eb..300fe4c 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -79,6 +79,10 @@ static bool verbose;
module_param(verbose, bool, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(verbose, "Enable \"success\" result messages (default: off)");
+static int alignment = -1;
+module_param(alignment, int, 0644);
+MODULE_PARM_DESC(alignment, "Custom data address alignment taken as 2^(alignment) (default: not used (-1))");
+
/**
* struct dmatest_params - test parameters.
* @buf_size: size of the memcpy test buffer
@@ -103,6 +107,7 @@ struct dmatest_params {
int timeout;
bool noverify;
bool norandom;
+ int alignment;
};
/**
@@ -526,22 +531,26 @@ static int dmatest_func(void *data)
chan = thread->chan;
dev = chan->device;
if (thread->type == DMA_MEMCPY) {
- align = dev->copy_align;
+ align = params->alignment < 0 ? dev->copy_align :
+ params->alignment;
src_cnt = dst_cnt = 1;
} else if (thread->type == DMA_MEMSET) {
- align = dev->fill_align;
+ align = params->alignment < 0 ? dev->fill_align :
+ params->alignment;
src_cnt = dst_cnt = 1;
is_memset = true;
} else if (thread->type == DMA_XOR) {
/* force odd to ensure dst = src */
src_cnt = min_odd(params->xor_sources | 1, dev->max_xor);
dst_cnt = 1;
- align = dev->xor_align;
+ align = params->alignment < 0 ? dev->xor_align :
+ params->alignment;
} else if (thread->type == DMA_PQ) {
/* force odd to ensure dst = src */
src_cnt = min_odd(params->pq_sources | 1, dma_maxpq(dev, 0));
dst_cnt = 2;
- align = dev->pq_align;
+ align = params->alignment < 0 ? dev->pq_align :
+ params->alignment;
pq_coefs = kmalloc(params->pq_sources + 1, GFP_KERNEL);
if (!pq_coefs)
@@ -1039,6 +1048,7 @@ static void add_threaded_test(struct dmatest_info *info)
params->timeout = timeout;
params->noverify = noverify;
params->norandom = norandom;
+ params->alignment = alignment;
request_channels(info, DMA_MEMCPY);
request_channels(info, DMA_MEMSET);
^ permalink raw reply related
* [V3,4/5] dmaengine: dmatest: Add transfer_size parameter
From: Seraj Alijan @ 2018-09-19 19:52 UTC (permalink / raw)
To: vkoul
Cc: dmaengine, dan.j.williams, james.hartley, sifan.naeem, ed.blake,
Seraj Alijan
Existing transfer size "len" is either generated randomly or set to the
size of test_buf_size. In some cases we need to explicitly specify a
transfer size that is different from the buffer size and non aligned to
test the target device's ability to handle unaligned transfers.
This patch adds optional parameter "transfer_size" to allow setting
explicit transfer size for dma transfers.
Signed-off-by: Seraj Alijan <seraj.alijan@sondrel.com>
---
drivers/dma/dmatest.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 300fe4c..b6e613b 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -83,6 +83,10 @@ static int alignment = -1;
module_param(alignment, int, 0644);
MODULE_PARM_DESC(alignment, "Custom data address alignment taken as 2^(alignment) (default: not used (-1))");
+static unsigned int transfer_size;
+module_param(transfer_size, uint, 0644);
+MODULE_PARM_DESC(transfer_size, "Optional custom transfer size in bytes (default: not used (0))");
+
/**
* struct dmatest_params - test parameters.
* @buf_size: size of the memcpy test buffer
@@ -108,6 +112,7 @@ struct dmatest_params {
bool noverify;
bool norandom;
int alignment;
+ unsigned int transfer_size;
};
/**
@@ -643,15 +648,25 @@ static int dmatest_func(void *data)
break;
}
- if (params->norandom)
+ if (params->transfer_size) {
+ if (params->transfer_size >= params->buf_size) {
+ pr_err("%u-byte transfer size must be lower than %u-buffer size\n",
+ params->transfer_size, params->buf_size);
+ break;
+ }
+ len = params->transfer_size;
+ } else if (params->norandom) {
len = params->buf_size;
- else
+ } else {
len = dmatest_random() % params->buf_size + 1;
+ }
- len = (len >> align) << align;
- if (!len)
- len = 1 << align;
-
+ /* Do not alter transfer size explicitly defined by user */
+ if (!params->transfer_size) {
+ len = (len >> align) << align;
+ if (!len)
+ len = 1 << align;
+ }
total_len += len;
if (params->norandom) {
@@ -1049,6 +1064,7 @@ static void add_threaded_test(struct dmatest_info *info)
params->noverify = noverify;
params->norandom = norandom;
params->alignment = alignment;
+ params->transfer_size = transfer_size;
request_channels(info, DMA_MEMCPY);
request_channels(info, DMA_MEMSET);
^ permalink raw reply related
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