* [v5,2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
From: Wen He @ 2018-05-31 1:58 UTC (permalink / raw)
To: Vinod Koul
Cc: dmaengine@vger.kernel.org, robh+dt@kernel.org,
devicetree@vger.kernel.org, Leo Li, Jiafei Pan, Jiaheng Fan
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogVmlub2QgS291bCBbbWFp
bHRvOnZpbm9kLmtvdWxAbGluYXJvLm9yZ10NCj4gU2VudDogMjAxOMTqNdTCMzDI1SAxODoyOA0K
PiBUbzogV2VuIEhlIDx3ZW4uaGVfMUBueHAuY29tPg0KPiBDYzogZG1hZW5naW5lQHZnZXIua2Vy
bmVsLm9yZzsgcm9iaCtkdEBrZXJuZWwub3JnOw0KPiBkZXZpY2V0cmVlQHZnZXIua2VybmVsLm9y
ZzsgTGVvIExpIDxsZW95YW5nLmxpQG54cC5jb20+OyBKaWFmZWkgUGFuDQo+IDxqaWFmZWkucGFu
QG54cC5jb20+OyBKaWFoZW5nIEZhbiA8amlhaGVuZy5mYW5AbnhwLmNvbT4NCj4gU3ViamVjdDog
UmU6IFt2NSAyLzZdIGRtYWVuZ2luZTogZnNsLXFkbWE6IEFkZCBxRE1BIGNvbnRyb2xsZXIgZHJp
dmVyIGZvcg0KPiBMYXllcnNjYXBlIFNvQ3MNCj4gDQo+IE9uIDI5LTA1LTE4LCAxMDozOCwgV2Vu
IEhlIHdyb3RlOg0KPiANCj4gPiA+ID4gPiA+ICsJLyoNCj4gPiA+ID4gPiA+ICsJICogQ2xlYXIg
dGhlIGNvbW1hbmQgcXVldWUgaW50ZXJydXB0IGRldGVjdCByZWdpc3RlciBmb3IgYWxsDQo+ID4g
PiBxdWV1ZXMuDQo+ID4gPiA+ID4gPiArCSAqLw0KPiA+ID4gPiA+ID4gKwlxZG1hX3dyaXRlbChm
c2xfcWRtYSwgMHhmZmZmZmZmZiwgYmxvY2sgKw0KPiA+ID4gPiA+ID4gK0ZTTF9RRE1BX0JDUUlE
UigwKSk7DQo+ID4gPiA+ID4NCj4gPiA+ID4gPiBidW5jaCBvZiB3cml0ZXMgd2l0aCAweGZmZmZm
ZmZmLCBjYW4geW91IGV4cGxhaW4gd2h5PyBBbHNvIGhlbHBzDQo+ID4gPiA+ID4gdG8gbWFrZSBh
IG1hY3JvIGZvciB0aGlzDQo+ID4gPiA+ID4NCj4gPiA+ID4NCj4gPiA+ID4gTWF5YmUgSSBtaXNz
ZWQgdGhhdCBJIHNob3VsZCBkZWZpbmVkIHRoZSB2YWx1ZSB0byB0aGUgbWFjcm8gYW5kDQo+ID4g
PiA+IGFkZA0KPiA+ID4gY29tbWVudC4NCj4gPiA+ID4gUmlnaHQ/DQo+ID4gPg0KPiA+ID4gdGhh
dCB3b3VsZCBoZWxwLCBidXQgd2h5IGFyZSB5b3Ugd3JpdGluZyAweGZmZmZmZmZmIGF0IGFsbCB0
aGVzZSBwbGFjZXM/DQo+ID4NCj4gPiBUaGlzIHZhbHVlIGlzIGZyb20gdGhlIGRhdGFzaGVldC4N
Cj4gPiBTaG91bGQgSSB3cml0ZSBjb21tZW50cyB0byBoZXJlPw0KPiANCj4gWWVzIHRoYXQgd291
bGQgaGVscCBleHBsYWluaW5nIHdoYXQgdGhpcyBkb2VzLi4NCj4gDQoNCkdvdCBpdC4gVGhhbmtz
DQoNCj4gPiA+ID4gPiA+ICtzdGF0aWMgdm9pZCBmc2xfcWRtYV9pc3N1ZV9wZW5kaW5nKHN0cnVj
dCBkbWFfY2hhbiAqY2hhbikgew0KPiA+ID4gPiA+ID4gKwlzdHJ1Y3QgZnNsX3FkbWFfY2hhbiAq
ZnNsX2NoYW4gPSB0b19mc2xfcWRtYV9jaGFuKGNoYW4pOw0KPiA+ID4gPiA+ID4gKwlzdHJ1Y3Qg
ZnNsX3FkbWFfcXVldWUgKmZzbF9xdWV1ZSA9IGZzbF9jaGFuLT5xdWV1ZTsNCj4gPiA+ID4gPiA+
ICsJdW5zaWduZWQgbG9uZyBmbGFnczsNCj4gPiA+ID4gPiA+ICsNCj4gPiA+ID4gPiA+ICsJc3Bp
bl9sb2NrX2lycXNhdmUoJmZzbF9xdWV1ZS0+cXVldWVfbG9jaywgZmxhZ3MpOw0KPiA+ID4gPiA+
ID4gKwlzcGluX2xvY2soJmZzbF9jaGFuLT52Y2hhbi5sb2NrKTsNCj4gPiA+ID4gPiA+ICsJaWYg
KHZjaGFuX2lzc3VlX3BlbmRpbmcoJmZzbF9jaGFuLT52Y2hhbikpDQo+ID4gPiA+ID4gPiArCQlm
c2xfcWRtYV9lbnF1ZXVlX2Rlc2MoZnNsX2NoYW4pOw0KPiA+ID4gPiA+ID4gKwlzcGluX3VubG9j
aygmZnNsX2NoYW4tPnZjaGFuLmxvY2spOw0KPiA+ID4gPiA+ID4gKwlzcGluX3VubG9ja19pcnFy
ZXN0b3JlKCZmc2xfcXVldWUtPnF1ZXVlX2xvY2ssIGZsYWdzKTsNCj4gPiA+ID4gPg0KPiA+ID4g
PiA+IHdoeSBkbyB3ZSBuZWVkIHR3byBsb2NrcywgYW5kIHNpbmNlIHlvdSBhcmUgZG9pbmcgdmNo
YW4gd2h5DQo+ID4gPiA+ID4gc2hvdWxkIHlvdSBhZGQgeW91ciBvd24gbG9jayBvbiB0b3ANCj4g
PiA+ID4gPg0KPiA+ID4gPg0KPiA+ID4gPiBZZXMsIHdlIG5lZWQgdHdvIGxvY2tzLg0KPiA+ID4g
PiBBcyB5b3Uga25vdywgdGhlIFFETUEgc3VwcG9ydCBtdWx0aXBsZSB2aXJ0dWFsaXplZCBibG9j
a3MgZm9yDQo+ID4gPiA+IG11bHRpLWNvcmUNCj4gPiA+IHN1cHBvcnQuDQo+ID4gPiA+IHNvIHdl
IG5lZWQgdG8gbWFrZSBzdXJlIHRoYXQgbXVsaXRpLWNvcmUgYWNjZXNzIGlzc3Vlcy4NCj4gPiA+
DQo+ID4gPiBidXQgd2h5IGNhbnQgeW91IHVzZSB2Y2hhbiBsb2NrIGZvciBhbGw/DQo+ID4gPg0K
PiA+DQo+ID4gV2UgY2FuJ3Qgb25seSB1c2UgdmNoYW4gbG9jayBmb3IgYWxsLiBvdGhlcndpc2Ug
ZW5xdWV1ZSBhY3Rpb24gd2lsbCBiZQ0KPiBpbnRlcnJ1cHRlZC4NCj4gDQo+IEkgdGhpbmsgaXQg
aXMgcG9zc2libGUgdG8gdXNlIG9ubHkgdmNoYW4gbG9jaw0KPiANCg0KSSB0cmllZCB0aGF0IGlm
IEkgdXNlIG9ubHkgdmNoYW4gbG9jayB0aGVuIHFkbWEgd2lsbCBiZSBjYW4ndCB3b3JrLg0KRG8g
eW91IGhhdmUgYSBvdGhlciBnb29kIGlkZWE/DQoNCkJlc3QgUmVnYXJkcywNCldlbg0KDQo+IC0t
DQo+IH5WaW5vZA0K
---
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
* [v5,3/6] dt-bindings: fsl-qdma: Add NXP Layerscpae qDMA controller bindings
From: Rob Herring @ 2018-05-31 0:49 UTC (permalink / raw)
To: Wen He; +Cc: vkoul, dmaengine, devicetree, leoyang.li, jiafei.pan, jiaheng.fan
On Fri, May 25, 2018 at 07:19:17PM +0800, Wen He wrote:
> Document the devicetree bindings for NXP Layerscape qDMA controller
> which could be found on NXP QorIQ Layerscape SoCs.
>
> Signed-off-by: Wen He <wen.he_1@nxp.com>
> ---
> change in v5:
> - Replace dts node variable 'queues' to 'fsl,queues' that add vendor prefix
>
> change in v4:
> - Rewrite the bindings document that follows generic DMA bindings file
>
> change in v3:
> - no change
>
> change in v2:
> - Remove indentation
> - Add "Should be" before 'fsl,ls1021a-qdma'
> - Replace 'channels' by 'dma-channels'
> - Replace 'qdma@8390000' by 'dma-controller@8390000'
>
> Documentation/devicetree/bindings/dma/fsl-qdma.txt | 41 ++++++++++++++++++++
> 1 files changed, 41 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/dma/fsl-qdma.txt
Reviewed-by: Rob Herring <robh@kernel.org>
---
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: mcf-edma: add ColdFire mcf5441x eDMA support
From: Angelo Dureghello @ 2018-05-30 20:59 UTC (permalink / raw)
To: Vinod; +Cc: Vinod Koul, dmaengine, gerg, linux-m68k
Hi Vinod,
On Mon, May 28, 2018 at 09:31:23AM +0530, Vinod wrote:
> Hi Angelo,
>
> On 26-05-18, 22:50, Angelo Dureghello wrote:
>
> > > wouldn't it be easier to just make common parts and then add edma specific code.
> > > If I was doing this it would be my apprach and that way code edma specific will
> > > be lesser and faster review
> > >
> >
> > I tried to set up a common module, but couldn't reach any good point.
> >
> > Issues are:
> > 1) Edma register set between 32 and 64ch is similar, but some offsets/names
> > are not matching between the 2 variants, some registers names are swapped over
> > the reg. address range,
> > 2) interrupt numbers and scheme is still different, handler implementation comes
> > different,
> > 3) as a corollary of the above, all the common functions that needs to access
> > edma registers should use same structure pointers. I could use a union
> > someway but points where register are accessed are many, and i should
> > differentiate the access in each case, referencing to a different structure
> > in each case.
> >
> > If you have any idea on how i could reach a common module, with 2 different
> > registers set, that's welcome.
> > I stay on the thought that a separate 64-channel module is the best
> > way to go here.
> >
> > Currently, as Freescale "edma" variants, i know:
> >
> > Vybrid VFXXX 32ch DMA multiplexer reg.set 1
> > Kynetis K70 (CortexM4) 32ch DMA multiplexer reg.set 1
> > imx8xx (coming) 32ch no multiplexer reg.set 1
> > MPC57xxk 32ch DMA multiplexer reg.set 1
> > ColdFire mcf5441x 64ch no multiplexer reg.set 2 <---
> >
> > There may me other cpu using this fsl edma module but not in my knowledge
> > right now.
> >
> > So i still think at the end, to have 2 separate drivers for the 32 and 64
> > variant is good and probably the most ordered/clean solution.
>
> Okay there are few ways we can do this. One is to use helpers for register
> access and these helpers are different for the variant you are loaded on.
>
> Another is to use register offsets which are set based on the variant loaded..
>
Ok i try with register offsets. Lets' see.
Thanks,
Angelo
> HTH
> --
> ~Vinod
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
---
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
* [RESEND,v2] dmaengine: pxa: add a default requestor policy
From: Robert Jarzmik @ 2018-05-30 20:12 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Vinod Koul,
Vinod Koul
Cc: linux-arm-kernel, dmaengine, linux-kernel, Arnd Bergmann
As what former drcmr -1 value meant, add a this as a default to each
channel, ie. that by default no requestor line is used.
This is specifically used for network drivers smc91x and smc911x, and
needed for their port to slave maps.
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v1: changed -1 to U32_MAX
---
drivers/dma/pxa_dma.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
index 9505334f9c6e..b31c28b67ad3 100644
--- a/drivers/dma/pxa_dma.c
+++ b/drivers/dma/pxa_dma.c
@@ -762,6 +762,8 @@ static void pxad_free_chan_resources(struct dma_chan *dchan)
dma_pool_destroy(chan->desc_pool);
chan->desc_pool = NULL;
+ chan->drcmr = U32_MAX;
+ chan->prio = PXAD_PRIO_LOWEST;
}
static void pxad_free_desc(struct virt_dma_desc *vd)
@@ -1386,6 +1388,9 @@ static int pxad_init_dmadev(struct platform_device *op,
c = devm_kzalloc(&op->dev, sizeof(*c), GFP_KERNEL);
if (!c)
return -ENOMEM;
+
+ c->drcmr = U32_MAX;
+ c->prio = PXAD_PRIO_LOWEST;
c->vc.desc_free = pxad_free_desc;
vchan_init(&c->vc, &pdev->slave);
init_waitqueue_head(&c->wq_state);
^ permalink raw reply related
* [v2] dmaengine: pxa: add a default requestor policy
From: Robert Jarzmik @ 2018-05-30 20:00 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Vinod Koul
Cc: linux-arm-kernel, dmaengine, linux-kernel, Arnd Bergmann
As what former drcmr -1 value meant, add a this as a default to each
channel, ie. that by default no requestor line is used.
This is specifically used for network drivers smc91x and smc911x, and
needed for their port to slave maps.
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v1: changed -1 to U32_MAX
---
drivers/dma/pxa_dma.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
index 9505334f9c6e..b31c28b67ad3 100644
--- a/drivers/dma/pxa_dma.c
+++ b/drivers/dma/pxa_dma.c
@@ -762,6 +762,8 @@ static void pxad_free_chan_resources(struct dma_chan *dchan)
dma_pool_destroy(chan->desc_pool);
chan->desc_pool = NULL;
+ chan->drcmr = U32_MAX;
+ chan->prio = PXAD_PRIO_LOWEST;
}
static void pxad_free_desc(struct virt_dma_desc *vd)
@@ -1386,6 +1388,9 @@ static int pxad_init_dmadev(struct platform_device *op,
c = devm_kzalloc(&op->dev, sizeof(*c), GFP_KERNEL);
if (!c)
return -ENOMEM;
+
+ c->drcmr = U32_MAX;
+ c->prio = PXAD_PRIO_LOWEST;
c->vc.desc_free = pxad_free_desc;
vchan_init(&c->vc, &pdev->slave);
init_waitqueue_head(&c->wq_state);
^ permalink raw reply related
* [v5,2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
From: Li Yang @ 2018-05-30 18:51 UTC (permalink / raw)
To: Wen He
Cc: vkoul, dmaengine, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
jiafei.pan, jiaheng.fan
On Fri, May 25, 2018 at 6:19 AM, Wen He <wen.he_1@nxp.com> wrote:
> NXP Queue DMA controller(qDMA) on Layerscape SoCs supports channel
> virtuallization by allowing DMA jobs to be enqueued into different
> command queues.
>
> Note that this module depends on NXP DPAA.
>
> Signed-off-by: Wen He <wen.he_1@nxp.com>
> Signed-off-by: Jiaheng Fan <jiaheng.fan@nxp.com>
> ---
> change in v5:
> - Fixed the issues includes:
> * add error handler which every function
> * replace word to bit definition
> * move global variable to struct definition
> * add some comments to context
>
> change in v4:
> - Fixed the issues that Vinod point out in the mail list.
>
> change in v3:
> - Add 'depends on ARM || ARM64' in file 'drivers/dma/Kconfig'
>
> change in v2:
> - Replace GPL V2 License details by SPDX tags
> - Replace Freescale by NXP
> - Reduce and optimize header file references
> - Replace big_endian by feature in struct fsl_qdma_engine
> - Replace struct fsl_qdma_format by struct fsl_qdma_ccdf
> and struct fsl_qdma_csgf
> - Remove empty line
> - Replace 'if..else' by macro 'FSL_QDMA_IN/OUT' in function
> qdma_readl() and qdma_writel()
> - Remove function fsl_qdma_alloc_chan_resources()
> - Replace 'prei' by 'pre'
> - Replace '-1' by '-ENOMEM' in function fsl_qdma_pre_request_enqueue_desc()
> - Fix dma pool allocation need to rolled back in function
> fsl_qdma_request_enqueue_desc()
> - Replace function of_property_read_u32_array() by device_property_read_u32_array()
> - Add functions fsl_qdma_cleanup_vchan() and fsl_qdma_irq_exit() to ensure
> irq and tasklets stopped
> - Replace dts node element 'channels' by 'dma-channels'
> - Replace function platform_driver_register() by module_platform_driver()
>
> drivers/dma/Kconfig | 13 +
> drivers/dma/Makefile | 1 +
> drivers/dma/fsl-qdma.c | 1101 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1115 insertions(+), 0 deletions(-)
> create mode 100644 drivers/dma/fsl-qdma.c
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 6d61cd0..99aff33 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -225,6 +225,19 @@ config FSL_EDMA
> multiplexing capability for DMA request sources(slot).
> This module can be found on Freescale Vybrid and LS-1 SoCs.
>
> +config FSL_QDMA
> + tristate "NXP Layerscape qDMA engine support"
> + depends on ARM || ARM64
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + select DMA_ENGINE_RAID
> + select ASYNC_TX_ENABLE_CHANNEL_SWITCH
> + help
> + Support the NXP Layerscape qDMA engine with command queue and legacy mode.
> + Channel virtualization is supported through enqueuing of DMA jobs to,
> + or dequeuing DMA jobs from, different work queues.
> + This module can be found on NXP Layerscape SoCs.
> +
> config FSL_RAID
> tristate "Freescale RAID engine Support"
> depends on FSL_SOC && !ASYNC_TX_ENABLE_CHANNEL_SWITCH
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 0f62a4d..93db0fc 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -33,6 +33,7 @@ obj-$(CONFIG_DW_DMAC_CORE) += dw/
> obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o
> obj-$(CONFIG_FSL_DMA) += fsldma.o
> obj-$(CONFIG_FSL_EDMA) += fsl-edma.o
> +obj-$(CONFIG_FSL_QDMA) += fsl-qdma.o
> obj-$(CONFIG_FSL_RAID) += fsl_raid.o
> obj-$(CONFIG_HSU_DMA) += hsu/
> obj-$(CONFIG_IMG_MDC_DMA) += img-mdc-dma.o
> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c
> new file mode 100644
> index 0000000..81df812
> --- /dev/null
> +++ b/drivers/dma/fsl-qdma.c
> @@ -0,0 +1,1101 @@
Not a critical issue, but the format of the file header looks a little
bit weird to me. Would you mind clean it up?
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2018 NXP
I know the SPDX tag is recommended to be the 1st line, but copyright
normally goes below the file description not above.
> +
No newline needed.
> +/*
> + * Driver for NXP Layerscape Queue Direct Memory Access Controller
> + *
> + * Author:
> + * Wen He <wen.he_1@nxp.com>
> + * Jiaheng Fan <jiaheng.fan@nxp.com>
> + *
No newline needed.
> + */
> +
Regards,
Leo
---
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
* [RFC,2/6] dmaengine: xilinx_dma: Pass AXI4-Stream control words to netdev dma client
From: Radhey Shyam Pandey @ 2018-05-30 17:29 UTC (permalink / raw)
To: Peter Ujfalusi, Vinod Koul
Cc: Lars-Peter Clausen, michal.simek@xilinx.com,
linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org,
dan.j.williams@intel.com, Appana Durga Kedareswara Rao,
linux-arm-kernel@lists.infradead.org
SGksDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogUGV0ZXIgVWpmYWx1
c2kgW21haWx0bzpwZXRlci51amZhbHVzaUB0aS5jb21dDQo+IFNlbnQ6IFR1ZXNkYXksIE1heSAy
OSwgMjAxOCA4OjM1IFBNDQo+IFRvOiBSYWRoZXkgU2h5YW0gUGFuZGV5IDxyYWRoZXlzQHhpbGlu
eC5jb20+OyBWaW5vZCBLb3VsDQo+IDx2aW5vZC5rb3VsQGludGVsLmNvbT4NCj4gQ2M6IExhcnMt
UGV0ZXIgQ2xhdXNlbiA8bGFyc0BtZXRhZm9vLmRlPjsgbWljaGFsLnNpbWVrQHhpbGlueC5jb207
IGxpbnV4LQ0KPiBrZXJuZWxAdmdlci5rZXJuZWwub3JnOyBkbWFlbmdpbmVAdmdlci5rZXJuZWwu
b3JnOw0KPiBkYW4uai53aWxsaWFtc0BpbnRlbC5jb207IEFwcGFuYSBEdXJnYSBLZWRhcmVzd2Fy
YSBSYW8NCj4gPGFwcGFuYWRAeGlsaW54LmNvbT47IGxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5m
cmFkZWFkLm9yZw0KPiBTdWJqZWN0OiBSZTogW1JGQyAyLzZdIGRtYWVuZ2luZTogeGlsaW54X2Rt
YTogUGFzcyBBWEk0LVN0cmVhbSBjb250cm9sIHdvcmRzDQo+IHRvIG5ldGRldiBkbWEgY2xpZW50
DQo+IA0KPiBIaSwNCj4gDQo+IE9uIDIwMTgtMDUtMTcgMDk6MzksIFJhZGhleSBTaHlhbSBQYW5k
ZXkgd3JvdGU6DQo+ID4+IFdlbGwsIGxldCdzIHNlZSB3aGVyZSB0aGlzIGlzIGdvaW5nIHRvIGdv
IHdoZW4gSSBjYW4gc2VuZCB0aGUgcGF0Y2hlcw0KPiA+PiBmb3IgcmV2aWV3Lg0KPiA+IFRoYW5r
cyBhbGwuIEBQZXRlcjogSWYgd2UgaGF2ZSBtZXRhZGF0YSBwYXRjaHNldCByZWFkeSBtYXkgYmUg
Z29vZA0KPiA+IHRvIHNlbmQgYW4gUkZDPw0KPiANCj4gU29ycnkgZm9yIHRoZSBkZWxheSwgSSBn
b3QgZGlzdHJhY3RlZCBieSB0aGlzOg0KPiBodHRwOi8vd3d3LnRpLmNvbS9saXQvcGRmL3NwcnVp
ZDcgQ2hhcHRlciAxMC4NCj4gDQo+IEkgaGF2ZSBnaXZlbiBzb21lIHRvdWdoIHRvIHRoZSBtZXRh
ZGF0YSBhdHRhY2ggcGF0Y2hlcy4NCj4gSW4gbXkgY2FzZSB0aGUgJ21ldGFkYXRhJyBpcyBtb3Jl
IGxpa2UgcHJpdmF0ZSBkYXRhIHNlY3Rpb24gd2l0aGluIHRoZQ0KPiBETUEgZGVzY3JpcHRvciAo
MTAuMS4yLjIuMSkgd2hpY2ggaXMgdXNlZCBieSB0aGUgcmVtb3RlIHBlcmlwaGVyYWwgYW5kDQo+
IHRoZSBkcml2ZXIgZm9yIHRoZSBnaXZlbiBwZXJpcGhlcmFsIGFuZCBpdCBpcyBvcHRpb25hbC4N
Cj4gDQo+IEkgbGlrZWQgdGhlIGlkZWEgb2YgdHJlYXRpbmcgaXQgYXMgbWV0YWRhdGEgYXMgaXQg
Z2l2ZXMgbW9yZSBnZW5lcmljIEFQSQ0KPiB3aGljaCBjYW4gYmUgYWRvcHRlZCBieSBvdGhlciBk
cml2ZXJzIGlmIHRoZXkgbmVlZCBzb21ldGhpbmcgc2ltaWxhci4NCj4gDQo+IEFub3RoZXIgaXNz
dWUgSSBoYXZlIHdpdGggdGhlIGF0dGFjaCBtZXRhZGF0YSB3YXkgaXMgdGhhdCBpdCB3b3VsZA0K
PiByZXF1aXJlIG9uZSBtZW1jcHkgdG8gY29weSB0aGUgZGF0YSB0byB0aGUgRE1BIGRlc2NyaXB0
b3IgYW5kIGluIGhpZ2gNCj4gdGhyb3VnaHB1dCBjYXNlIGl0IGlzIG5vdCBhY2NlcHRhYmxlLg0K
DQpJIHRoaW5rIG1lbWNweSBpcyBuZWVkZWQgKGFsdGVybmF0aXZlPykgaWYgZG1hIGVuZ2luZSBk
b2VzbuKAmXQgZGlyZWN0bHkNCnVwZGF0ZSBtZXRhZGF0YSBidWZmZXJzIGkuZSBpbiBSWCwgd2Ug
bmVlZCB0byBjb3B5IG1ldGFkYXRhIGZyb20gDQpkbWEgZGVzY3JpcHRvciBmaWVsZHMgdG8gY2xp
ZW50IGFsbG9jYXRlZCBtZXRhZGF0YSBidWZmZXIgKHNpZGViYW5kLw0KbWV0YWRhdGEgaW5mbyBp
cyBwYXJ0IG9mIEJ1ZmZlciBkZXNjcmlwdG9yIGZpZWxkcykgDQoNCm1lbWNweShhcHBfdywgaHct
PmFwcCwgc2l6ZW9mKHUzMikgKiBYSUxJTlhfRE1BX05VTV9BUFBfV09SRFMpOw0KDQpEZXNjcmlw
dG9yIEZpZWxkcw0KQWRkcmVzcyBTcGFjZSAgICAgICAgICAgICAgIE9mZnNldCAgICAgICAgICBO
YW1lIERlc2NyaXB0aW9uDQoyMGggICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgQVBQ
MCAgICAgICAgICAgVXNlciBBcHBsaWNhdGlvbiBGaWVsZCAwDQoyNGggICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgQVBQMSAgICAgICAgICAgVXNlciBBcHBsaWNhdGlvbiBGaWVsZCAx
DQouLi4NCjMwaCAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIEFQUDUgICAgICAgICAg
IFVzZXIgQXBwbGljYXRpb24gRmllbGQgNQ0KDQo+IA0KPiBGb3IgbWUgcHJvYmFibHkgYSAuZ2V0
X3ByaXZhdGVfYXJlYSAvIC5wdXRfcHJpdmF0ZV9hcmVhIGxpa2UgQVBJIHdvdWxkDQo+IGJlIGRl
c2lyYWJsZSB3aGVyZSBJIGNhbiBnaXZlIHRoZSBwb2ludGVyIG9mIHRoZSAnbWV0YWRhdGEnIGFy
ZSAoYW5kDQo+IHNpemUpIHRvIHRoZSB1c2VyLg0KPiANCj4gQnV0IHRoZXNlIGNhbiBjby1leGlz
dCBpbiBteSBvcGluaW9uIGFuZCBETUEgZHJpdmVycyBjYW4gb3B0IHRvDQo+IGltcGxlbWVudCBu
b25lLCBlaXRoZXIgb3IgYm90aCBvZiB0aGUgY2FsbGJhY2tzLg0KPiANCj4gSW4gY291cGxlIG9m
IGRheXMgSSBjYW4gdXBkYXRlIHRoZSBtZXRhZGF0YSBwYXRjaGVzIEkgaGF2ZSBhdG0gYW5kIHNl
bmQNCj4gYXMgUkZDLg0KPiANCj4gSXMgdGhlcmUgYW55dGhpbmcgZnJvbSB5b3VyIHNpZGUgSSBz
aG91bGQgdGFrZSBpbnRvIGFjY291bnQgd2hlbiBkb2luZyB0aGF0Pw0KSSB0aGluayBhIGdlbmVy
aWMgaW50ZXJmYWNlIHRvIGF0dGFjaC9zaGFyZSBtZXRhZGF0YSBidWZmZXIgYi93IGNsaWVudCBh
bmQgdGhlDQpkbWFlbmdpbmUgZHJpdmVyIGlzIGdvb2QgZW5vdWdoLiBJcyBtZXRhZGF0YSBwYXRj
aHNldCAoZWFybHkgdmVyc2lvbikgDQphdmFpbGFibGUgaW4gVEkgZXh0ZXJuYWwgcmVwb3M/IA0K
DQpUaGFua3MsDQpSYWRoZXkNCg0KPiANCj4gLSBQw6l0ZXINCj4gDQo+IFRleGFzIEluc3RydW1l
bnRzIEZpbmxhbmQgT3ksIFBvcmtrYWxhbmthdHUgMjIsIDAwMTgwIEhlbHNpbmtpLg0KPiBZLXR1
bm51cy9CdXNpbmVzcyBJRDogMDYxNTUyMS00LiBLb3RpcGFpa2thL0RvbWljaWxlOiBIZWxzaW5r
aQ0K
---
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
* [v5,2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
From: Vinod Koul @ 2018-05-30 10:27 UTC (permalink / raw)
To: Wen He
Cc: dmaengine@vger.kernel.org, robh+dt@kernel.org,
devicetree@vger.kernel.org, Leo Li, Jiafei Pan, Jiaheng Fan
On 29-05-18, 10:38, Wen He wrote:
> > > > > + /*
> > > > > + * Clear the command queue interrupt detect register for all
> > queues.
> > > > > + */
> > > > > + qdma_writel(fsl_qdma, 0xffffffff, block + FSL_QDMA_BCQIDR(0));
> > > >
> > > > bunch of writes with 0xffffffff, can you explain why? Also helps to
> > > > make a macro for this
> > > >
> > >
> > > Maybe I missed that I should defined the value to the macro and add
> > comment.
> > > Right?
> >
> > that would help, but why are you writing 0xffffffff at all these places?
>
> This value is from the datasheet.
> Should I write comments to here?
Yes that would help explaining what this does..
> > > > > +static void fsl_qdma_issue_pending(struct dma_chan *chan) {
> > > > > + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan);
> > > > > + struct fsl_qdma_queue *fsl_queue = fsl_chan->queue;
> > > > > + unsigned long flags;
> > > > > +
> > > > > + spin_lock_irqsave(&fsl_queue->queue_lock, flags);
> > > > > + spin_lock(&fsl_chan->vchan.lock);
> > > > > + if (vchan_issue_pending(&fsl_chan->vchan))
> > > > > + fsl_qdma_enqueue_desc(fsl_chan);
> > > > > + spin_unlock(&fsl_chan->vchan.lock);
> > > > > + spin_unlock_irqrestore(&fsl_queue->queue_lock, flags);
> > > >
> > > > why do we need two locks, and since you are doing vchan why should
> > > > you add your own lock on top
> > > >
> > >
> > > Yes, we need two locks.
> > > As you know, the QDMA support multiple virtualized blocks for multi-core
> > support.
> > > so we need to make sure that muliti-core access issues.
> >
> > but why cant you use vchan lock for all?
> >
>
> We can't only use vchan lock for all. otherwise enqueue action will be interrupted.
I think it is possible to use only vchan lock
^ permalink raw reply
* [RESEND] dmaengine: pxa: add a default requestor policy
From: Vinod Koul @ 2018-05-30 10:25 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Daniel Mack, Haojian Zhuang, Vinod Koul, linux-arm-kernel,
dmaengine, linux-kernel, Arnd Bergmann
On 29-05-18, 21:13, Robert Jarzmik wrote:
> Vinod <vkoul@kernel.org> writes:
>
> > On 26-05-18, 11:54, Robert Jarzmik wrote:
> >> @@ -762,6 +762,8 @@ static void pxad_free_chan_resources(struct dma_chan *dchan)
> >> dma_pool_destroy(chan->desc_pool);
> >> chan->desc_pool = NULL;
> >>
> >> + chan->drcmr = (u32)-1;
> >
> > why not use U32_MAX for this?
> But of course, anything else you see ?
Nope that was the only one :)
^ permalink raw reply
* [RESEND] dmaengine: pxa: add a default requestor policy
From: Robert Jarzmik @ 2018-05-29 19:13 UTC (permalink / raw)
To: Vinod
Cc: Daniel Mack, Haojian Zhuang, Vinod Koul, linux-arm-kernel,
dmaengine, linux-kernel, Arnd Bergmann
Vinod <vkoul@kernel.org> writes:
> On 26-05-18, 11:54, Robert Jarzmik wrote:
>> @@ -762,6 +762,8 @@ static void pxad_free_chan_resources(struct dma_chan *dchan)
>> dma_pool_destroy(chan->desc_pool);
>> chan->desc_pool = NULL;
>>
>> + chan->drcmr = (u32)-1;
>
> why not use U32_MAX for this?
But of course, anything else you see ?
Cheers.
^ permalink raw reply
* [RFC,2/6] dmaengine: xilinx_dma: Pass AXI4-Stream control words to netdev dma client
From: Peter Ujfalusi @ 2018-05-29 15:04 UTC (permalink / raw)
To: Radhey Shyam Pandey, Vinod Koul
Cc: Lars-Peter Clausen, michal.simek@xilinx.com,
linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org,
dan.j.williams@intel.com, Appana Durga Kedareswara Rao,
linux-arm-kernel@lists.infradead.org
Hi,
On 2018-05-17 09:39, Radhey Shyam Pandey wrote:
>> Well, let's see where this is going to go when I can send the patches
>> for review.
> Thanks all. @Peter: If we have metadata patchset ready may be good
> to send an RFC?
Sorry for the delay, I got distracted by this:
http://www.ti.com/lit/pdf/spruid7 Chapter 10.
I have given some tough to the metadata attach patches.
In my case the 'metadata' is more like private data section within the
DMA descriptor (10.1.2.2.1) which is used by the remote peripheral and
the driver for the given peripheral and it is optional.
I liked the idea of treating it as metadata as it gives more generic API
which can be adopted by other drivers if they need something similar.
Another issue I have with the attach metadata way is that it would
require one memcpy to copy the data to the DMA descriptor and in high
throughput case it is not acceptable.
For me probably a .get_private_area / .put_private_area like API would
be desirable where I can give the pointer of the 'metadata' are (and
size) to the user.
But these can co-exist in my opinion and DMA drivers can opt to
implement none, either or both of the callbacks.
In couple of days I can update the metadata patches I have atm and send
as RFC.
Is there anything from your side I should take into account when doing that?
- Péter
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
* [v5,2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
From: Wen He @ 2018-05-29 10:38 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年5月29日 18:20
> 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: [v5 2/6] dmaengine: fsl-qdma: Add qDMA controller driver for
> Layerscape SoCs
>
> On 29-05-18, 09:59, Wen He wrote:
> > > On 25-05-18, 19:19, Wen He wrote:
> > >
> > > > +/**
> > > > + * struct fsl_qdma_format - This is the struct holding describing
> compound
> > > > + * descriptor format with qDMA.
> > > > + * @status: This field which describes command status and
> > > > + * enqueue status notification.
> > > > + * @cfg: This field which describes frame offset and
> frame
> > > > + * format.
> > > > + * @addr_lo: This field which indicating the start of the
> buffer
> > > > + * holding the compound descriptor of the lower 32-bits
> > > > + * address in memory 40-bit address.
> > > > + * @addr_hi: This field's the same as above field, but
> point
> > > high
> > > > + * 8-bits in memory 40-bit address.
> > > > + * @__reserved1: Reserved field.
> > > > + * @cfg8b_w1: This field which describes compound
> descriptor
> > > > + * command queue origin produced by qDMA and
> > > dynamic
> > >
> > > you may remove 'This field which describes'... in above lines, give
> > > reader no information :)
> > >
> >
> > Ok, so I remove 'this filed which describes' but keep the second half, right?
>
> right
>
> > > > + 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;
> > >
> > > the indents here are bad for reading..
> > >
> >
> > So I add a empty line in here?
>
> No, indent of trailing condition is same as code block, that causes confusion,
> they should have different indent
>
Got it.
> > > > + /*
> > > > + * Clear the command queue interrupt detect register for all
> queues.
> > > > + */
> > > > + qdma_writel(fsl_qdma, 0xffffffff, block + FSL_QDMA_BCQIDR(0));
> > >
> > > bunch of writes with 0xffffffff, can you explain why? Also helps to
> > > make a macro for this
> > >
> >
> > Maybe I missed that I should defined the value to the macro and add
> comment.
> > Right?
>
> that would help, but why are you writing 0xffffffff at all these places?
>
This value is from the datasheet.
Should I write comments to here?
> > > > +static void fsl_qdma_issue_pending(struct dma_chan *chan) {
> > > > + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan);
> > > > + struct fsl_qdma_queue *fsl_queue = fsl_chan->queue;
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&fsl_queue->queue_lock, flags);
> > > > + spin_lock(&fsl_chan->vchan.lock);
> > > > + if (vchan_issue_pending(&fsl_chan->vchan))
> > > > + fsl_qdma_enqueue_desc(fsl_chan);
> > > > + spin_unlock(&fsl_chan->vchan.lock);
> > > > + spin_unlock_irqrestore(&fsl_queue->queue_lock, flags);
> > >
> > > why do we need two locks, and since you are doing vchan why should
> > > you add your own lock on top
> > >
> >
> > Yes, we need two locks.
> > As you know, the QDMA support multiple virtualized blocks for multi-core
> support.
> > so we need to make sure that muliti-core access issues.
>
> but why cant you use vchan lock for all?
>
We can't only use vchan lock for all. otherwise enqueue action will be interrupted.
Best Regards,
Wen
> --
> ~Vinod
^ permalink raw reply
* [v5,1/6] dmaengine: fsldma: Replace DMA_IN/OUT by FSL_DMA_IN/OUT
From: Wen He @ 2018-05-29 10:22 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年5月29日 18:21
> 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: [v5 1/6] dmaengine: fsldma: Replace DMA_IN/OUT by
> FSL_DMA_IN/OUT
>
> On 29-05-18, 10:14, Wen He wrote:
> > >
> > > Well that patch seems to do a bit more than replace DMA_IN/OUT by
> > > FSL_DMA_IN/OUT care to explain those. best would be to do a patch
> > > which does replace and then add new things in separate patch, easier
> > > to review
> > >
> >
> > What does means?
> > This also a patch.
>
> You cna do only replace DMA_IN/OUT by FSL_DMA_IN/OUT in this patch.
> Second patch can add additional macros you have added and explain what
> they do and why they are needed
>
Got it, it's good idea.
Wen
> --
> ~Vinod
^ permalink raw reply
* [v5,1/6] dmaengine: fsldma: Replace DMA_IN/OUT by FSL_DMA_IN/OUT
From: Vinod Koul @ 2018-05-29 10:21 UTC (permalink / raw)
To: Wen He
Cc: dmaengine@vger.kernel.org, robh+dt@kernel.org,
devicetree@vger.kernel.org, Leo Li, Jiafei Pan, Jiaheng Fan
On 29-05-18, 10:14, Wen He wrote:
> >
> > Well that patch seems to do a bit more than replace DMA_IN/OUT by
> > FSL_DMA_IN/OUT care to explain those. best would be to do a patch which
> > does replace and then add new things in separate patch, easier to review
> >
>
> What does means?
> This also a patch.
You cna do only replace DMA_IN/OUT by FSL_DMA_IN/OUT in this patch. Second patch
can add additional macros you have added and explain what they do and why they
are needed
^ permalink raw reply
* [v5,2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
From: Vinod Koul @ 2018-05-29 10:19 UTC (permalink / raw)
To: Wen He
Cc: dmaengine@vger.kernel.org, robh+dt@kernel.org,
devicetree@vger.kernel.org, Leo Li, Jiafei Pan, Jiaheng Fan
On 29-05-18, 09:59, Wen He wrote:
> > On 25-05-18, 19:19, Wen He wrote:
> >
> > > +/**
> > > + * struct fsl_qdma_format - This is the struct holding describing compound
> > > + * descriptor format with qDMA.
> > > + * @status: This field which describes command status and
> > > + * enqueue status notification.
> > > + * @cfg: This field which describes frame offset and frame
> > > + * format.
> > > + * @addr_lo: This field which indicating the start of the buffer
> > > + * holding the compound descriptor of the lower 32-bits
> > > + * address in memory 40-bit address.
> > > + * @addr_hi: This field's the same as above field, but point
> > high
> > > + * 8-bits in memory 40-bit address.
> > > + * @__reserved1: Reserved field.
> > > + * @cfg8b_w1: This field which describes compound descriptor
> > > + * command queue origin produced by qDMA and
> > dynamic
> >
> > you may remove 'This field which describes'... in above lines, give reader no
> > information :)
> >
>
> Ok, so I remove 'this filed which describes' but keep the second half, right?
right
> > > + 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;
> >
> > the indents here are bad for reading..
> >
>
> So I add a empty line in here?
No, indent of trailing condition is same as code block, that causes confusion,
they should have different indent
> > > + /*
> > > + * Clear the command queue interrupt detect register for all queues.
> > > + */
> > > + qdma_writel(fsl_qdma, 0xffffffff, block + FSL_QDMA_BCQIDR(0));
> >
> > bunch of writes with 0xffffffff, can you explain why? Also helps to make a
> > macro for this
> >
>
> Maybe I missed that I should defined the value to the macro and add comment.
> Right?
that would help, but why are you writing 0xffffffff at all these places?
> > > +static void fsl_qdma_issue_pending(struct dma_chan *chan) {
> > > + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan);
> > > + struct fsl_qdma_queue *fsl_queue = fsl_chan->queue;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&fsl_queue->queue_lock, flags);
> > > + spin_lock(&fsl_chan->vchan.lock);
> > > + if (vchan_issue_pending(&fsl_chan->vchan))
> > > + fsl_qdma_enqueue_desc(fsl_chan);
> > > + spin_unlock(&fsl_chan->vchan.lock);
> > > + spin_unlock_irqrestore(&fsl_queue->queue_lock, flags);
> >
> > why do we need two locks, and since you are doing vchan why should you add
> > your own lock on top
> >
>
> Yes, we need two locks.
> As you know, the QDMA support multiple virtualized blocks for multi-core support.
> so we need to make sure that muliti-core access issues.
but why cant you use vchan lock for all?
^ permalink raw reply
* [v5,1/6] dmaengine: fsldma: Replace DMA_IN/OUT by FSL_DMA_IN/OUT
From: Wen He @ 2018-05-29 10:14 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: dmaengine-owner@vger.kernel.org
> [mailto:dmaengine-owner@vger.kernel.org] On Behalf Of Vinod
> Sent: 2018年5月29日 12:49
> 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: [v5 1/6] dmaengine: fsldma: Replace DMA_IN/OUT by
> FSL_DMA_IN/OUT
>
> On 25-05-18, 19:19, Wen He wrote:
> > This patch implenment a standard macro call functions is used to NXP
> > dma drivers.
>
> Well that patch seems to do a bit more than replace DMA_IN/OUT by
> FSL_DMA_IN/OUT care to explain those. best would be to do a patch which
> does replace and then add new things in separate patch, easier to review
>
What does means?
This also a patch.
Best Regards,
Wen
> --
> ~Vinod
> --
> 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
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.
> kernel.org%2Fmajordomo-info.html&data=02%7C01%7Cwen.he_1%40nxp.co
> m%7Cdacb8ef7648a4347203c08d5c51f907b%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636631661717012484&sdata=F4lY39RgFbR3707Vj
> DuCskaes3ppyu5kMD55wDjnCwk%3D&reserved=0
^ permalink raw reply
* [v5,2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
From: Wen He @ 2018-05-29 9:59 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: dmaengine-owner@vger.kernel.org
> [mailto:dmaengine-owner@vger.kernel.org] On Behalf Of Vinod
> Sent: 2018年5月29日 15:07
> 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: [v5 2/6] dmaengine: fsl-qdma: Add qDMA controller driver for
> Layerscape SoCs
>
> On 25-05-18, 19:19, Wen He wrote:
>
> > +/**
> > + * struct fsl_qdma_format - This is the struct holding describing compound
> > + * descriptor format with qDMA.
> > + * @status: This field which describes command status and
> > + * enqueue status notification.
> > + * @cfg: This field which describes frame offset and frame
> > + * format.
> > + * @addr_lo: This field which indicating the start of the buffer
> > + * holding the compound descriptor of the lower 32-bits
> > + * address in memory 40-bit address.
> > + * @addr_hi: This field's the same as above field, but point
> high
> > + * 8-bits in memory 40-bit address.
> > + * @__reserved1: Reserved field.
> > + * @cfg8b_w1: This field which describes compound descriptor
> > + * command queue origin produced by qDMA and
> dynamic
>
> you may remove 'This field which describes'... in above lines, give reader no
> information :)
>
Ok, so I remove 'this filed which describes' but keep the second half, right?
> > + * debug field.
> > + * @data Pointer to the memory 40-bit address, describes
> DMA
> > + * source informaion and DMA destination information.
>
> typo informaion
>
I am sorry , It's stupid.
> > +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);
> > + 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;
>
> the indents here are bad for reading..
>
So I add a empty line in here?
> > +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) {
> > + duplicate = 0;
> > + duplicate_handle = 0;
> > + reg = qdma_readl(fsl_qdma, block + FSL_QDMA_BSQSR);
> > + if (reg & FSL_QDMA_BSQSR_QE)
> > + return 0;
> > + 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;
> > + 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;
>
> code style mandates braces for this as else has braces..
>
Thanks, it's good comments.
> > + else {
> > + spin_unlock(&temp_queue->queue_lock);
> > + return -EAGAIN;
> > + }
> > + } else {
> > + fsl_comp = list_first_entry(&temp_queue->comp_used,
> > + struct fsl_qdma_comp,
> > + list);
> > + csgf_src = fsl_comp->virt_addr + 2;
> > + if (fsl_comp->bus_addr + 16 != pre.addr) {
> > + if (duplicate)
> > + duplicate_handle = 1;
>
> here as well
>
sure
> > +static irqreturn_t fsl_qdma_error_handler(int irq, void *dev_id) {
> > + struct fsl_qdma_engine *fsl_qdma = dev_id;
> > + unsigned int intr;
> > + void __iomem *status = fsl_qdma->status_base;
> > +
> > + intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
> > +
> > + if (intr)
> > + dev_err(fsl_qdma->dma_dev.dev, "DMA transaction error!\n");
> > +
> > + qdma_writel(fsl_qdma, 0xffffffff, status + FSL_QDMA_DEDR);
>
> why unconditional write, was expecting that you would write if intr is non null
>
> > +static int fsl_qdma_reg_init(struct fsl_qdma_engine *fsl_qdma) {
> > + struct fsl_qdma_queue *fsl_queue = fsl_qdma->queue;
> > + struct fsl_qdma_queue *temp;
> > + void __iomem *ctrl = fsl_qdma->ctrl_base;
> > + void __iomem *status = fsl_qdma->status_base;
> > + void __iomem *block = fsl_qdma->block_base;
> > + int i, ret;
> > + u32 reg;
> > +
> > + /* Try to halt the qDMA engine first. */
> > + ret = fsl_qdma_halt(fsl_qdma);
> > + if (ret) {
> > + dev_err(fsl_qdma->dma_dev.dev, "DMA halt failed!");
> > + return ret;
> > + }
> > +
> > + /*
> > + * Clear the command queue interrupt detect register for all queues.
> > + */
> > + qdma_writel(fsl_qdma, 0xffffffff, block + FSL_QDMA_BCQIDR(0));
>
> bunch of writes with 0xffffffff, can you explain why? Also helps to make a
> macro for this
>
Maybe I missed that I should defined the value to the macro and add comment.
Right?
> > +
> > + for (i = 0; i < fsl_qdma->n_queues; i++) {
> > + temp = fsl_queue + i;
> > + /*
> > + * Initialize Command Queue registers to point to the first
> > + * command descriptor in memory.
> > + * Dequeue Pointer Address Registers
> > + * Enqueue Pointer Address Registers
> > + */
> > + qdma_writel(fsl_qdma, temp->bus_addr,
> > + block + FSL_QDMA_BCQDPA_SADDR(i));
> > + qdma_writel(fsl_qdma, temp->bus_addr,
> > + block + FSL_QDMA_BCQEPA_SADDR(i));
> > +
> > + /* Initialize the queue mode. */
> > + reg = FSL_QDMA_BCQMR_EN;
> > + reg |= FSL_QDMA_BCQMR_CD_THLD(ilog2(temp->n_cq)-4);
> > + reg |= FSL_QDMA_BCQMR_CQ_SIZE(ilog2(temp->n_cq)-6);
>
> space around - in the above two lines
>
Sure, thanks.
> > +static enum dma_status fsl_qdma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie, struct dma_tx_state *txstate) {
> > + enum dma_status ret;
> > +
> > + ret = dma_cookie_status(chan, cookie, txstate);
> > + if (ret == DMA_COMPLETE || !txstate)
> > + return ret;
> > +
> > + return ret;
>
> hmmm, this seems same as return dma_cookie_status() so why should we
> have rest of the code
>
Right, here code only fill the struct dma_device member's device_tx_status.
> > +static void fsl_qdma_issue_pending(struct dma_chan *chan) {
> > + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan);
> > + struct fsl_qdma_queue *fsl_queue = fsl_chan->queue;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&fsl_queue->queue_lock, flags);
> > + spin_lock(&fsl_chan->vchan.lock);
> > + if (vchan_issue_pending(&fsl_chan->vchan))
> > + fsl_qdma_enqueue_desc(fsl_chan);
> > + spin_unlock(&fsl_chan->vchan.lock);
> > + spin_unlock_irqrestore(&fsl_queue->queue_lock, flags);
>
> why do we need two locks, and since you are doing vchan why should you add
> your own lock on top
>
Yes, we need two locks.
As you know, the QDMA support multiple virtualized blocks for multi-core support.
so we need to make sure that muliti-core access issues.
Best Regards,
Wen
> ...
>
> Overall the patch has some code style issues which keep catching my eye, can
> you please check them. Also would help to run checkpatch with --strict and
> --codespell option to catch typos and alignment issue. Please beware
> checkpatch is a guide and NOT a rulebook so use your discretion :)
>
> --
> ~Vinod
> --
> 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
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.
> kernel.org%2Fmajordomo-info.html&data=02%7C01%7Cwen.he_1%40nxp.co
> m%7C15adc08d93884118988108d5c532d7ef%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636631744524104561&sdata=0ucY71mrKgPEe8e3
> IhON91zaUeOzTgFVbzRnFPwKwQo%3D&reserved=0
^ permalink raw reply
* [RESEND] dmaengine: pxa: add a default requestor policy
From: Vinod Koul @ 2018-05-29 7:24 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Daniel Mack, Haojian Zhuang, Vinod Koul, linux-arm-kernel,
dmaengine, linux-kernel, Arnd Bergmann
On 26-05-18, 11:54, Robert Jarzmik wrote:
> @@ -762,6 +762,8 @@ static void pxad_free_chan_resources(struct dma_chan *dchan)
> dma_pool_destroy(chan->desc_pool);
> chan->desc_pool = NULL;
>
> + chan->drcmr = (u32)-1;
why not use U32_MAX for this?
^ permalink raw reply
* Revert "dmaengine: pl330: add DMA_PAUSE feature"
From: Vinod Koul @ 2018-05-29 7:09 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Frank Mori Hess, dmaengine, linux-kernel, Dan Williams, r.baldyga,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Linux Samsung SOC
On 29-05-18, 07:17, Marek Szyprowski wrote:
> Hi Vinod,
>
> On 2018-05-18 09:21, Vinod wrote:
> > On 18-05-18, 08:28, Marek Szyprowski wrote:
> >> Hi Vinod,
> >>
> >> Okay, I see that in theory, there are some tricky bits in implementing DMA
> >> support in UART drivers. On the other hand there are already drivers
> >> with seems
> >> to be working fine. This discussion is about revert of the feature
> >> present in
> >> pl330 driver, which is required by other in-kernel driver without proper
> >> fix to
> >> them.
> >>
> >> Can we drop it for now and discuss what and how should be implemented to
> >> make
> >> everyone happy, without any regressions?
> > Sure am dropping the revert, we can always bring it back if it is required
>
> This revert is still in your -next branch. Do you plan to update it as well?
I dont send it to linus and merge topic/* into for-linus and send. It gets
updated on rebase on -rc1.
^ permalink raw reply
* [v5,2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
From: Vinod Koul @ 2018-05-29 7:07 UTC (permalink / raw)
To: Wen He; +Cc: dmaengine, robh+dt, devicetree, leoyang.li, jiafei.pan,
jiaheng.fan
On 25-05-18, 19:19, Wen He wrote:
> +/**
> + * struct fsl_qdma_format - This is the struct holding describing compound
> + * descriptor format with qDMA.
> + * @status: This field which describes command status and
> + * enqueue status notification.
> + * @cfg: This field which describes frame offset and frame
> + * format.
> + * @addr_lo: This field which indicating the start of the buffer
> + * holding the compound descriptor of the lower 32-bits
> + * address in memory 40-bit address.
> + * @addr_hi: This field's the same as above field, but point high
> + * 8-bits in memory 40-bit address.
> + * @__reserved1: Reserved field.
> + * @cfg8b_w1: This field which describes compound descriptor
> + * command queue origin produced by qDMA and dynamic
you may remove 'This field which describes'... in above lines, give reader no
information :)
> + * debug field.
> + * @data Pointer to the memory 40-bit address, describes DMA
> + * source informaion and DMA destination information.
typo informaion
> +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);
> + 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;
the indents here are bad for reading..
> +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) {
> + duplicate = 0;
> + duplicate_handle = 0;
> + reg = qdma_readl(fsl_qdma, block + FSL_QDMA_BSQSR);
> + if (reg & FSL_QDMA_BSQSR_QE)
> + return 0;
> + 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;
> + 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;
code style mandates braces for this as else has braces..
> + else {
> + spin_unlock(&temp_queue->queue_lock);
> + return -EAGAIN;
> + }
> + } else {
> + fsl_comp = list_first_entry(&temp_queue->comp_used,
> + struct fsl_qdma_comp,
> + list);
> + csgf_src = fsl_comp->virt_addr + 2;
> + if (fsl_comp->bus_addr + 16 != pre.addr) {
> + if (duplicate)
> + duplicate_handle = 1;
here as well
> +static irqreturn_t fsl_qdma_error_handler(int irq, void *dev_id)
> +{
> + struct fsl_qdma_engine *fsl_qdma = dev_id;
> + unsigned int intr;
> + void __iomem *status = fsl_qdma->status_base;
> +
> + intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
> +
> + if (intr)
> + dev_err(fsl_qdma->dma_dev.dev, "DMA transaction error!\n");
> +
> + qdma_writel(fsl_qdma, 0xffffffff, status + FSL_QDMA_DEDR);
why unconditional write, was expecting that you would write if intr is non null
> +static int fsl_qdma_reg_init(struct fsl_qdma_engine *fsl_qdma)
> +{
> + struct fsl_qdma_queue *fsl_queue = fsl_qdma->queue;
> + struct fsl_qdma_queue *temp;
> + void __iomem *ctrl = fsl_qdma->ctrl_base;
> + void __iomem *status = fsl_qdma->status_base;
> + void __iomem *block = fsl_qdma->block_base;
> + int i, ret;
> + u32 reg;
> +
> + /* Try to halt the qDMA engine first. */
> + ret = fsl_qdma_halt(fsl_qdma);
> + if (ret) {
> + dev_err(fsl_qdma->dma_dev.dev, "DMA halt failed!");
> + return ret;
> + }
> +
> + /*
> + * Clear the command queue interrupt detect register for all queues.
> + */
> + qdma_writel(fsl_qdma, 0xffffffff, block + FSL_QDMA_BCQIDR(0));
bunch of writes with 0xffffffff, can you explain why? Also helps to make a
macro for this
> +
> + for (i = 0; i < fsl_qdma->n_queues; i++) {
> + temp = fsl_queue + i;
> + /*
> + * Initialize Command Queue registers to point to the first
> + * command descriptor in memory.
> + * Dequeue Pointer Address Registers
> + * Enqueue Pointer Address Registers
> + */
> + qdma_writel(fsl_qdma, temp->bus_addr,
> + block + FSL_QDMA_BCQDPA_SADDR(i));
> + qdma_writel(fsl_qdma, temp->bus_addr,
> + block + FSL_QDMA_BCQEPA_SADDR(i));
> +
> + /* Initialize the queue mode. */
> + reg = FSL_QDMA_BCQMR_EN;
> + reg |= FSL_QDMA_BCQMR_CD_THLD(ilog2(temp->n_cq)-4);
> + reg |= FSL_QDMA_BCQMR_CQ_SIZE(ilog2(temp->n_cq)-6);
space around - in the above two lines
> +static enum dma_status fsl_qdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie, struct dma_tx_state *txstate)
> +{
> + enum dma_status ret;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + if (ret == DMA_COMPLETE || !txstate)
> + return ret;
> +
> + return ret;
hmmm, this seems same as return dma_cookie_status() so why should we have rest
of the code
> +static void fsl_qdma_issue_pending(struct dma_chan *chan)
> +{
> + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan);
> + struct fsl_qdma_queue *fsl_queue = fsl_chan->queue;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&fsl_queue->queue_lock, flags);
> + spin_lock(&fsl_chan->vchan.lock);
> + if (vchan_issue_pending(&fsl_chan->vchan))
> + fsl_qdma_enqueue_desc(fsl_chan);
> + spin_unlock(&fsl_chan->vchan.lock);
> + spin_unlock_irqrestore(&fsl_queue->queue_lock, flags);
why do we need two locks, and since you are doing vchan why should you add your
own lock on top
...
Overall the patch has some code style issues which keep catching my eye, can
you please check them. Also would help to run checkpatch with --strict and
--codespell option to catch typos and alignment issue. Please beware checkpatch
is a guide and NOT a rulebook so use your discretion :)
^ permalink raw reply
* Revert "dmaengine: pl330: add DMA_PAUSE feature"
From: Marek Szyprowski @ 2018-05-29 5:17 UTC (permalink / raw)
To: Vinod
Cc: Frank Mori Hess, dmaengine, linux-kernel, Dan Williams, r.baldyga,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Linux Samsung SOC
Hi Vinod,
On 2018-05-18 09:21, Vinod wrote:
> On 18-05-18, 08:28, Marek Szyprowski wrote:
>> Hi Vinod,
>>
>> Okay, I see that in theory, there are some tricky bits in implementing DMA
>> support in UART drivers. On the other hand there are already drivers
>> with seems
>> to be working fine. This discussion is about revert of the feature
>> present in
>> pl330 driver, which is required by other in-kernel driver without proper
>> fix to
>> them.
>>
>> Can we drop it for now and discuss what and how should be implemented to
>> make
>> everyone happy, without any regressions?
> Sure am dropping the revert, we can always bring it back if it is required
This revert is still in your -next branch. Do you plan to update it as well?
Best regards
^ permalink raw reply
* [v5,1/6] dmaengine: fsldma: Replace DMA_IN/OUT by FSL_DMA_IN/OUT
From: Vinod Koul @ 2018-05-29 4:49 UTC (permalink / raw)
To: Wen He; +Cc: dmaengine, robh+dt, devicetree, leoyang.li, jiafei.pan,
jiaheng.fan
On 25-05-18, 19:19, Wen He wrote:
> This patch implenment a standard macro call functions is
> used to NXP dma drivers.
Well that patch seems to do a bit more than replace DMA_IN/OUT by FSL_DMA_IN/OUT
care to explain those. best would be to do a patch which does replace and then
add new things in separate patch, easier to review
^ permalink raw reply
* [V4,1/2] dmaengine: sprd: Optimize the sprd_dma_prep_dma_memcpy()
From: Vinod Koul @ 2018-05-29 4:45 UTC (permalink / raw)
To: Baolin Wang; +Cc: dan.j.williams, eric.long, broonie, dmaengine, linux-kernel
On 23-05-18, 17:31, Baolin Wang wrote:
> From: Eric Long <eric.long@spreadtrum.com>
>
> This is one preparation patch, we can use default DMA configuration to
> implement the device_prep_dma_memcpy() interface instead of issuing
> sprd_dma_config().
>
> We will implement one new sprd_dma_config() function with introducing
> device_prep_slave_sg() interface in following patch. So we can remove
> the obsolete sprd_dma_config() firstly.
Applied both, thanks
^ permalink raw reply
* dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support
From: Vinod Koul @ 2018-05-28 4:01 UTC (permalink / raw)
To: Angelo Dureghello; +Cc: Vinod Koul, dmaengine, gerg, linux-m68k
Hi Angelo,
On 26-05-18, 22:50, Angelo Dureghello wrote:
> > wouldn't it be easier to just make common parts and then add edma specific code.
> > If I was doing this it would be my apprach and that way code edma specific will
> > be lesser and faster review
> >
>
> I tried to set up a common module, but couldn't reach any good point.
>
> Issues are:
> 1) Edma register set between 32 and 64ch is similar, but some offsets/names
> are not matching between the 2 variants, some registers names are swapped over
> the reg. address range,
> 2) interrupt numbers and scheme is still different, handler implementation comes
> different,
> 3) as a corollary of the above, all the common functions that needs to access
> edma registers should use same structure pointers. I could use a union
> someway but points where register are accessed are many, and i should
> differentiate the access in each case, referencing to a different structure
> in each case.
>
> If you have any idea on how i could reach a common module, with 2 different
> registers set, that's welcome.
> I stay on the thought that a separate 64-channel module is the best
> way to go here.
>
> Currently, as Freescale "edma" variants, i know:
>
> Vybrid VFXXX 32ch DMA multiplexer reg.set 1
> Kynetis K70 (CortexM4) 32ch DMA multiplexer reg.set 1
> imx8xx (coming) 32ch no multiplexer reg.set 1
> MPC57xxk 32ch DMA multiplexer reg.set 1
> ColdFire mcf5441x 64ch no multiplexer reg.set 2 <---
>
> There may me other cpu using this fsl edma module but not in my knowledge
> right now.
>
> So i still think at the end, to have 2 separate drivers for the 32 and 64
> variant is good and probably the most ordered/clean solution.
Okay there are few ways we can do this. One is to use helpers for register
access and these helpers are different for the variant you are loaded on.
Another is to use register offsets which are set based on the variant loaded..
HTH
^ permalink raw reply
* dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support
From: Angelo Dureghello @ 2018-05-26 20:50 UTC (permalink / raw)
To: Vinod; +Cc: Vinod Koul, dmaengine, gerg, linux-m68k
Hi Vinod,
thanks for your support.
On Wed, May 23, 2018 at 11:07:06AM +0530, Vinod wrote:
> On 22-05-18, 23:28, Angelo Dureghello wrote:
> > Hi Vinod,
> >
> > On Mon, May 07, 2018 at 07:45:35PM +0530, Vinod Koul wrote:
> > > On Fri, May 04, 2018 at 09:18:19PM +0200, Angelo Dureghello wrote:
> > > > Hi Vinod,
> > > >
> > > > thanks for the review,
> > > >
> > > > On Thu, May 03, 2018 at 10:18:30PM +0530, Vinod Koul wrote:
> > > > > On Wed, Apr 25, 2018 at 10:08:17PM +0200, Angelo Dureghello wrote:
> > > > > > This patch adds dma support for NXP mcf5441x (ColdFire) family.
> > > > > >
> > > > > > ColdFire mcf5441x implements an edma hw module similar to the
> > > > >
> > > > > Is it similar to to edma ?
> > > > >
> > > >
> > > > It is similar to Freescale "edma" but with a different number of
> > > > channels, a bit different register set, different interrupt
> > > > structure, no channel multiplexer.
> > >
> > > ok
> > >
> > > > > > one implemented in Vybrid VFxxx controllers, but with a slightly
> > > > > > different register set, more dma channels (64 instead of 32),
> > > > > > a different interrupt mechanism and some other minor differences.
> > > > > >
> > > > > > For the above reasons, modfying fsl-edma.c was too complex and
> > > > > > likely too ugly. From here, the decision to create a different
> > > > > > driver, but starting from fsl-edma.
> > > > >
> > > > > can the common stuff be made into a lib and shared between then two rather
> > > > > than having a same driver or different drivers?
> > > >
> > > > It should be possible to collect some common code in a kind of
> > > > mcf_edma_core.c common module, but in this case i cannot then test
> > > > the Vybrid edma after the changes since i miss that hardware.
> > >
> > > Sure you should send the patches and folks who care about fsl driver
> > > would look it up and test
> > >
> > > > Would be maybe possible for you to diff fsl-edma and this mcf-edma,
> > > > just to confirm if i can still stay this way, or if moving to a
> > > > library becomes mandatory ?
> > >
> > > well since you know the IP you would make a better guess on that, best is
> > > to check register sets in drivers
> > >
> > I fixed all the discussed points.
> >
> > Actaully mcf-edma (ColdFire) has a slightly different register set (due to 64
> > channels in place of 16 of fsl-edma) and, for the same reason, a different
> > DMA interrupt structure.
> > Also, i simplified some parts of the driver considering ColdFire (mcf)
> > big endian architecture.
> >
> > So i would send a rev 2 patch with all the fixes, than eventually in a second
> > phase i may try to create some common code, but at least we have the ColdFire
> > DMA. What do you think ?
>
> wouldn't it be easier to just make common parts and then add edma specific code.
> If I was doing this it would be my apprach and that way code edma specific will
> be lesser and faster review
>
I tried to set up a common module, but couldn't reach any good point.
Issues are:
1) Edma register set between 32 and 64ch is similar, but some offsets/names
are not matching between the 2 variants, some registers names are swapped over
the reg. address range,
2) interrupt numbers and scheme is still different, handler implementation comes
different,
3) as a corollary of the above, all the common functions that needs to access
edma registers should use same structure pointers. I could use a union
someway but points where register are accessed are many, and i should
differentiate the access in each case, referencing to a different structure
in each case.
If you have any idea on how i could reach a common module, with 2 different
registers set, that's welcome.
I stay on the thought that a separate 64-channel module is the best
way to go here.
Currently, as Freescale "edma" variants, i know:
Vybrid VFXXX 32ch DMA multiplexer reg.set 1
Kynetis K70 (CortexM4) 32ch DMA multiplexer reg.set 1
imx8xx (coming) 32ch no multiplexer reg.set 1
MPC57xxk 32ch DMA multiplexer reg.set 1
ColdFire mcf5441x 64ch no multiplexer reg.set 2 <---
There may me other cpu using this fsl edma module but not in my knowledge
right now.
So i still think at the end, to have 2 separate drivers for the 32 and 64
variant is good and probably the most ordered/clean solution.
Regards,
Angelo
> --
> ~Vinod
> --
> 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
---
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
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