Messages in this thread | | | Date | Wed, 15 Apr 2015 21:00:26 +0200 | From | Stefan Wahren <> | Subject | Re: [PATCH] dmaengine: bcm2835: Add slave dma support |
| |
Hi Noralf,
Am 15.04.2015 um 11:56 schrieb Noralf Trønnes: > Add slave transfer capability to BCM2835 dmaengine driver. > This patch is pulled from the bcm2708-dmaengine driver in the > Raspberry Pi repo. The work was done by Gellert Weisz. > > Tested with the bcm2835-mmc driver from the same repo.
why not with the upstream kernel?
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > > Gellert Weisz has ended his internship with Raspberry Pi Trading and > was not available to sign off this patch. > > Changes from original code: > Remove slave_id use. > SDHCI_BCM_DMA_WAITS changed to BCM2835_DMA_WAIT_CYCLES. > Use SZ_* macros instead of decimal values. > Change MAX_LITE_TRANSFER from 32k to 64K - 1. > Fix several whitespace issues. > > Lee Jones' comments in previous email to Piotr Król: > Remove __func__ from dev_err message. > Cleanup comments. > > > Quick testing: > Enable CONFIG_DMA_BCM2835 > Apply patch: https://gist.github.com/notro/0c9184df1b43a793f006 > > > drivers/dma/bcm2835-dma.c | 200 ++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 186 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c > index c92d6a7..4230aac 100644 > --- a/drivers/dma/bcm2835-dma.c > +++ b/drivers/dma/bcm2835-dma.c > @@ -1,11 +1,10 @@ > [...] > + > +static struct dma_async_tx_descriptor * > +bcm2835_dma_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 bcm2835_chan *c = to_bcm2835_dma_chan(chan); > + enum dma_slave_buswidth dev_width; > + struct bcm2835_desc *d; > + dma_addr_t dev_addr; > + struct scatterlist *sgent; > + unsigned int es, sync_type; > + unsigned int i, j, splitct, max_size;
I think "split_cnt" would be better.
> + > + if (!is_slave_direction(direction)) { > + dev_err(chan->device->dev, "direction not supported\n"); > + return NULL; > + } > + > + if (direction == DMA_DEV_TO_MEM) { > + dev_addr = c->cfg.src_addr; > + dev_width = c->cfg.src_addr_width; > + sync_type = BCM2835_DMA_S_DREQ; > + } else { > + dev_addr = c->cfg.dst_addr; > + dev_width = c->cfg.dst_addr_width; > + sync_type = BCM2835_DMA_D_DREQ; > + } > + > + /* Bus width translates to the element size (ES) */ > + switch (dev_width) { > + case DMA_SLAVE_BUSWIDTH_4_BYTES: > + es = BCM2835_DMA_DATA_TYPE_S32;
Looks like "es" is never used.
> + break; > + default:
A dev_err() might be useful here.
> + return NULL; > + } > + > + /* Allocate and setup the descriptor. */ > + d = kzalloc(sizeof(*d), GFP_NOWAIT); > + if (!d) > + return NULL; > + > + d->dir = direction; > + > + if (c->ch >= 8) /* LITE channel */ > + max_size = MAX_LITE_TRANSFER; > + else > + max_size = MAX_NORMAL_TRANSFER; > + > + /* > + * Store the length of the SG list in d->frames > + * taking care to account for splitting up transfers > + * too large for a LITE channel > + */ > + d->frames = 0; > + for_each_sg(sgl, sgent, sg_len, i) { > + unsigned int len = sg_dma_len(sgent); > + > + d->frames += 1 + len / max_size;
If it's correct this should be more intuitive:
d->frames += len / max_size + 1;
> + } > + > + /* Allocate memory for control blocks */ > + d->control_block_size = d->frames * sizeof(struct bcm2835_dma_cb); > + d->control_block_base = dma_zalloc_coherent(chan->device->dev, > + d->control_block_size, &d->control_block_base_phys, > + GFP_NOWAIT); > + if (!d->control_block_base) { > + kfree(d); > + return NULL; > + } > + > + /* > + * Iterate over all SG entries, create a control block > + * for each frame and link them together. > + * Count the number of times an SG entry had to be split > + * as a result of using a LITE channel > + */ > + splitct = 0; > + > + for_each_sg(sgl, sgent, sg_len, i) { > + dma_addr_t addr = sg_dma_address(sgent); > + unsigned int len = sg_dma_len(sgent); > + > + for (j = 0; j < len; j += max_size) {
It should be possible to move declaration of "j" down here.
> + struct bcm2835_dma_cb *control_block = > + &d->control_block_base[i + splitct]; > + > + /* Setup addresses */ > + if (d->dir == DMA_DEV_TO_MEM) { > + control_block->info = BCM2835_DMA_D_INC | > + BCM2835_DMA_D_WIDTH | > + BCM2835_DMA_S_DREQ; > + control_block->src = dev_addr; > + control_block->dst = addr + (dma_addr_t)j; > + } else { > + control_block->info = BCM2835_DMA_S_INC | > + BCM2835_DMA_S_WIDTH | > + BCM2835_DMA_D_DREQ; > + control_block->src = addr + (dma_addr_t)j; > + control_block->dst = dev_addr; > + } > + > + /* Common part */ > + control_block->info |= > + BCM2835_DMA_WAITS(BCM2835_DMA_WAIT_CYCLES); > + control_block->info |= BCM2835_DMA_WAIT_RESP; > + > + /* Enable */ > + if (i == sg_len - 1 && len - j <= max_size) > + control_block->info |= BCM2835_DMA_INT_EN; > + > + /* Setup synchronization */ > + if (sync_type != 0)
if (sync_type) ?
> + control_block->info |= sync_type; > + > + /* Setup DREQ channel */ > + if (c->dreq) > + control_block->info |= > + BCM2835_DMA_PER_MAP(c->dreq); > +
Best regards Stefan
| |