* [01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels
From: Paul Cercueil @ 2018-07-07 11:01 UTC (permalink / raw)
To: PrasannaKumar Muralidharan
Cc: Vinod Koul, Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton,
James Hogan, Zubair Lutfullah Kakakhel, Mathieu Malaterre,
Daniel Silsby, dmaengine,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Linux-MIPS
Le sam. 7 juil. 2018 à 9:34, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> a écrit :
> On 5 July 2018 at 23:56, Paul Cercueil <paul@crapouillou.net> wrote:
>> Hi PrasannaKumar,
>>
>>
>>> Hi Paul,
>>>
>>> On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net>
>>> wrote:
>>>>
>>>> As part of the work to support various other Ingenic JZ47xx SoC
>>>> versions,
>>>> which don't feature the same number of DMA channels per core, we
>>>> now
>>>> deduce the number of DMA channels available from the devicetree
>>>> compatible string.
>>>>
>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>> ---
>>>> drivers/dma/dma-jz4780.c | 53
>>>> +++++++++++++++++++++++++++++-----------
>>>> 1 file changed, 39 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
>>>> index 85820a2d69d4..b40f491f0367 100644
>>>> --- a/drivers/dma/dma-jz4780.c
>>>> +++ b/drivers/dma/dma-jz4780.c
>>>> @@ -16,6 +16,7 @@
>>>> #include <linux/interrupt.h>
>>>> #include <linux/module.h>
>>>> #include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> #include <linux/of_dma.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/slab.h>
>>>> @@ -23,8 +24,6 @@
>>>> #include "dmaengine.h"
>>>> #include "virt-dma.h"
>>>>
>>>> -#define JZ_DMA_NR_CHANNELS 32
>>>> -
>>>> /* Global registers. */
>>>> #define JZ_DMA_REG_DMAC 0x1000
>>>> #define JZ_DMA_REG_DIRQP 0x1004
>>>> @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
>>>> unsigned int curr_hwdesc;
>>>> };
>>>>
>>>> +enum jz_version {
>>>> + ID_JZ4780,
>>>> +};
>>>> +
>>>> struct jz4780_dma_dev {
>>>> struct dma_device dma_device;
>>>> void __iomem *base;
>>>> struct clk *clk;
>>>> unsigned int irq;
>>>> + unsigned int nb_channels;
>>>> + enum jz_version version;
>>>>
>>>> uint32_t chan_reserved;
>>>> - struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
>>>> + struct jz4780_dma_chan chan[];
>>>
>>>
>>> Looks like a variable length array in struct. I think there is some
>>> effort to remove the usage of VLA. Can you revisit this? I may be
>>> wrong, please feel free to correct.
>>
>>
>> Are you sure? It's the first time I hear about it.
>> Could anybody confirm?
>
> Please see [1] for info.
>
> Variable Length Arrays in struct is expressly forbidden in C99, C11.
> Clang does not support it. To make kernel compile with Clang few
> people are trying to remove/reduce VLAIS usage.
>
> 1.
> https://blog.linuxplumbersconf.org/2013/ocw/system/presentations/1221/original/VLAIS.pdf
I read it, and my structure is not a VLAIS; my "chan" array is a
flexible
array, its sizeof() is 0, so the sizeof() of the structure is constant.
See page 6 of the PDF, about alternatives to VLAIS:
"If possible use a flexible array member and move the array to the end
of
the struct"
Which is what I am doing here.
-Paul
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* dma: stm32: replace "%p" with "%pK"
From: Benjamin Gaignard @ 2018-07-07 11:28 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: vkoul, Dan Williams, Maxime Coquelin, Alexandre Torgue, dmaengine,
Linux Kernel Mailing List, Linux ARM, Benjamin Gaignard
2018-07-06 21:38 GMT+02:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> Hi Benjamin,
>
> On Fri, Jul 6, 2018 at 3:03 PM Benjamin Gaignard
> <benjamin.gaignard@linaro.org> wrote:
>> The format specifier "%p" can leak kernel addresses.
>> Use "%pK" instead.
>
> Still? Isn't the value randomized these days?
%p give an hashed value, with %pK you can use a sysctl to get the real
value if you are root
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> 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
^ permalink raw reply
* [v2,1/4] dt-bindings: k3dma: add optional property hisilicon,dma-min-chan
From: Vinod Koul @ 2018-07-09 10:34 UTC (permalink / raw)
To: Guodong Xu
Cc: robh+dt, mark.rutland, dan.j.williams, xuwei5, catalin.marinas,
will.deacon, liyu65, suzhuangluan, xuhongtao8, zhongkaihua,
xuezhiliang, xupeng7, sunliang10, fengbaopeng, dmaengine,
devicetree, linux-kernel, linux-arm-kernel
On 06-07-18, 11:55, Guodong Xu wrote:
> From: Li Yu <liyu65@hisilicon.com>
>
> Add optional property hisilicon,dma-min-chan for k3dma.
>
> Signed-off-by: Li Yu <liyu65@hisilicon.com>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
> Documentation/devicetree/bindings/dma/k3dma.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> index 4945aeac4dc4..f34202a80f3c 100644
> --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> @@ -12,6 +12,11 @@ Required properties:
> have specific request line
> - clocks: clock required
>
> +Optional properties:
> +- hisilicon,dma-min-chan: the minimum DMA channel number which is usable
> + Default value is 0, but in some platform it is
> + configured 1, like in hi3660 platform
> +
> Example:
>
> Controller:
> @@ -21,6 +26,7 @@ Controller:
> #dma-cells = <1>;
> dma-channels = <16>;
> dma-requests = <27>;
> + hisilicon,dma-min-chan = <1>;
Am still expecting this to be a mask
^ permalink raw reply
* dma: add support for reporting pause and resume separately
From: Mark Brown @ 2018-07-09 11:17 UTC (permalink / raw)
To: Vinod
Cc: Marek Szyprowski, Lars-Peter Clausen, dmaengine, alsa-devel,
linux-kernel, Dan Williams, Bartlomiej Zolnierkiewicz
On Wed, Jul 04, 2018 at 12:30:33PM +0530, Vinod wrote:
> > Existing clients of dma_get_slave_caps() have been checked and the only
> > driver which rely on proper channel resuming is soc-generic-dmaengine-pcm
> > driver, which has been updated to check the newly added capability.
> > Existing 'cmd_pause' now only indicates that DMA engine support pausing
> > given DMA channel.
> The change looks fine to me. I was hoping that serial would also check
> this..
> Mark, Lars you okay with this?
Acked-by: Mark Brown <broonie@kernel.org>
^ permalink raw reply
* dmaengine: nbpfaxi: Mark expected switch fall-through
From: Vinod Koul @ 2018-07-09 11:32 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Dan Williams, dmaengine, linux-kernel
On 06-07-18, 06:47, Gustavo A. R. Silva wrote:
> Hi Vinod,
>
> On 07/06/2018 12:56 AM, Vinod wrote:
> > On 02-07-18, 13:06, Gustavo A. R. Silva wrote:
> >> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> >> where we are expecting to fall through.
> >>
> >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >> ---
> >> drivers/dma/nbpfaxi.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/dma/nbpfaxi.c b/drivers/dma/nbpfaxi.c
> >> index 2f9974d..8c7b2e8 100644
> >> --- a/drivers/dma/nbpfaxi.c
> >> +++ b/drivers/dma/nbpfaxi.c
> >> @@ -479,6 +479,7 @@ static size_t nbpf_xfer_size(struct nbpf_device *nbpf,
> >>
> >> default:
> >> pr_warn("%s(): invalid bus width %u\n", __func__, width);
> >> + /* fall through */
> >
> > Hmm this looks okay but am not able to trigger this warning..(used W=1) Did you
> > see this warning on your build, if so what options?
>
> Add this to your Makefile:
>
> KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=2)
Thanks, that did it..
Applied now thanks
^ permalink raw reply
* dmaengine: pl330: Mark expected switch fall-through
From: Vinod Koul @ 2018-07-09 11:45 UTC (permalink / raw)
To: dmaengine; +Cc: krzk, Vinod Koul
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/dma/pl330.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index defcdde4d358..04fc4d8da0e9 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1046,13 +1046,16 @@ static bool _start(struct pl330_thread *thrd)
if (_state(thrd) == PL330_STATE_KILLING)
UNTIL(thrd, PL330_STATE_STOPPED)
+ /* fall through */
case PL330_STATE_FAULTING:
_stop(thrd);
+ /* fall through */
case PL330_STATE_KILLING:
case PL330_STATE_COMPLETING:
UNTIL(thrd, PL330_STATE_STOPPED)
+ /* fall through */
case PL330_STATE_STOPPED:
return _trigger(thrd);
^ permalink raw reply related
* dmaengine: pl330: Mark expected switch fall-through
From: Krzysztof Kozlowski @ 2018-07-09 12:37 UTC (permalink / raw)
To: Vinod Koul; +Cc: dmaengine
On 9 July 2018 at 13:45, Vinod Koul <vkoul@kernel.org> wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/dma/pl330.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index defcdde4d358..04fc4d8da0e9 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -1046,13 +1046,16 @@ static bool _start(struct pl330_thread *thrd)
>
> if (_state(thrd) == PL330_STATE_KILLING)
> UNTIL(thrd, PL330_STATE_STOPPED)
> + /* fall through */
>
> case PL330_STATE_FAULTING:
> _stop(thrd);
> + /* fall through */
>
> case PL330_STATE_KILLING:
> case PL330_STATE_COMPLETING:
> UNTIL(thrd, PL330_STATE_STOPPED)
> + /* fall through */
>
> case PL330_STATE_STOPPED:
> return _trigger(thrd);
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* dmaengine: pl330: remove set but unused variable
From: Vinod Koul @ 2018-07-09 14:43 UTC (permalink / raw)
To: dmaengine; +Cc: krzk, Vinod Koul
Compiler complains (with W=1):
drivers/dma/pl330.c: In function ‘pl330_release_channel’:
drivers/dma/pl330.c:1782:21: warning:
variable ‘pl330’ set but not used [-Wunused-but-set-variable]
struct pl330_dmac *pl330;
^~~~~
Remove the pl330 variable in pl330_release_channel as it is set but
never used.
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/dma/pl330.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 04fc4d8da0e9..451370da909d 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1782,8 +1782,6 @@ static inline void _free_event(struct pl330_thread *thrd, int ev)
static void pl330_release_channel(struct pl330_thread *thrd)
{
- struct pl330_dmac *pl330;
-
if (!thrd || thrd->free)
return;
@@ -1792,8 +1790,6 @@ static void pl330_release_channel(struct pl330_thread *thrd)
dma_pl330_rqcb(thrd->req[1 - thrd->lstenq].desc, PL330_ERR_ABORT);
dma_pl330_rqcb(thrd->req[thrd->lstenq].desc, PL330_ERR_ABORT);
- pl330 = thrd->dmac;
-
_free_event(thrd, thrd->ev);
thrd->free = true;
}
^ permalink raw reply related
* dmaengine: rcar-dmac: Disable interrupts while stopping channels
From: Vinod Koul @ 2018-07-09 16:51 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Dan Williams, Kuninori Morimoto, Yoshihiro Shimoda, dmaengine,
linux-renesas-soc
On 02-07-18, 17:02, Geert Uytterhoeven wrote:
> During system reboot or halt, with lockdep enabled:
>
> ================================
> WARNING: inconsistent lock state
> 4.18.0-rc1-salvator-x-00002-g9203dbec90a68103 #41 Tainted: G W
> --------------------------------
> inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
> reboot/2779 [HC0[0]:SC0[0]:HE1:SE1] takes:
> 0000000098ae4ad3 (&(&rchan->lock)->rlock){?.-.}, at: rcar_dmac_shutdown+0x58/0x6c
> {IN-HARDIRQ-W} state was registered at:
> lock_acquire+0x208/0x238
> _raw_spin_lock+0x40/0x54
> rcar_dmac_isr_channel+0x28/0x200
> __handle_irq_event_percpu+0x1c0/0x3c8
> handle_irq_event_percpu+0x34/0x88
> handle_irq_event+0x48/0x78
> handle_fasteoi_irq+0xc4/0x12c
> generic_handle_irq+0x18/0x2c
> __handle_domain_irq+0xa8/0xac
> gic_handle_irq+0x78/0xbc
> el1_irq+0xec/0x1c0
> arch_cpu_idle+0xe8/0x1bc
> default_idle_call+0x2c/0x30
> do_idle+0x144/0x234
> cpu_startup_entry+0x20/0x24
> rest_init+0x27c/0x290
> start_kernel+0x430/0x45c
> irq event stamp: 12177
> hardirqs last enabled at (12177): [<ffffff800881d804>] _raw_spin_unlock_irq+0x2c/0x4c
> hardirqs last disabled at (12176): [<ffffff800881d638>] _raw_spin_lock_irq+0x1c/0x60
> softirqs last enabled at (11948): [<ffffff8008081da8>] __do_softirq+0x160/0x4ec
> softirqs last disabled at (11935): [<ffffff80080ec948>] irq_exit+0xa0/0xfc
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&(&rchan->lock)->rlock);
> <Interrupt>
> lock(&(&rchan->lock)->rlock);
>
> *** DEADLOCK ***
>
> 3 locks held by reboot/2779:
> #0: 00000000bfabfa74 (reboot_mutex){+.+.}, at: sys_reboot+0xdc/0x208
> #1: 00000000c75d8c3a (&dev->mutex){....}, at: device_shutdown+0xc8/0x1c4
> #2: 00000000ebec58ec (&dev->mutex){....}, at: device_shutdown+0xd8/0x1c4
>
> stack backtrace:
> CPU: 6 PID: 2779 Comm: reboot Tainted: G W 4.18.0-rc1-salvator-x-00002-g9203dbec90a68103 #41
> Hardware name: Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+ (DT)
> Call trace:
> dump_backtrace+0x0/0x148
> show_stack+0x14/0x1c
> dump_stack+0xb0/0xf0
> print_usage_bug.part.26+0x1c4/0x27c
> mark_lock+0x38c/0x610
> __lock_acquire+0x3fc/0x14d4
> lock_acquire+0x208/0x238
> _raw_spin_lock+0x40/0x54
> rcar_dmac_shutdown+0x58/0x6c
> platform_drv_shutdown+0x20/0x2c
> device_shutdown+0x160/0x1c4
> kernel_restart_prepare+0x34/0x3c
> kernel_restart+0x14/0x5c
> sys_reboot+0x160/0x208
> el0_svc_naked+0x30/0x34
>
> rcar_dmac_stop_all_chan() takes the channel lock while stopping a
> channel, but does not disable interrupts, leading to a deadlock when a
> DMAC interrupt comes in. Before, the same code block was called from an
> interrupt handler, hence taking the spinlock was sufficient.
>
> Fix this by disabling local interrupts while taking the spinlock.
Applied, thanks
^ permalink raw reply
* [v3] dmaengine: rcar-dmac: clear channel register when error
From: Vinod Koul @ 2018-07-09 16:52 UTC (permalink / raw)
To: Kuninori Morimoto
Cc: Geert Uytterhoeven, Dan Williams, Magnus Damm, Linux-Renesas,
Laurent Pinchart, Kieran Bingham, Geert Uytterhoeven, dmaengine,
Hiroki Negishi
On 03-07-18, 00:29, Kuninori Morimoto wrote:
>
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> We need to clear channel register in error case as recovery.
> The channel is already stopped in such case, thus we don't need to call
> rcar_dmac_chan_halt() before clearing.
>
> rcar_dmac_chan_halt() will clear and confirm DE bit.
> But it will be failed because channel is already stopped in error case.
> In other words, we shouldn't call it then.
Applied, thanks
^ permalink raw reply
* [01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels
From: Vinod Koul @ 2018-07-09 16:59 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
On 03-07-18, 14:32, Paul Cercueil wrote:
> struct jz4780_dma_dev {
> struct dma_device dma_device;
> void __iomem *base;
> struct clk *clk;
> unsigned int irq;
> + unsigned int nb_channels;
> + enum jz_version version;
>
> uint32_t chan_reserved;
> - struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
> + struct jz4780_dma_chan chan[];
why array, why not channel pointer?
> +static const unsigned int jz4780_dma_nb_channels[] = {
> + [ID_JZ4780] = 32,
> +};
> +
> +static const struct of_device_id jz4780_dma_dt_match[] = {
> + { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> + {},
> +};
Looking at description I was hoping that channels would be in DT,
channels is hardware information, so should come from DT rather than
coding the kernel...
> - jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
> + if (of_id)
> + version = (enum jz_version)of_id->data;
> + else
> + version = ID_JZ4780; /* Default when not probed from DT */
where else would it be probed from.... ?
^ permalink raw reply
* [02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers
From: Vinod Koul @ 2018-07-09 17:03 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
On 03-07-18, 14:32, Paul Cercueil wrote:
> The register area of the JZ4780 DMA core can be split into different
> sections for different purposes:
>
> * one set of registers is used to perform actions at the DMA core level,
> that will generally affect all channels;
>
> * one set of registers per DMA channel, to perform actions at the DMA
> channel level, that will only affect the channel in question.
>
> The problem rises when trying to support new versions of the JZ47xx
> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
> with six DMA channels, and the register sets are interleaved:
> <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs>
>
> By using one memory resource for the channel-specific registers and
> one memory resource for the core-specific registers, we can support
> the JZ4770, by initializing the driver once per DMA core with different
> addresses.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> .../devicetree/bindings/dma/jz4780-dma.txt | 6 +-
Pls move to separate patch.
> drivers/dma/dma-jz4780.c | 106 +++++++++++-------
> 2 files changed, 69 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index f25feee62b15..f9b1864f5b77 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -3,7 +3,8 @@
> Required properties:
>
> - compatible: Should be "ingenic,jz4780-dma"
> -- reg: Should contain the DMA controller registers location and length.
> +- reg: Should contain the DMA channel registers location and length, followed
> + by the DMA controller registers location and length.
> - interrupts: Should contain the interrupt specifier of the DMA controller.
> - interrupt-parent: Should be the phandle of the interrupt controller that
> - clocks: Should contain a clock specifier for the JZ4780 PDMA clock.
> @@ -22,7 +23,8 @@ Example:
>
> dma: dma@13420000 {
> compatible = "ingenic,jz4780-dma";
> - reg = <0x13420000 0x10000>;
> + reg = <0x13420000 0x400
> + 0x13421000 0x40>;
Second should be optional or we break platform which may not have
updated DT..
> - jzdma->base = devm_ioremap_resource(dev, res);
> - if (IS_ERR(jzdma->base))
> - return PTR_ERR(jzdma->base);
> + jzdma->chn_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(jzdma->chn_base))
> + return PTR_ERR(jzdma->chn_base);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res) {
> + dev_err(dev, "failed to get I/O memory\n");
> + return -EINVAL;
> + }
okay and this breaks if you happen to get probed on older DT. I think DT
is treated as ABI so you need to continue support older method while
finding if DT has split resources
^ permalink raw reply
* [04/14] dmaengine: dma-jz4780: Add support for the JZ4770 SoC
From: Vinod Koul @ 2018-07-09 17:10 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
On 03-07-18, 14:32, Paul Cercueil wrote:
> +static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
> + unsigned int chn)
> +{
> + if (jzdma->version == ID_JZ4770)
> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));
> +}
this sounds as hardware behaviour, so why not describe as a property in
DT?
> +
> static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
> struct jz4780_dma_chan *jzchan, unsigned int count,
> enum dma_transaction_type type)
> @@ -228,8 +246,15 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
> kfree(desc);
> }
>
> -static uint32_t jz4780_dma_transfer_size(unsigned long val, uint32_t *shift)
> +static const unsigned int jz4780_dma_ord_max[] = {
> + [ID_JZ4770] = 6,
> + [ID_JZ4780] = 7,
> +};
So this gives the transfer length supported?
^ permalink raw reply
* [05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC
From: Vinod Koul @ 2018-07-09 17:12 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
On 03-07-18, 14:32, Paul Cercueil wrote:
> enum jz_version {
> + ID_JZ4740,
> ID_JZ4770,
> ID_JZ4780,
> };
> @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
> }
>
> static const unsigned int jz4780_dma_ord_max[] = {
> + [ID_JZ4740] = 5,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 7,
> };
> @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
> }
>
> static const unsigned int jz4780_dma_nb_channels[] = {
> + [ID_JZ4740] = 6,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 32,
> };
I feel these should be done away with if we describe hardware in DT
>
> static const struct of_device_id jz4780_dma_dt_match[] = {
> + { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
adding .compatible should be the only thing required, if at all for this
addition :)
^ permalink raw reply
* [06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC
From: Vinod Koul @ 2018-07-09 17:14 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
On 03-07-18, 14:32, Paul Cercueil wrote:
> The JZ4725B has one DMA core starring six DMA channels.
> As for the JZ4770, each DMA channel's clock can be enabled with
> a register write, the difference here being that once started, it
> is not possible to turn it off.
ok so disable for this, right..
> @@ -204,6 +205,8 @@ static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
> {
> if (jzdma->version == ID_JZ4770)
> jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> + else if (jzdma->version == ID_JZ4725B)
> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
but you are writing to a different register here..
^ permalink raw reply
* dma: sh: rcar-dmac: avoid to write CHCR.TE to 1 if TCR is set to 0
From: Vinod Koul @ 2018-07-09 17:23 UTC (permalink / raw)
To: Geert Uytterhoeven, Kuninori Morimoto
Cc: Yoshihiro Shimoda, dmaengine, linux-renesas-soc
On 02-07-18, 18:18, Yoshihiro Shimoda wrote:
> This patch fixes an issue that unexpected retransfering happens
> if TCR is set to 0 before rcar_dmac_sync_tcr() writes DE bit to
> the CHCR register. For example, sh-sci driver can reproduce this
> issue like below:
>
> In rx_timer_fn(): /* CHCR DE bit may be set to 1 */
> dmaengine_tx_status()
> rcar_dmac_tx_status()
> rcar_dmac_chan_get_residue()
> rcar_dmac_sync_tcr() /* TCR is possible to be set to 0 */
>
> According to the description of commit 73a47bd0da66 ("dmaengine:
> rcar-dmac: use TCRB instead of TCR for residue"), "this buffered data
> will be transferred if CHCR::DE bit was cleared". So, this patch
> doesn't need to check TCRB register.
>
> Fixes: 73a47bd0da66 ("dmaengine: rcar-dmac: use TCRB instead of TCR for residue")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
> This patch is based on the latest slave-dma / fixes branch.
>
> This issue can be reproduced by the following commands on r8a7795
> Salvator-XS and Windows Teraterm :) :
> # stty 921600
> (Change Teraterm baud rate)
> # cat > rx.txt
> (Send 5MiB file by Teraterm. After a few minutes later, we cannot
> input any commands by the serial console.)
Ack/tested-by ..?
>
> drivers/dma/sh/rcar-dmac.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 2a2ccd9..8305a1c 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -774,8 +774,9 @@ static void rcar_dmac_sync_tcr(struct rcar_dmac_chan *chan)
> /* make sure all remaining data was flushed */
> rcar_dmac_chcr_de_barrier(chan);
>
> - /* back DE */
> - rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr);
> + /* back DE if remain data exists */
> + if (rcar_dmac_chan_read(chan, RCAR_DMATCR))
> + rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr);
> }
>
> static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* dma: stm32: replace "%p" with "%pK"
From: Vinod Koul @ 2018-07-09 17:27 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: dan.j.williams, mcoquelin.stm32, alexandre.torgue, dmaengine,
linux-arm-kernel, linux-kernel, Benjamin Gaignard
On 06-07-18, 15:02, Benjamin Gaignard wrote:
> The format specifier "%p" can leak kernel addresses.
> Use "%pK" instead.
The subsystem name is 'dmaengine' and not dma.
The git log on the subsystem should have told you this
Applied after fixing the tag, thanks.
^ permalink raw reply
* dma: add support for reporting pause and resume separately
From: Vinod Koul @ 2018-07-09 17:29 UTC (permalink / raw)
To: Marek Szyprowski
Cc: dmaengine, alsa-devel, linux-kernel, Dan Williams,
Lars-Peter Clausen, Mark Brown, Bartlomiej Zolnierkiewicz
On 02-07-18, 15:08, Marek Szyprowski wrote:
> 'cmd_pause' DMA channel capability means that respective DMA engine
> supports both pausing and resuming given DMA channel. However, in some
> cases it is important to know if DMA channel can be paused without the
> need to resume it. This is a typical requirement for proper residue
> reading on transfer timeout in UART drivers. There are also some DMA
> engines with limited hardware, which doesn't really support resuming.
>
> Reporting pause and resume capabilities separately allows UART drivers to
> properly check for the really required capabilities and operate in DMA
> mode also in systems with limited DMA hardware. On the other hand drivers,
> which rely on full channel suspend/resume support, should now check for
> both 'pause' and 'resume' features.
>
> Existing clients of dma_get_slave_caps() have been checked and the only
> driver which rely on proper channel resuming is soc-generic-dmaengine-pcm
> driver, which has been updated to check the newly added capability.
> Existing 'cmd_pause' now only indicates that DMA engine support pausing
> given DMA channel.
Applied after fixing the subsystem name, thanks
^ permalink raw reply
* [RFC] dmaengine: Add metadat_ops for dma_async_tx_descriptor
From: Vinod Koul @ 2018-07-10 5:52 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: radheys, vinod.koul, lars, michal.simek, linux-kernel, dmaengine,
dan.j.williams, appanad, linux-arm-kernel
Hey Peter,
Sorry for late response on this..
On 01-06-18, 13:24, Peter Ujfalusi wrote:
> If the DMA supports per descriptor metadata it can implement the attach,
> get_ptr/set_len callbacks.
>
> Client drivers must only use either attach or get_ptr/set_len to avoid
> miss configuration.
>
> Wrappers are also added for the metadata_ops:
> dmaengine_desc_attach_metadata()
> dmaengine_desc_get_metadata_ptr()
> dmaengine_desc_set_metadata_len()
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> Hi,
>
> since attachments are bouncing back, I send the patch separately
>
> Regards,
> Peter
>
> include/linux/dmaengine.h | 50 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 51fbb861e84b..ac42ace36aa3 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -491,6 +491,18 @@ struct dmaengine_unmap_data {
> dma_addr_t addr[0];
> };
>
> +struct dma_async_tx_descriptor;
> +
> +struct dma_descriptor_metadata_ops {
> + int (*attach)(struct dma_async_tx_descriptor *desc, void *data,
> + size_t len);
How does one detach? When should the client free up the memory, IOW when
does dma driver drop ref to data.
> +
> + void *(*get_ptr)(struct dma_async_tx_descriptor *desc,
> + size_t *payload_len, size_t *max_len);
so what is this supposed to do..?
> + int (*set_len)(struct dma_async_tx_descriptor *desc,
> + size_t payload_len);
attach already has length, so how does this help?
> +};
> +
> /**
> * struct dma_async_tx_descriptor - async transaction descriptor
> * ---dma generic offload fields---
> @@ -520,6 +532,7 @@ struct dma_async_tx_descriptor {
> dma_async_tx_callback_result callback_result;
> void *callback_param;
> struct dmaengine_unmap_data *unmap;
> + struct dma_descriptor_metadata_ops *metadata_ops;
> #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH
> struct dma_async_tx_descriptor *next;
> struct dma_async_tx_descriptor *parent;
> @@ -932,6 +945,43 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memcpy(
> len, flags);
> }
>
> +static inline int dmaengine_desc_attach_metadata(
> + struct dma_async_tx_descriptor *desc, void *data, size_t len)
> +{
> + if (!desc)
> + return 0;
> +
> + if (!desc->metadata_ops || !desc->metadata_ops->attach)
> + return -ENOTSUPP;
> +
> + return desc->metadata_ops->attach(desc, data, len);
> +}
> +
> +static inline void *dmaengine_desc_get_metadata_ptr(
> + struct dma_async_tx_descriptor *desc, size_t *payload_len,
> + size_t *max_len)
> +{
> + if (!desc)
> + return NULL;
> +
> + if (!desc->metadata_ops || !desc->metadata_ops->get_ptr)
> + return ERR_PTR(-ENOTSUPP);
> +
> + return desc->metadata_ops->get_ptr(desc, payload_len, max_len);
> +}
> +
> +static inline int dmaengine_desc_set_metadata_len(
> + struct dma_async_tx_descriptor *desc, size_t payload_len)
> +{
> + if (!desc)
> + return 0;
> +
> + if (!desc->metadata_ops || !desc->metadata_ops->set_len)
> + return -ENOTSUPP;
> +
> + return desc->metadata_ops->set_len(desc, payload_len);
> +}
> +
> /**
> * dmaengine_terminate_all() - Terminate all active DMA transfers
> * @chan: The channel for which to terminate the transfers
> --
> Peter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [v6,3/7] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
From: Wen He @ 2018-07-10 6:36 UTC (permalink / raw)
To: Vinod
Cc: dmaengine@vger.kernel.org, robh+dt@kernel.org,
devicetree@vger.kernel.org, Leo Li, Jiafei Pan, Jiaheng Fan
> -----Original Message-----
> From: Vinod [mailto:vkoul@kernel.org]
> Sent: 2018年6月28日 13:17
> To: Wen He <wen.he_1@nxp.com>
> Cc: dmaengine@vger.kernel.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; Leo Li <leoyang.li@nxp.com>; Jiafei Pan
> <jiafei.pan@nxp.com>; Jiaheng Fan <jiaheng.fan@nxp.com>
> Subject: Re: [v6 3/7] dmaengine: fsl-qdma: Add qDMA controller driver for
> Layerscape SoCs
>
> On 15-06-18, 18:22, Wen He wrote:
>
> > +static struct fsl_qdma_comp *fsl_qdma_request_enqueue_desc(
> > + struct fsl_qdma_chan *fsl_chan,
> > + unsigned int dst_nents,
> > + unsigned int src_nents)
> > +{
> > + struct fsl_qdma_comp *comp_temp;
> > + struct fsl_qdma_queue *queue = fsl_chan->queue;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&queue->queue_lock, flags);
> > + if (list_empty(&queue->comp_free)) {
> > + spin_unlock_irqrestore(&queue->queue_lock, flags);
> > + comp_temp = kzalloc(sizeof(*comp_temp), GFP_KERNEL);
>
> this is invoked from fsl_qdma_prep_memcpy, so needs to be GFP_NOWAIT
> flag
>
OK
> > + if (!comp_temp)
> > + return NULL;
> > +
> > + comp_temp->virt_addr = dma_pool_alloc(queue->comp_pool,
> > + GFP_KERNEL,
>
> here as well
>
> > +static struct fsl_qdma_queue *fsl_qdma_alloc_queue_resources(
> > + struct platform_device *pdev,
> > + unsigned int queue_num)
> > +{
> > + struct fsl_qdma_queue *queue_head, *queue_temp;
> > + int ret, len, i;
> > + unsigned int queue_size[FSL_QDMA_QUEUE_MAX];
> > +
> > + if (queue_num > FSL_QDMA_QUEUE_MAX)
> > + queue_num = FSL_QDMA_QUEUE_MAX;
> > + len = sizeof(*queue_head) * queue_num;
> > + queue_head = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
>
> we have kcalloc variant for this
>
OK
> > + if (!queue_head)
> > + return NULL;
> > +
> > + ret = device_property_read_u32_array(&pdev->dev, "queue-sizes",
> > + queue_size, queue_num);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Can't get queue-sizes.\n");
> > + return NULL;
> > + }
> > +
> > + for (i = 0; i < queue_num; i++) {
> > + if (queue_size[i] > FSL_QDMA_CIRCULAR_DESC_SIZE_MAX ||
> > + queue_size[i] < FSL_QDMA_CIRCULAR_DESC_SIZE_MIN)
> {
> > + dev_err(&pdev->dev, "Get wrong queue-sizes.\n");
> > + return NULL;
> > + }
> > + queue_temp = queue_head + i;
> > + queue_temp->cq = dma_alloc_coherent(&pdev->dev,
> > + sizeof(struct fsl_qdma_format) *
> > + queue_size[i],
> > + &queue_temp->bus_addr,
> > + GFP_KERNEL);
> > + if (!queue_temp->cq) {
> > + devm_kfree(&pdev->dev, queue_head);
>
> any reason this is freed explicitly
>
> > +static int fsl_qdma_halt(struct fsl_qdma_engine *fsl_qdma) {
> > + void __iomem *ctrl = fsl_qdma->ctrl_base;
> > + void __iomem *block = fsl_qdma->block_base;
> > + int i, count = 5;
>
> why 5
>
Nothing, a condition.
Maybe I should be set a safeguards timers in here(count=N).
> > +static int fsl_qdma_queue_transfer_complete(struct fsl_qdma_engine
> > +*fsl_qdma) {
> > + struct fsl_qdma_queue *fsl_queue = fsl_qdma->queue;
> > + struct fsl_qdma_queue *fsl_status = fsl_qdma->status;
> > + struct fsl_qdma_queue *temp_queue;
> > + struct fsl_qdma_comp *fsl_comp;
> > + struct fsl_qdma_format *status_addr;
> > + struct fsl_qdma_format *csgf_src;
> > + struct fsl_pre_status pre;
> > + void __iomem *block = fsl_qdma->block_base;
> > + u32 reg, i;
> > + bool duplicate, duplicate_handle;
> > +
> > + memset(&pre, 0, sizeof(struct fsl_pre_status));
> > +
> > + while (1) {
>
> this loop can potentially read bad data from HW and keep spining forever.
> Good practice would be to add safeguards and loop 'N' times which you deem
> is large enough to process
>
Yes, it's a good idea.
> > + duplicate = 0;
> > + duplicate_handle = 0;
>
> empty line here
>
> > + reg = qdma_readl(fsl_qdma, block + FSL_QDMA_BSQSR);
> > + if (reg & FSL_QDMA_BSQSR_QE)
> > + return 0;
>
> here..
>
> > + status_addr = fsl_status->virt_head;
> > + if (qdma_ccdf_get_queue(status_addr) == pre.queue &&
> > + qdma_ccdf_addr_get64(status_addr) == pre.addr)
> > + duplicate = 1;
>
> here (basically after each logical block to help read)
>
got it.
> > + i = qdma_ccdf_get_queue(status_addr);
> > + pre.queue = qdma_ccdf_get_queue(status_addr);
> > + pre.addr = qdma_ccdf_addr_get64(status_addr);
> > + temp_queue = fsl_queue + i;
> > + spin_lock(&temp_queue->queue_lock);
> > + if (list_empty(&temp_queue->comp_used)) {
> > + if (duplicate) {
> > + duplicate_handle = 1;
>
> why do this, you can remove duplicate_handle and use duplicate below?
Thank you, I will remove it in next version.
Best Regards,
Wen
>
> --
> ~Vinod
^ permalink raw reply
* [v1] dmaengine: hsu: Support dmaengine_terminate_sync()
From: Andy Shevchenko @ 2018-07-10 11:49 UTC (permalink / raw)
To: Vinod Koul, dmaengine; +Cc: Andy Shevchenko
It appears that the driver misses the support of dmaengine_terminate_sync().
Since many of callers expects this behaviour implement the new
device_synchronize() callback to allow proper synchronization when stopping
a channel.
Fixes: b36f09c3c441 ("dmaengine: Add transfer termination synchronization support")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Fixes tag is just an anchor for anyone who would like to see this change
backported.
drivers/dma/hsu/hsu.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/dma/hsu/hsu.c b/drivers/dma/hsu/hsu.c
index 29d04ca71d52..202ffa9f7611 100644
--- a/drivers/dma/hsu/hsu.c
+++ b/drivers/dma/hsu/hsu.c
@@ -413,6 +413,13 @@ static void hsu_dma_free_chan_resources(struct dma_chan *chan)
vchan_free_chan_resources(to_virt_chan(chan));
}
+static void hsu_dma_synchronize(struct dma_chan *chan)
+{
+ struct hsu_dma_chan *hsuc = to_hsu_dma_chan(chan);
+
+ vchan_synchronize(&hsuc->vchan);
+}
+
int hsu_dma_probe(struct hsu_dma_chip *chip)
{
struct hsu_dma *hsu;
@@ -459,6 +466,7 @@ int hsu_dma_probe(struct hsu_dma_chip *chip)
hsu->dma.device_pause = hsu_dma_pause;
hsu->dma.device_resume = hsu_dma_resume;
hsu->dma.device_terminate_all = hsu_dma_terminate_all;
+ hsu->dma.device_synchronize = hsu_dma_synchronize;
hsu->dma.src_addr_widths = HSU_DMA_BUSWIDTHS;
hsu->dma.dst_addr_widths = HSU_DMA_BUSWIDTHS;
^ permalink raw reply related
* [v1] dmaengine: idma64: Support dmaengine_terminate_sync()
From: Andy Shevchenko @ 2018-07-10 11:49 UTC (permalink / raw)
To: Vinod Koul, dmaengine; +Cc: Andy Shevchenko
It appears that the driver misses the support of dmaengine_terminate_sync().
Since many of callers expects this behaviour implement the new
device_synchronize() callback to allow proper synchronization when stopping
a channel.
Fixes: b36f09c3c441 ("dmaengine: Add transfer termination synchronization support")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Fixes tag is just an anchor for anyone who would like to see this change
backported.
drivers/dma/idma64.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
index e5c911200bdb..1fbf9cb9b742 100644
--- a/drivers/dma/idma64.c
+++ b/drivers/dma/idma64.c
@@ -496,6 +496,13 @@ static int idma64_terminate_all(struct dma_chan *chan)
return 0;
}
+static void idma64_synchronize(struct dma_chan *chan)
+{
+ struct idma64_chan *idma64c = to_idma64_chan(chan);
+
+ vchan_synchronize(&idma64c->vchan);
+}
+
static int idma64_alloc_chan_resources(struct dma_chan *chan)
{
struct idma64_chan *idma64c = to_idma64_chan(chan);
@@ -583,6 +590,7 @@ static int idma64_probe(struct idma64_chip *chip)
idma64->dma.device_pause = idma64_pause;
idma64->dma.device_resume = idma64_resume;
idma64->dma.device_terminate_all = idma64_terminate_all;
+ idma64->dma.device_synchronize = idma64_synchronize;
idma64->dma.src_addr_widths = IDMA64_BUSWIDTHS;
idma64->dma.dst_addr_widths = IDMA64_BUSWIDTHS;
^ permalink raw reply related
* dma: sh: rcar-dmac: avoid to write CHCR.TE to 1 if TCR is set to 0
From: Vinod Koul @ 2018-07-10 15:20 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: vinod.koul, dmaengine, linux-renesas-soc
On 02-07-18, 18:18, Yoshihiro Shimoda wrote:
> This patch fixes an issue that unexpected retransfering happens
> if TCR is set to 0 before rcar_dmac_sync_tcr() writes DE bit to
> the CHCR register. For example, sh-sci driver can reproduce this
> issue like below:
>
> In rx_timer_fn(): /* CHCR DE bit may be set to 1 */
> dmaengine_tx_status()
> rcar_dmac_tx_status()
> rcar_dmac_chan_get_residue()
> rcar_dmac_sync_tcr() /* TCR is possible to be set to 0 */
>
> According to the description of commit 73a47bd0da66 ("dmaengine:
> rcar-dmac: use TCRB instead of TCR for residue"), "this buffered data
> will be transferred if CHCR::DE bit was cleared". So, this patch
> doesn't need to check TCRB register.
>
> Fixes: 73a47bd0da66 ("dmaengine: rcar-dmac: use TCRB instead of TCR for residue")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
> This patch is based on the latest slave-dma / fixes branch.
>
> This issue can be reproduced by the following commands on r8a7795
> Salvator-XS and Windows Teraterm :) :
> # stty 921600
> (Change Teraterm baud rate)
> # cat > rx.txt
> (Send 5MiB file by Teraterm. After a few minutes later, we cannot
> input any commands by the serial console.)
Applied after fixing subsystem name
^ permalink raw reply
* [v1,1/4] dmaengine: imx-sdma: add memcpy interface
From: Vinod Koul @ 2018-07-10 15:29 UTC (permalink / raw)
To: Robin Gong
Cc: dan.j.williams, shawnguo, s.hauer, fabio.estevam, linux,
linux-arm-kernel, kernel, dmaengine, linux-kernel, linux-imx
Hi Robin,
On 11-07-18, 00:23, Robin Gong wrote:
> Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead
> of '0xffff'.
latter part should be its own patch. Never mix things
> +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> + struct dma_chan *chan, dma_addr_t dma_dst,
> + dma_addr_t dma_src, size_t len, unsigned long flags)
> +{
> + struct sdma_channel *sdmac = to_sdma_chan(chan);
> + struct sdma_engine *sdma = sdmac->sdma;
> + int channel = sdmac->channel;
> + size_t count;
> + int i = 0, param;
> + struct sdma_buffer_descriptor *bd;
> + struct sdma_desc *desc;
> +
> + if (!chan || !len)
> + return NULL;
> +
> + dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
> + &dma_src, &dma_dst, len, channel);
> +
> + desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len / SDMA_BD_MAX_CNT
> + + 1);
this looks quite odd to read consider:
esc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM,
len / SDMA_BD_MAX_CNT + 1);
> + if (!desc)
> + goto err_out;
> +
> + do {
> + count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> + bd = &desc->bd[i];
> + bd->buffer_addr = dma_src;
> + bd->ext_buffer_addr = dma_dst;
> + bd->mode.count = count;
> + desc->chn_count += count;
> +
> + switch (sdmac->word_size) {
> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
This looks wrong, we are in memcpy, there is no SLAVE so no SLAVE
widths..
> static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
> struct dma_chan *chan, struct scatterlist *sgl,
> unsigned int sg_len, enum dma_transfer_direction direction,
> @@ -1344,9 +1431,9 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
>
> count = sg_dma_len(sg);
>
> - if (count > 0xffff) {
> + if (count > SDMA_BD_MAX_CNT) {
> dev_err(sdma->dev, "SDMA channel %d: maximum bytes for sg entry exceeded: %d > %d\n",
> - channel, count, 0xffff);
> + channel, count, SDMA_BD_MAX_CNT);
these changes dont belong to this patch
> @@ -1486,6 +1573,8 @@ static int sdma_config(struct dma_chan *chan,
> sdmac->watermark_level |= (dmaengine_cfg->dst_maxburst << 16) &
> SDMA_WATERMARK_LEVEL_HWML;
> sdmac->word_size = dmaengine_cfg->dst_addr_width;
> + } else if (dmaengine_cfg->direction == DMA_MEM_TO_MEM) {
> + sdmac->word_size = dmaengine_cfg->dst_addr_width;
same here too, we are in .device_config which deals with slave. Not
memcpy!
> } else {
> sdmac->per_address = dmaengine_cfg->dst_addr;
> sdmac->watermark_level = dmaengine_cfg->dst_maxburst *
> @@ -1902,6 +1991,7 @@ static int sdma_probe(struct platform_device *pdev)
>
> dma_cap_set(DMA_SLAVE, sdma->dma_device.cap_mask);
> dma_cap_set(DMA_CYCLIC, sdma->dma_device.cap_mask);
> + dma_cap_set(DMA_MEMCPY, sdma->dma_device.cap_mask);
>
> INIT_LIST_HEAD(&sdma->dma_device.channels);
> /* Initialize channel parameters */
> @@ -1968,9 +2058,11 @@ static int sdma_probe(struct platform_device *pdev)
> sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
> sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
> sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> + sdma->dma_device.device_prep_dma_memcpy = sdma_prep_memcpy;
> sdma->dma_device.device_issue_pending = sdma_issue_pending;
> sdma->dma_device.dev->dma_parms = &sdma->dma_parms;
> - dma_set_max_seg_size(sdma->dma_device.dev, 65535);
> + sdma->dma_device.copy_align = DMAENGINE_ALIGN_4_BYTES;
> + dma_set_max_seg_size(sdma->dma_device.dev, SDMA_BD_MAX_CNT);
this line should not be part of this patch
^ permalink raw reply
* [v1,2/4] dmaengine: imx-sdma: add check_bd_buswidth() to kill the dulicated code
From: Vinod Koul @ 2018-07-10 15:31 UTC (permalink / raw)
To: Robin Gong
Cc: dan.j.williams, shawnguo, s.hauer, fabio.estevam, linux,
linux-arm-kernel, kernel, dmaengine, linux-kernel, linux-imx
On 11-07-18, 00:23, Robin Gong wrote:
> Add check_bd_buswidth() to minimize the code size.
this looks mostly fine and I think this should be first patch..
>
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
> drivers/dma/imx-sdma.c | 64 +++++++++++++++++++++++---------------------------
> 1 file changed, 29 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 27ccabf..ed2267d 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1326,6 +1326,33 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
> return NULL;
> }
>
> +static int check_bd_buswidth(struct sdma_buffer_descriptor *bd,
> + struct sdma_channel *sdmac, int count,
> + dma_addr_t dma_dst, dma_addr_t dma_src)
> +{
> + int ret = 0;
> +
> + switch (sdmac->word_size) {
> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
> + bd->mode.command = 0;
> + if ((count | dma_dst | dma_src) & 3)
> + ret = -EINVAL;
> + break;
empty line after each break please
> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
> + bd->mode.command = 2;
> + if ((count | dma_dst | dma_src) & 1)
> + ret = -EINVAL;
> + break;
> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
> + bd->mode.command = 1;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return ret;
> +}
> +
^ 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