Re: [PATCH] dmaengine: bcm2835: Add slave dma support

[Stefan Wahren <info@lategoodbye.de>]

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