Add Qualcomm BAM dmaengine driver
Subsystems (3)
v5 cover Add Qualcomm BAM dmaengine driver
This patch set introduces the dmaengine driver for the Qualcomm Bus Access Manager (BAM) DMA controller present on MSM 8x74 devices. A number of the on-chip devices have their own BAM DMA controller and use it to move data between system memory and peripherals or between two peripherals.
The initial version of this driver will only support slave DMA operations between system memory and peripherals.
Changes from v4:
- Add devm_free_irq() to .remove to avoid race condition
- Free FIFO memory in .remove
Changes from v3:
- Remove unused bam_channel_dir.
- Remove incorrect write to BAM_IRQ_SRCS_EE (read only).
- Remove dma direction from DT binding and revise driver to use direction from prep_slave_sg.
- Remove unnecessary channel reset from channel_init. This could affect channels controlled from other execution environments.
- Change terminate_all to also take care of the current active descriptor.
- Rework .remove function to correctly mask interrupts and clean up resources and tasklets.
Changes from v2:
- Corrected Kconfig dependencies
- Moved execution environment ID to controller DT binding. The EE is a global setting across all of the channels on the controller.
- Combined header into source file.
- Corrected copyright date.
- Moved channel hardware initialization to occur when channel is used for the first time.
- Converted dma_alloc_coherent to dma_alloc_writecombine
- Removed unecessary reset of channel from the dma terminate_all
- Corrected usage of EE in irq handler and channel configuration functions.
- Changed resource functions inside probe to use correct APIs.
- Removed dma filter function and modified dma_xlate to use dma_get_slave_channel API
- Fixed various nit comments
Changes from v1:
- Converted driver to use virt-dma
- Reworked probe function per review comments
- tx_status function now computes and returns residuals
- Removed proprietary slave config. Removed associated include file.
- Renamed files to reflect vendor name instead of specific device
- Converted to use (readl|writel)_relaxed w/ appropriate barriers
- Removed unions in favor of standard types.
Andy Gross (2):
dmaengine: add Qualcomm BAM dma driver
dmaengine: qcom_bam_dma: Add device tree binding
.../devicetree/bindings/dma/qcom_bam_dma.txt | 48 + drivers/dma/Kconfig | 9 + drivers/dma/Makefile | 1 + drivers/dma/qcom_bam_dma.c | 1066 ++++++++++++++++++++ 4 files changed, 1124 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt create mode 100644 drivers/dma/qcom_bam_dma.c
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
PATCH v5 1/2 dmaengine: add Qualcomm BAM dma driver
Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller found in the MSM 8x74 platforms.
Each BAM DMA device is associated with a specific on-chip peripheral. Each channel provides a uni-directional data transfer engine that is capable of transferring data between the peripheral and system memory (System mode), or between two peripherals (BAM2BAM).
The initial release of this driver only supports slave transfers between peripherals and system memory.
--- drivers/dma/Kconfig | 9 + drivers/dma/Makefile | 1 + drivers/dma/qcom_bam_dma.c | 1066 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1076 insertions(+) create mode 100644 drivers/dma/qcom_bam_dma.c
drivers/dma/Kconfig +9 -0
@@ -386,4 +386,13 @@ config DMATEST config DMA_ENGINE_RAID bool +config QCOM_BAM_DMA + tristate "QCOM BAM DMA support" + depends on ARCH_MSM_DT || (COMPILE_TEST && OF && ARM) + select DMA_ENGINE + select DMA_VIRTUAL_CHANNELS + ---help--- + Enable support for the QCOM BAM DMA controller. This controller + provides DMA capabilities for a variety of on-chip devices. + endif
drivers/dma/Makefile +1 -0
@@ -42,3 +42,4 @@ obj-$(CONFIG_MMP_PDMA) += mmp_pdma.o obj-$(CONFIG_DMA_JZ4740) += dma-jz4740.o obj-$(CONFIG_TI_CPPI41) += cppi41.o obj-$(CONFIG_K3_DMA) += k3dma.o +obj-$(CONFIG_QCOM_BAM_DMA) += qcom_bam_dma.o
drivers/dma/qcom_bam_dma.c new file +1066 -0
1,066 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver
Quoted 2 lines · Andy Gross
Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller found in the MSM 8x74 platforms.
trivia: fixable later.
drivers/dma/qcom_bam_dma.c
Quoted 1 line
drivers/dma/qcom_bam_dma.c +0
-0
[]
drivers/dma/qcom_bam_dma.c line 488
Quoted 6 lines
drivers/dma/qcom_bam_dma.c +6
-0+/* allocate enough room to accomodate the number of entries */ + async_desc =kzalloc (sizeof (*async_desc) + + (sg_len *sizeof (struct bam_desc_hw )),GFP_NOWAIT ); + +if (!async_desc) { +dev_err (bdev->dev,"failed to allocate async descriptor\n" );
Unnecessary OOM message as generic alloc has an
OOM message with a dump_stack();
drivers/dma/qcom_bam_dma.c line 911
Quoted 2 lines
drivers/dma/qcom_bam_dma.c +2
-0+static int bam_dma_probe (struct platform_device *pdev) +{
[]
drivers/dma/qcom_bam_dma.c line 937
Quoted 3 lines
drivers/dma/qcom_bam_dma.c +3
-0+ ret =clk_prepare_enable (bdev->bamclk); +if (ret) { +dev_err (bdev->dev,"failed to prepare/enable clock" );
Missing terminating \n newline
Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver
In reply to Andy Grossdrivers/dma/qcom_bam_dma.c line 2
Quoted 8 lines · Andy Gross
drivers/dma/qcom_bam_dma.c new file +2
-0@@ -0,0 +1,1066 @@ +/* +* QCOM BAM DMA engine driver
Can you please move this down into the comment below?
drivers/dma/qcom_bam_dma.c line 14
Quoted 12 lines
drivers/dma/qcom_bam_dma.c new file +12
-0+* +* Copyright (c) 2013-2014, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*
And split this out into its own comment block? I think this will make the lawyers happier.
Quoted 18 lines
drivers/dma/qcom_bam_dma.c new file +18
-0+* +* QCOM BAM DMA blocks are distributed amongst a number of the on-chip +* peripherals on the MSM 8x74. The configuration of the channels are dependent +* on the way they are hard wired to that specific peripheral. The peripheral +* device tree entries specify the configuration of each channel. +* +* The DMA controller requires the use of external memory for storage of the +* hardware descriptors for each channel. The descriptor FIFO is accessed as a +* circular buffer and operations are managed according to the offset within the +* FIFO. After pipe/channel reset, all of the pipe registers and internal state +* are back to defaults. +* +* During DMA operations, we write descriptors to the FIFO, being careful to +* handle wrapping and then write the last FIFO offset to that channel's +* P_EVNT_REG register to kick off the transaction. The P_SW_OFSTS register +* indicates the current FIFO offset that is being processed, so there is some +* indication of where the hardware is currently working. +*/
[...]
drivers/dma/qcom_bam_dma.c line 186
Quoted 3 lines
drivers/dma/qcom_bam_dma.c new file +3
-0+ +/* PIPE CTRL */ +#define P_EN BIT(1)
^^^^
Nitpick: Weird tab here?
drivers/dma/qcom_bam_dma.c line 723
Quoted 6 lines
drivers/dma/qcom_bam_dma.c new file +6
-0+ +/** +* bam_start_dma - start next transaction +* @bchan - bam dma channel +* +* Note: must hold bam dma channel vc.lock
You can use lockdep_assert_held() here to document this requirement and test for it at runtime.
Quoted 2 lines
+ */
+static voidbam_start_dma(struct bam_chan *bchan)
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver
In reply to Andy GrossQuoted 15 lines · Andy Gross
Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller found in the MSM 8x74 platforms.
Each BAM DMA device is associated with a specific on-chip peripheral. Each channel provides a uni-directional data transfer engine that is capable of transferring data between the peripheral and system memory (System mode), or between two peripherals (BAM2BAM).
The initial release of this driver only supports slave transfers between peripherals and system memory.
Signed-off-by Andy Gross <[email protected]>+++
b/drivers/dma/qcom_bam_dma.c
drivers/dma/qcom_bam_dma.c line 4
Quoted 4 lines
drivers/dma/qcom_bam_dma.c new file +4
-0@@ -0,0 +1,1066 @@ +/* +* QCOM BAM DMA engine driver +* +* Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
This is bit strange, usually copyright is retained by your employer, but I am fine with it :)
drivers/dma/qcom_bam_dma.c line 55
Quoted 2 lines
drivers/dma/qcom_bam_dma.c new file +2
-0+struct bam_desc_hw { +u32 addr;/* Buffer physical address */
this is where we might have an issue. Is this the DMA register which is 32bit wide or you are descibing it so.
Will this work if you are in 64 bit?
Quoted 12 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+u16 size;/* Buffer size in bytes */ +u16 flags; +}; + +/** +* bam_reset_channel - Reset individual BAM DMA channel +* @bchan: bam channel +* +* This function resets a specific BAM channel +*/
drivers/dma/qcom_bam_dma.c line 316
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+static void bam_reset_channel (struct bam_chan *bchan) +{ +struct bam_device *bdev = bchan->bdev; + +/* reset channel */ +writel_relaxed (1 , bdev->regs +BAM_P_RST (bchan->id)); +writel_relaxed (0 , bdev->regs +BAM_P_RST (bchan->id)); + +/* don't allow reorder of the channel reset */ +wmb ();
Documentation/memory-barriers.txt describes wmb() as a CPU barier but based on above you want it to be a compiler barrier then you should do 1st write, barrier(), second write.
drivers/dma/qcom_bam_dma.c line 320
Quoted 4 lines
drivers/dma/qcom_bam_dma.c new file +4
-0+ +/* make sure hw is initialized when channel is used the first time */ + bchan->initialized =0 ; +}
okay why no locks held while reset?
Quoted 30 lines
drivers/dma/qcom_bam_dma.c new file +30
-0+ +/** +* bam_chan_init_hw - Initialize channel hardware +* @bchan: bam channel +* +* This function resets and initializes the BAM channel +*/ +static void bam_chan_init_hw (struct bam_chan *bchan, +enum dma_transfer_direction dir) +{ +struct bam_device *bdev = bchan->bdev; +u32 val; + +/* Reset the channel to clear internal state of the FIFO */ +bam_reset_channel (bchan); + +/* +* write out 8 byte aligned address. We have enough space for this +* because we allocated 1 more descriptor (8 bytes) than we can use +*/ +writel_relaxed (ALIGN (bchan->fifo_phys,sizeof (struct bam_desc_hw )), + bdev->regs +BAM_P_DESC_FIFO_ADDR (bchan->id)); +writel_relaxed (BAM_DESC_FIFO_SIZE , bdev->regs + +BAM_P_FIFO_SIZES (bchan->id)); + +/* enable the per pipe interrupts, enable EOT, ERR, and INT irqs */ +writel_relaxed (P_DEFAULT_IRQS_EN , bdev->regs +BAM_P_IRQ_EN (bchan->id)); + +/* unmask the specific pipe and EE combo */ + val =readl_relaxed (bdev->regs +BAM_IRQ_SRCS_MSK_EE (bdev->ee));
drivers/dma/qcom_bam_dma.c line 360
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+ val |=BIT (bchan->id); +writel_relaxed (val, bdev->regs +BAM_IRQ_SRCS_MSK_EE (bdev->ee)); + +/* set fixed direction and mode, then enable channel */ + val =P_EN |P_SYS_MODE ; +if (dir ==DMA_DEV_TO_MEM ) + val |=P_DIRECTION ; + +/* make sure the other stores occur before enabling channel */ +wmb ();
here again!
Quoted 23 lines
drivers/dma/qcom_bam_dma.c new file +23
-0+writel_relaxed (val, bdev->regs +BAM_P_CTRL (bchan->id)); + + bchan->initialized =1 ; + +/* init FIFO pointers */ + bchan->head =0 ; + bchan->tail =0 ; +} + +/** +* bam_alloc_chan - Allocate channel resources for DMA channel. +* @chan: specified channel +* +* This function allocates the FIFO descriptor memory +*/ +static int bam_alloc_chan (struct dma_chan *chan) +{ +struct bam_chan *bchan =to_bam_chan (chan); +struct bam_device *bdev = bchan->bdev; + +/* allocate FIFO descriptor space, but only if necessary */ +if (!bchan->fifo_virt) { + bchan->fifo_virt = dma_alloc_writecombine(bdev->dev,
drivers/dma/qcom_bam_dma.c line 393
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+BAM_DESC_FIFO_SIZE , &bchan->fifo_phys, + GFP_KERNEL); + +if (!bchan->fifo_virt) { +dev_err (bdev->dev,"Failed to allocate desc fifo\n" ); +return -ENOMEM ; + } + } + +return BAM_DESC_FIFO_SIZE ;
But you cna have SW descriptors more than HW ones and issue new ones once HW is done with them. Why tie the limit to what HW supports and not create a better driver?
Quoted 9 lines
drivers/dma/qcom_bam_dma.c new file +9
-0+ +/** +* bam_slave_config - set slave configuration for channel +* @chan: dma channel +* @cfg: slave configuration +* +* Sets slave configuration for channel +* +*/
drivers/dma/qcom_bam_dma.c line 448
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+static void bam_slave_config (struct bam_chan *bchan, +struct dma_slave_config *cfg) +{ +struct bam_device *bdev = bchan->bdev; +u32 maxburst; + +if (bchan->slave.direction ==DMA_DEV_TO_MEM ) + maxburst = bchan->slave.src_maxburst = cfg->src_maxburst; +else + maxburst = bchan->slave.dst_maxburst = cfg->dst_maxburst;
can you remove usage of slave.direction have save both. I am going to remove this member...
Quoted 27 lines
drivers/dma/qcom_bam_dma.c new file +27
-0+ +/* set desc threshold */ +writel_relaxed (maxburst, bdev->regs +BAM_DESC_CNT_TRSHLD ); +} + +/** +* bam_prep_slave_sg - Prep slave sg transaction +* +* @chan: dma channel +* @sgl: scatter gather list +* @sg_len: length of sg +* @direction: DMA transfer direction +* @flags: DMA flags +* @context: transfer context (unused) +*/ +static struct dma_async_tx_descriptor *bam_prep_slave_sg (struct dma_chan *chan, +struct scatterlist *sgl,unsigned int sg_len, +enum dma_transfer_direction direction,unsigned long flags, +void *context) +{ +struct bam_chan *bchan =to_bam_chan (chan); +struct bam_device *bdev = bchan->bdev; +struct bam_async_desc *async_desc; +struct scatterlist *sg; +u32 i; +struct bam_desc_hw *desc; +
drivers/dma/qcom_bam_dma.c line 485
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+ +if (!is_slave_direction (direction)) { +dev_err (bdev->dev,"invalid dma direction\n" ); +return NULL ; + } + + +/* allocate enough room to accomodate the number of entries */ + async_desc =kzalloc (sizeof (*async_desc) + + (sg_len *sizeof (struct bam_desc_hw )),GFP_NOWAIT );
this seems to assume that each sg entry length will not exceed the HW capablity. Does engine support any length descriptor, if not you may want to split to multiple.
Quoted 6 lines
drivers/dma/qcom_bam_dma.c new file +6
-0+ +if (!async_desc) { +dev_err (bdev->dev,"failed to allocate async descriptor\n" ); + goto err_out; + } +
drivers/dma/qcom_bam_dma.c line 501
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+ async_desc->num_desc = sg_len; + async_desc->curr_desc = async_desc->desc; + async_desc->dir = direction; + +/* fill in descriptors, align hw descriptor to 8 bytes */ + desc = async_desc->desc; +for_each_sg (sgl, sg, sg_len, i) { +if (sg_dma_len (sg) >BAM_MAX_DATA_SIZE ) { +dev_err (bdev->dev,"segment exceeds max size\n" ); + goto err_out;
gottcha, okay why not split here to multiple descriptors with max size all but last?
Quoted 29 lines
drivers/dma/qcom_bam_dma.c new file +29
-0+ } + + desc->addr =sg_dma_address (sg); + desc->size =sg_dma_len (sg); + async_desc->length +=sg_dma_len (sg); + desc++; + } + +return vchan_tx_prep (&bchan->vc, &async_desc->vd, flags); + +err_out: +kfree (async_desc); +return NULL ; +} + +/** +* bam_dma_terminate_all - terminate all transactions on a channel +* @bchan: bam dma channel +* +* Dequeues and frees all transactions +* No callbacks are done +* +*/ +static void bam_dma_terminate_all (struct bam_chan *bchan) +{ +unsigned long flag; +LIST_HEAD (head); + +/* remove all transactions, including active transaction */
drivers/dma/qcom_bam_dma.c line 540
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+spin_lock_irqsave (&bchan->vc.lock, flag); +if (bchan->curr_txd) { +list_add (&bchan->curr_txd->vd.node, &bchan->vc.desc_issued); + bchan->curr_txd =NULL ; + } + +vchan_get_all_descriptors (&bchan->vc, &head); +spin_unlock_irqrestore (&bchan->vc.lock, flag); + +vchan_dma_desc_free_list (&bchan->vc, &head);
why drop the lock here, i dont think this one needs to be called with lock dropped, didnt see it garbbing.
Quoted 32 lines
drivers/dma/qcom_bam_dma.c new file +32
-0+} + +/** +* bam_control - DMA device control +* @chan: dma channel +* @cmd: control cmd +* @arg: cmd argument +* +* Perform DMA control command +* +*/ +static int bam_control (struct dma_chan *chan,enum dma_ctrl_cmd cmd, +unsigned long arg) +{ +struct bam_chan *bchan =to_bam_chan (chan); +struct bam_device *bdev = bchan->bdev; +int ret =0 ; +unsigned long flag; + +switch (cmd) { +case DMA_PAUSE : +spin_lock_irqsave (&bchan->vc.lock, flag); +writel_relaxed (1 , bdev->regs +BAM_P_HALT (bchan->id)); + bchan->paused =1 ; +spin_unlock_irqrestore (&bchan->vc.lock, flag); +break ; +case DMA_RESUME : +spin_lock_irqsave (&bchan->vc.lock, flag); +writel_relaxed (0 , bdev->regs +BAM_P_HALT (bchan->id)); + bchan->paused =0 ; +spin_unlock_irqrestore (&bchan->vc.lock, flag); +break ;
drivers/dma/qcom_bam_dma.c line 582
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+case DMA_TERMINATE_ALL : +bam_dma_terminate_all (bchan); +break ; +case DMA_SLAVE_CONFIG : +bam_slave_config (bchan, (struct dma_slave_config *)arg); +break ; +default : + ret = -ENXIO ; +break ; + }
empty line after each break would be appreciated.
Quoted 43 lines
drivers/dma/qcom_bam_dma.c new file +42
-0+ +return ret; +} + +/** +* bam_start_dma - start next transaction +* @bchan - bam dma channel +* +* Note: must hold bam dma channel vc.lock +*/ +static void bam_start_dma (struct bam_chan *bchan) +{ +struct virt_dma_desc *vd =vchan_next_desc (&bchan->vc); +struct bam_device *bdev = bchan->bdev; +struct bam_async_desc *async_desc; +struct bam_desc_hw *desc; +struct bam_desc_hw *fifo =PTR_ALIGN (bchan->fifo_virt, +sizeof (struct bam_desc_hw )); + +if (!vd) +return ; + +list_del (&vd->node); + + async_desc = container_of(vd, struct bam_async_desc, vd); + bchan->curr_txd = async_desc; + +/* on first use, initialize the channel hardware */ +if (!bchan->initialized) +bam_chan_init_hw (bchan, async_desc->dir); + + + desc = bchan->curr_txd->curr_desc; + +if (async_desc->num_desc >MAX_DESCRIPTORS ) + async_desc->xfer_len =MAX_DESCRIPTORS ; +else + async_desc->xfer_len = async_desc->num_desc; + +/* set INT on last descriptor */ + desc[async_desc->xfer_len -1 ].flags |=DESC_FLAG_INT ; +
drivers/dma/qcom_bam_dma.c line 766
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+if (bchan->tail + async_desc->xfer_len >MAX_DESCRIPTORS ) { +u32 partial =MAX_DESCRIPTORS - bchan->tail; + +memcpy (&fifo[bchan->tail], desc, + partial *sizeof (struct bam_desc_hw )); +memcpy (fifo, &desc[partial], (async_desc->xfer_len - partial) * +sizeof (struct bam_desc_hw )); + }else { +memcpy (&fifo[bchan->tail], desc, + async_desc->xfer_len *sizeof (struct bam_desc_hw ));
where is the destination memeory located, device or system memory. I think later, right?
drivers/dma/qcom_bam_dma.c line 777
Quoted 11 lines
drivers/dma/qcom_bam_dma.c new file +11
-0+ } + + bchan->tail += async_desc->xfer_len; + bchan->tail %=MAX_DESCRIPTORS ; + +/* ensure descriptor writes and dma start not reordered */ +wmb (); +writel_relaxed (bchan->tail *sizeof (struct bam_desc_hw ), + bdev->regs +BAM_P_EVNT_REG (bchan->id)); +} +
Overall driver loooks fine mostly with few comments as above.
And yes for 2nd patch, would need someone to ACK it before we can appply this
Signature · 1 line
~Vinod
Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver
Quoted 15 lines · Vinod Koul
Quoted 14 lines · Andy Gross
Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller found in the MSM 8x74 platforms.
Each BAM DMA device is associated with a specific on-chip peripheral. Each channel provides a uni-directional data transfer engine that is capable of transferring data between the peripheral and system memory (System mode), or between two peripherals (BAM2BAM).
The initial release of this driver only supports slave transfers between peripherals and system memory.
Signed-off-by Andy Gross <[email protected]>+++
b/drivers/dma/qcom_bam_dma.c
[..]
Quoted 5 lines
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+static void bam_reset_channel (struct bam_chan *bchan) +{ +struct bam_device *bdev = bchan->bdev; + +/* reset channel */
Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 316
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+writel_relaxed (1 , bdev->regs +BAM_P_RST (bchan->id)); +writel_relaxed (0 , bdev->regs +BAM_P_RST (bchan->id)); + +/* don't allow reorder of the channel reset */ +wmb ();
Documentation/memory-barriers.txtdescribeswmb()as a CPU barier but based on above you want it to be a compiler barrier then you should do 1st write,barrier(), second write.
It could also be that the intent was to prevent these writes from being ordered before setting the initialized flag below, either way the comment could be made clearer.
Quoted 4 lines
Quoted 4 lines
drivers/dma/qcom_bam_dma.c new file +4
-0+ +/* make sure hw is initialized when channel is used the first time */ + bchan->initialized =0 ; +}
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver
Ugh.
Quoted 31 lines · Josh Cartwright
Quoted 15 lines · Vinod Koul
Quoted 14 lines · Andy Gross
Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller found in the MSM 8x74 platforms.
Each BAM DMA device is associated with a specific on-chip peripheral. Each channel provides a uni-directional data transfer engine that is capable of transferring data between the peripheral and system memory (System mode), or between two peripherals (BAM2BAM).
The initial release of this driver only supports slave transfers between peripherals and system memory.
Signed-off-by Andy Gross <[email protected]>+++
b/drivers/dma/qcom_bam_dma.c[..]
Quoted 5 lines
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+static void bam_reset_channel (struct bam_chan *bchan) +{ +struct bam_device *bdev = bchan->bdev; + +/* reset channel */ Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 316
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+writel_relaxed (1 , bdev->regs +BAM_P_RST (bchan->id)); +writel_relaxed (0 , bdev->regs +BAM_P_RST (bchan->id)); + +/* don't allow reorder of the channel reset */ +wmb ();
Documentation/memory-barriers.txtdescribeswmb()as a CPU barier but based on above you want it to be a compiler barrier then you should do 1st write,barrier(), second write.
Quoted 2 lines
It could also be that the intent was to prevent these writes from being ordered before setting the initialized flag below, either way the
^ after
Quoted 1 line
comment could be made clearer.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver
In reply to Josh CartwrightQuoted 31 lines · Josh Cartwright
Quoted 15 lines · Vinod Koul
Quoted 14 lines · Andy Gross
Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller found in the MSM 8x74 platforms.
Each BAM DMA device is associated with a specific on-chip peripheral. Each channel provides a uni-directional data transfer engine that is capable of transferring data between the peripheral and system memory (System mode), or between two peripherals (BAM2BAM).
The initial release of this driver only supports slave transfers between peripherals and system memory.
Signed-off-by Andy Gross <[email protected]>+++
b/drivers/dma/qcom_bam_dma.c[..]
Quoted 5 lines
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+static void bam_reset_channel (struct bam_chan *bchan) +{ +struct bam_device *bdev = bchan->bdev; + +/* reset channel */ Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 316
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+writel_relaxed (1 , bdev->regs +BAM_P_RST (bchan->id)); +writel_relaxed (0 , bdev->regs +BAM_P_RST (bchan->id)); + +/* don't allow reorder of the channel reset */ +wmb ();
Documentation/memory-barriers.txtdescribeswmb()as a CPU barier but based on above you want it to be a compiler barrier then you should do 1st write,barrier(), second write.
Quoted 3 lines
It could also be that the intent was to prevent these writes from being ordered before setting the initialized flag below, either way the comment could be made clearer.
yes for that case, but i am suspecting the comment is correct as it would make sense to ensure reset is in sequence...
Signature · 1 line
~Vinod
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +4
-0+ +/* make sure hw is initialized when channel is used the first time */ + bchan->initialized =0 ; +}
Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver
In reply to Vinod KoulQuoted 4 lines · Vinod Koul
Quoted 3 lines · Andy Gross
Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller found in the MSM 8x74 platforms.
[.....]
Quoted 5 lines
Quoted 3 lines
+ * QCOM BAM DMA engine driver
+ *
+ * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.This is bit strange, usually copyright is retained by your employer, but I am fine with it :)
The short answer is that this is due to legal requirements.
Quoted 5 lines
drivers/dma/qcom_bam_dma.c line 55
Quoted 2 lines
drivers/dma/qcom_bam_dma.c new file +2
-0+struct bam_desc_hw { +u32 addr;/* Buffer physical address */ this is where we might have an issue. Is this the DMA register which is 32bit wide or you are descibing it so.
Will this work if you are in 64 bit?
In later implementations, they add 6 more bits to the address field from some of the reserved bits in the flags. I am not sure what the solution will be for 64 bit, but it may very well be a slightly different descriptor. We'll have to address this when we add support for chips requiring that functionality.
Quoted 3 lines
Quoted 3 lines
drivers/dma/qcom_bam_dma.c new file +3
-0+u16 size;/* Buffer size in bytes */ +u16 flags; +};
[....]
Quoted 10 lines
drivers/dma/qcom_bam_dma.c line 316
Quoted 7 lines
drivers/dma/qcom_bam_dma.c new file +7
-0+ +/* reset channel */ +writel_relaxed (1 , bdev->regs +BAM_P_RST (bchan->id)); +writel_relaxed (0 , bdev->regs +BAM_P_RST (bchan->id)); + +/* don't allow reorder of the channel reset */ +wmb ();
Documentation/memory-barriers.txtdescribeswmb()as a CPU barier but based on above you want it to be a compiler barrier then you should do 1st write,barrier(), second write.
Sorry, the comment was not clear. I need to reword this. On the Krait, accesses within a 1k band are not allowed to be reordered. This memory barrier is to make sure that no reordering is allowed past the call to this function.
Quoted 5 lines
drivers/dma/qcom_bam_dma.c line 320
Quoted 4 lines
drivers/dma/qcom_bam_dma.c new file +4
-0+ +/* make sure hw is initialized when channel is used the first time */ + bchan->initialized =0 ; +}okay why no locks held while reset?
This function is called from 2 places. One is during the freeing of the channel. The other is when starting the channel (protected via vc.lock). I was thinking this was safe, but it's easy enough to add a lock around the bam_reset_channel in the bam_chan_init_hw() and then add a comment to the reset function saying that a lock is required.
[.....]
Quoted 9 lines
drivers/dma/qcom_bam_dma.c line 360
Quoted 8 lines
drivers/dma/qcom_bam_dma.c new file +8
-0+ +/* set fixed direction and mode, then enable channel */ + val =P_EN |P_SYS_MODE ; +if (dir ==DMA_DEV_TO_MEM ) + val |=P_DIRECTION ; + +/* make sure the other stores occur before enabling channel */ +wmb ();here again!
I need to reword. The various registers are over 1K boundaries, so the barrier is required to make sure that all the settings are not allowed to be reordered past the channel enable.
Quoted 28 lines
Quoted 28 lines
drivers/dma/qcom_bam_dma.c new file +28
-0+writel_relaxed (val, bdev->regs +BAM_P_CTRL (bchan->id)); + + bchan->initialized =1 ; + +/* init FIFO pointers */ + bchan->head =0 ; + bchan->tail =0 ; +} + +/** +* bam_alloc_chan - Allocate channel resources for DMA channel. +* @chan: specified channel +* +* This function allocates the FIFO descriptor memory +*/ +static int bam_alloc_chan (struct dma_chan *chan) +{ +struct bam_chan *bchan =to_bam_chan (chan); +struct bam_device *bdev = bchan->bdev; + +/* allocate FIFO descriptor space, but only if necessary */ +if (!bchan->fifo_virt) { + bchan->fifo_virt =dma_alloc_writecombine (bdev->dev, +BAM_DESC_FIFO_SIZE , &bchan->fifo_phys, +GFP_KERNEL ); + +if (!bchan->fifo_virt) { +dev_err (bdev->dev,"Failed to allocate desc fifo\n" );
Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 393
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+return -ENOMEM ; + } + } + +return BAM_DESC_FIFO_SIZE ;But you cna have SW descriptors more than HW ones and issue new ones once HW is done with them. Why tie the limit to what HW supports and not create a better driver?
Given that the virt-dma only allows me to have one outstanding transaction that consumes the fifo, and that I allocate descriptors from kzalloc, this would be as many as you can get until you go OOM.
The slave_sg() doesn't pull from a pool of descriptors. It uses kzalloc. So what value should I use for return value? Most drivers use 1.
[....]
Quoted 4 lines
Quoted 4 lines
drivers/dma/qcom_bam_dma.c new file +4
-0+static void bam_slave_config (struct bam_chan *bchan, +struct dma_slave_config *cfg) +{ +struct bam_device *bdev = bchan->bdev;
Quoted 9 lines
drivers/dma/qcom_bam_dma.c line 448
Quoted 6 lines
drivers/dma/qcom_bam_dma.c new file +6
-0+u32 maxburst; + +if (bchan->slave.direction ==DMA_DEV_TO_MEM ) + maxburst = bchan->slave.src_maxburst = cfg->src_maxburst; +else + maxburst = bchan->slave.dst_maxburst = cfg->dst_maxburst;can you remove usage of slave.direction have save both. I am going to remove this member...
Yes. no problem.
[....]
Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 485
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+ + +/* allocate enough room to accomodate the number of entries */ + async_desc =kzalloc (sizeof (*async_desc) + + (sg_len *sizeof (struct bam_desc_hw )),GFP_NOWAIT );this seems to assume that each sg entry length will not exceed the HW capablity. Does engine support any length descriptor, if not you may want to split to multiple.
Isn't this what the dma_set_max_seg_size supposed to fix? The client is not supposed to send segments larger than the max segment you can take. If that is true, then I have no issues.
Quoted 10 lines
Quoted 10 lines
drivers/dma/qcom_bam_dma.c new file +10
-0+ +if (!async_desc) { +dev_err (bdev->dev,"failed to allocate async descriptor\n" ); + goto err_out; + } + + async_desc->num_desc = sg_len; + async_desc->curr_desc = async_desc->desc; + async_desc->dir = direction; +
Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 501
Quoted 6 lines
drivers/dma/qcom_bam_dma.c new file +6
-0+/* fill in descriptors, align hw descriptor to 8 bytes */ + desc = async_desc->desc; +for_each_sg (sgl, sg, sg_len, i) { +if (sg_dma_len (sg) >BAM_MAX_DATA_SIZE ) { +dev_err (bdev->dev,"segment exceeds max size\n" ); + goto err_out;gottcha, okay why not split here to multiple descriptors with max size all but last?
The client exceeded my dma_max_seg_size. I reject that. If that is not an allowed solution, then I have to fix my kzalloc size above and split it here.
[....]
Quoted 7 lines
drivers/dma/qcom_bam_dma.c line 540
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+ +vchan_get_all_descriptors (&bchan->vc, &head); +spin_unlock_irqrestore (&bchan->vc.lock, flag); + +vchan_dma_desc_free_list (&bchan->vc, &head);why drop the lock here, i dont think this one needs to be called with lock dropped, didnt see it garbbing.
It doesn't need lock. This func just iterates over the head and calls the desc_free func.
[....]
Quoted 3 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ break; + }empty line after each break would be appreciated.
Ok.
[....]
Quoted 4 lines
Quoted 4 lines
drivers/dma/qcom_bam_dma.c new file +4
-0+if (bchan->tail + async_desc->xfer_len >MAX_DESCRIPTORS ) { +u32 partial =MAX_DESCRIPTORS - bchan->tail; + + memcpy(&fifo[bchan->tail], desc,
Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 766
Quoted 6 lines
drivers/dma/qcom_bam_dma.c new file +6
-0+ partial * sizeof(struct bam_desc_hw)); +memcpy (fifo, &desc[partial], (async_desc->xfer_len - partial) * +sizeof (struct bam_desc_hw )); + }else { +memcpy (&fifo[bchan->tail], desc, + async_desc->xfer_len *sizeof (struct bam_desc_hw ));where is the destination memeory located, device or system memory. I think later, right?
The destination is the system memory being used for the hw fifo. This was allocated using dma_alloc_writecombine.
Quoted 14 lines
drivers/dma/qcom_bam_dma.c line 777
Quoted 11 lines
drivers/dma/qcom_bam_dma.c new file +11
-0+ } + + bchan->tail += async_desc->xfer_len; + bchan->tail %=MAX_DESCRIPTORS ; + +/* ensure descriptor writes and dma start not reordered */ +wmb (); +writel_relaxed (bchan->tail *sizeof (struct bam_desc_hw ), + bdev->regs +BAM_P_EVNT_REG (bchan->id)); +} +Overall driver loooks fine mostly with few comments as above.
Quoted 1 line
And yes for 2nd patch, would need someone to ACK it before we can appply this
Thanks for the comments. I'll get those done and see about getting the DT ACK'd.
Signature · 3 lines
sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver
Quoted 34 lines · Andy Gross
Quoted 24 lines · Vinod Koul
Quoted 23 lines · Andy Gross
Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller found in the MSM 8x74 platforms.
drivers/dma/qcom_bam_dma.c new file +19
-0+/** +* bam_alloc_chan - Allocate channel resources for DMA channel. +* @chan: specified channel +* +* This function allocates the FIFO descriptor memory +*/ +static int bam_alloc_chan (struct dma_chan *chan) +{ +struct bam_chan *bchan =to_bam_chan (chan); +struct bam_device *bdev = bchan->bdev; + +/* allocate FIFO descriptor space, but only if necessary */ +if (!bchan->fifo_virt) { + bchan->fifo_virt =dma_alloc_writecombine (bdev->dev, +BAM_DESC_FIFO_SIZE , &bchan->fifo_phys, +GFP_KERNEL ); + +if (!bchan->fifo_virt) { +dev_err (bdev->dev,"Failed to allocate desc fifo\n" );Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 393
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+return -ENOMEM ; + } + } + +return BAM_DESC_FIFO_SIZE ;But you cna have SW descriptors more than HW ones and issue new ones once HW is done with them. Why tie the limit to what HW supports and not create a better driver?
Quoted 3 lines
Given that the virt-dma only allows me to have one outstanding transaction that consumes the fifo, and that I allocate descriptors from kzalloc, this would be as many as you can get until you go OOM.
well you can update virt-dma to queue up multiple descriptors to HW is thats supported :)
Quoted 3 lines
The
slave_sg()doesn't pull from a pool of descriptors. It uses kzalloc. So what value should I use for return value? Most drivers use 1.
Lots do 0 as they dont allocate descriptor here, they allocate when the .prep_xxx call gets invoked, which is what I suspect from you case too.
Quoted 30 lines
[....]
Quoted 4 lines
Quoted 4 lines
drivers/dma/qcom_bam_dma.c new file +4
-0+static void bam_slave_config (struct bam_chan *bchan, +struct dma_slave_config *cfg) +{ +struct bam_device *bdev = bchan->bdev;Quoted 9 lines
drivers/dma/qcom_bam_dma.c line 448
Quoted 6 lines
drivers/dma/qcom_bam_dma.c new file +6
-0+u32 maxburst; + +if (bchan->slave.direction ==DMA_DEV_TO_MEM ) + maxburst = bchan->slave.src_maxburst = cfg->src_maxburst; +else + maxburst = bchan->slave.dst_maxburst = cfg->dst_maxburst;can you remove usage of slave.direction have save both. I am going to remove this member...
Yes. no problem.
[....]
Quoted 8 lines
drivers/dma/qcom_bam_dma.c line 485
Quoted 5 lines
drivers/dma/qcom_bam_dma.c new file +5
-0+ + +/* allocate enough room to accomodate the number of entries */ + async_desc =kzalloc (sizeof (*async_desc) + + (sg_len *sizeof (struct bam_desc_hw )),GFP_NOWAIT );this seems to assume that each sg entry length will not exceed the HW capablity. Does engine support any length descriptor, if not you may want to split to multiple.
Quoted 3 lines
Isn't this what the
dma_set_max_seg_sizesupposed to fix? The client is not supposed to send segments larger than the max segment you can take. If that is true, then I have no issues.
Thats is true too, but do we want to limit this in driver ? This is more of a SW limitation of who breaks up. for some like audio it makese sense but other not too sure...
Signature · 1 line
~Vinod
PATCH v5 2/2 dmaengine: qcom_bam_dma: Add device tree binding
In reply to Andy GrossAdd device tree binding support for the QCOM BAM DMA driver.
--- .../devicetree/bindings/dma/qcom_bam_dma.txt | 48 ++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
Documentation/devicetree/bindings/dma/qcom_bam_dma.txt new file +48 -0
@@ -0,0 +1,48 @@ +QCOM BAM DMA controller + +Required properties: +- compatible: Must be "qcom,bam-v1.4.0" for MSM8974 V1 + Must be "qcom,bam-v1.4.1" for MSM8974 V2 +- reg: Address range for DMA registers +- interrupts: single interrupt for this controller +- #dma-cells: must be <1> +- clocks: required clock +- clock-names: name of clock +- qcom,ee : indicates the active Execution Environment identifier (0-7) + +Example: + + uart-bam: dma at f9984000 = { + compatible = "qcom,bam-v1.4.1"; + reg = <0xf9984000 0x15000>; + interrupts = <0 94 0>; + clocks = <&gcc GCC_BAM_DMA_AHB_CLK>; + clock-names = "bam_clk"; + #dma-cells = <1>; + qcom,ee = <0>; + }; + +Client: +Required properties: +- dmas: List of dma channel requests +- dma-names: Names of aforementioned requested channels + +Clients must use the format described in the dma.txt file, using a two cell +specifier for each channel. + +The three cells in order are: + 1. A phandle pointing to the DMA controller + 2. The channel number + +Example: + serial at f991e000 { + compatible = "qcom,msm-uart"; + reg = <0xf991e000 0x1000> + <0xf9944000 0x19000>; + interrupts = <0 108 0>; + clocks = <&gcc GCC_BLSP1_UART2_APPS_CLK>, <&gcc GCC_BLSP1_AHB_CLK>; + clock-names = "core", "iface"; + + dmas = <&uart-bam 0>, <&uart-bam 1>; + dma-names = "rx", "tx"; + };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Re: [Patch v5 2/2] dmaengine: qcom_bam_dma: Add device tree binding
Quoted 7 lines · Andy Gross
Add device tree binding support for the QCOM BAM DMA driver.
Signed-off-by Andy Gross <[email protected]>---
.../devicetree/bindings/dma/qcom_bam_dma.txt| 48 ++++++++++++++++++++1 file changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
- k
Quoted 63 lines
Documentation/devicetree/bindings/dma/qcom_bam_dma.txt new file +48
-0@@ -0,0 +1,48 @@ +QCOM BAM DMA controller + +Required properties: +- compatible: Must be "qcom,bam-v1.4.0" for MSM8974 V1 + Must be "qcom,bam-v1.4.1" for MSM8974 V2 +- reg: Address range for DMA registers +- interrupts: single interrupt for this controller +- #dma-cells: must be <1> +- clocks: required clock +- clock-names: name of clock +- qcom,ee : indicates the active Execution Environment identifier (0-7) + +Example: + + uart-bam: dma at f9984000 = { + compatible = "qcom,bam-v1.4.1"; + reg = <0xf9984000 0x15000>; + interrupts = <0 94 0>; + clocks = <&gcc GCC_BAM_DMA_AHB_CLK>; + clock-names = "bam_clk"; + #dma-cells = <1>; + qcom,ee = <0>; + }; + +Client: +Required properties: +- dmas: List of dma channel requests +- dma-names: Names of aforementioned requested channels + +Clients must use the format described in the dma.txt file, using a two cell +specifier for each channel. + +The three cells in order are: + 1. A phandle pointing to the DMA controller + 2. The channel number + +Example: + serial at f991e000 { + compatible = "qcom,msm-uart"; + reg = <0xf991e000 0x1000> + <0xf9944000 0x19000>; + interrupts = <0 108 0>; + clocks = <&gcc GCC_BLSP1_UART2_APPS_CLK>, <&gcc GCC_BLSP1_AHB_CLK>; + clock-names = "core", "iface"; + + dmas = <&uart-bam 0>, <&uart-bam 1>; + dma-names = "rx", "tx"; + };Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation