Re: [PATCH 4/7] dmaengine: rcar-dmac: Add Renesas R-Car Gen2 DMA Controller (DMAC) driver

[Kuninori Morimoto <kuninori.morimoto.gx@gmail.com>]

Hi Laurent

Thank you for your patch

> The DMAC is a general purpose multi-channel DMA controller that supports
> both slave and memcpy transfers.
> 
> The driver currently supports the DMAC found in the r8a7790 and r8a7791
> SoCs. Support for compatible DMA controllers (such as the audio DMAC)
> will be added later.
> 
> Feature-wise, automatic hardware handling of descriptors chains isn't
> supported yet. LPAE support is implemented.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
(snip)
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> new file mode 100644
> index 0000000..9cb1dd3
> --- /dev/null
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -0,0 +1,1514 @@
> +/*
> + * Renesas R-Car Gen2 DMA Controller Driver

We can reuse this driver in Gen3 too ?
"Renesas R-Car" only is very enough.

> +static void rcar_dmac_chan_start_xfer(struct rcar_dmac_chan *chan)
> +{
> +	struct rcar_dmac_desc *desc = chan->desc.running;
> +	struct rcar_dmac_hw_desc *hwdesc = desc->running;
> +
> +	dev_dbg(chan->chan.device->dev,
> +		"chan%u: queue hwdesc %p: %u@%pad -> %pad\n",
> +		chan->index, hwdesc, hwdesc->size, &hwdesc->src_addr,
> +		&hwdesc->dst_addr);
> +
> +	WARN_ON_ONCE(rcar_dmac_chan_is_busy(chan));
> +
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> +	rcar_dmac_chan_write(chan, RCAR_DMAFIXSAR, hwdesc->src_addr >> 32);
> +	rcar_dmac_chan_write(chan, RCAR_DMAFIXDAR, hwdesc->dst_addr >> 32);
> +#endif
> +	rcar_dmac_chan_write(chan, RCAR_DMASAR, hwdesc->src_addr & 0xffffffff);
> +	rcar_dmac_chan_write(chan, RCAR_DMADAR, hwdesc->dst_addr & 0xffffffff);
> +
> +	if (chan->mid_rid >= 0)
> +		rcar_dmac_chan_write(chan, RCAR_DMARS, chan->mid_rid);

I guess (chan->mid_rid < 0) case should be error

> +
> +	rcar_dmac_chan_write(chan, RCAR_DMATCR,
> +			     hwdesc->size >> desc->xfer_shift);
> +
> +	rcar_dmac_chan_write(chan, RCAR_DMACHCR, desc->chcr | RCAR_DMACHCR_DE |
> +			     RCAR_DMACHCR_IE);
> +}
(snip)
> +static dma_cookie_t rcar_dmac_tx_submit(struct dma_async_tx_descriptor *tx)
> +{
> +	struct rcar_dmac_chan *chan = to_rcar_dmac_chan(tx->chan);
> +	struct rcar_dmac_desc *desc = to_rcar_dmac_desc(tx);
> +	unsigned long flags;
> +	dma_cookie_t cookie;
> +	bool resume;
> +
> +	mutex_lock(&chan->power_lock);
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +
> +	cookie = dma_cookie_assign(tx);
> +
> +	dev_dbg(chan->chan.device->dev, "chan%u: submit #%d@%p\n",
> +		chan->index, tx->cookie, desc);
> +
> +	list_add_tail(&desc->node, &chan->desc.pending);
> +	desc->running = list_first_entry(&desc->hwdescs,
> +					 struct rcar_dmac_hw_desc, node);
> +
> +	/* Resume the device when submitting the first descriptor. */
> +	resume = chan->desc.submitted++ = 0;
> +
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	if (resume)
> +		pm_runtime_get_sync(chan->chan.device->dev);

pm_runtime_xxx itself has inside counter ?

> +
> +	mutex_unlock(&chan->power_lock);
> +
> +	return cookie;
> +}
(snip)
> +static int rcar_dmac_hw_desc_alloc(struct rcar_dmac_chan *chan)
> +{
> +	struct rcar_dmac_desc_page *page;
> +	LIST_HEAD(list);
> +	unsigned int i;
> +
> +	page = (void *)get_zeroed_page(GFP_KERNEL);
> +	if (page = NULL)
> +		return -ENOMEM;

I like "if (!page)"

> +
> +	for (i = 0; i < RCAR_DMAC_HWDESCS_PER_PAGE; ++i) {
> +		struct rcar_dmac_hw_desc *hwdesc = &page->hwdescs[i];
> +
> +		list_add_tail(&hwdesc->node, &list);
> +	}
> +
> +	spin_lock_irq(&chan->lock);
> +	list_splice_tail(&list, &chan->desc.hw_free);
> +	list_add_tail(&page->node, &chan->desc.pages);
> +	spin_unlock_irq(&chan->lock);
> +
> +	return 0;
> +}
(snip)
> +static void rcar_dmac_chan_configure_desc(struct rcar_dmac_chan *chan,
> +					  struct rcar_dmac_desc *desc)
> +{
> +	static const u32 chcr_ts[] = {
> +		RCAR_DMACHCR_TS_1B, RCAR_DMACHCR_TS_2B,
> +		RCAR_DMACHCR_TS_4B, RCAR_DMACHCR_TS_8B,
> +		RCAR_DMACHCR_TS_16B, RCAR_DMACHCR_TS_32B,
> +		RCAR_DMACHCR_TS_64B,
> +	};
> +
> +	unsigned int xfer_size;
> +	u32 chcr;
> +
> +	switch (desc->direction) {
> +	case DMA_DEV_TO_MEM:
> +		chcr = RCAR_DMACHCR_DM_INC | RCAR_DMACHCR_SM_FIXED
> +		     | RCAR_DMACHCR_RS_DMARS;
> +		xfer_size = chan->xfer_size;
> +		break;
> +
> +	case DMA_MEM_TO_DEV:
> +		chcr = RCAR_DMACHCR_DM_FIXED | RCAR_DMACHCR_SM_INC
> +		     | RCAR_DMACHCR_RS_DMARS;
> +		xfer_size = chan->xfer_size;
> +		break;
> +
> +	case DMA_MEM_TO_MEM:
> +	default:
> +		chcr = RCAR_DMACHCR_DM_INC | RCAR_DMACHCR_SM_INC
> +		     | RCAR_DMACHCR_RS_AUTO;
> +		xfer_size = RCAR_DMAC_MEMCPY_XFER_SIZE;
> +		break;
> +	}
> +
> +	desc->xfer_shift = ilog2(xfer_size);
> +	desc->chcr = chcr | chcr_ts[desc->xfer_shift];
> +}

Here and rcar_dmac_slave_config()'s chcr method is very nice for me !!

> +/*
> + * rcar_dmac_chan_prep_sg - prepare transfer descriptors from an SG list
> + *
> + * Common routine for public (MEMCPY) and slave DMA. The MEMCPY case is also
> + * converted to scatter-gather to guarantee consistent locking and a correct
> + * list manipulation. For slave DMA direction carries the usual meaning, and,
> + * logically, the SG list is RAM and the addr variable contains slave address,
> + * e.g., the FIFO I/O register. For MEMCPY direction equals DMA_MEM_TO_MEM
> + * and the SG list contains only one element and points at the source buffer.
> + */
> +static struct dma_async_tx_descriptor *
> +rcar_dmac_chan_prep_sg(struct rcar_dmac_chan *chan, struct scatterlist *sgl,
> +		       unsigned int sg_len, dma_addr_t dev_addr,
> +		       enum dma_transfer_direction dir, unsigned long dma_flags,
> +		       bool cyclic)
> +{
> +	struct rcar_dmac_hw_desc *hwdesc;
> +	struct rcar_dmac_desc *desc;
> +	struct scatterlist *sg = sgl;
> +	size_t full_size = 0;
> +	unsigned int i;
> +
> +	desc = rcar_dmac_desc_get(chan);
> +	if (desc = NULL)
> +		return NULL;

ditto

> +static struct dma_async_tx_descriptor *
> +rcar_dmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
> +			  size_t buf_len, size_t period_len,
> +			  enum dma_transfer_direction dir, unsigned long flags,
> +			  void *context)
> +{
> +	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
> +	struct dma_async_tx_descriptor *desc;
> +	struct scatterlist *sgl;
> +	unsigned int sg_len;
> +	unsigned int i;
> +
> +	/* Someone calling slave DMA on a generic channel? */
> +	if (rchan->mid_rid < 0 || buf_len < period_len) {
> +		dev_warn(chan->device->dev,
> +			"%s: bad parameter: buf_len=%zu, period_len=%zu, id=%d\n",
> +			__func__, buf_len, period_len, rchan->mid_rid);
> +		return NULL;
> +	}
> +
> +	sg_len = buf_len / period_len;
> +	if (sg_len > RCAR_DMAC_MAX_SG_LEN) {
> +		dev_err(chan->device->dev,
> +			"chan%u: sg length %d exceds limit %d",
> +			rchan->index, sg_len, RCAR_DMAC_MAX_SG_LEN);
> +		return NULL;
> +	}
> +
> +	sgl = kcalloc(RCAR_DMAC_MAX_SG_LEN, sizeof(*sgl), GFP_NOWAIT);
> +	if (!sgl)
> +		return NULL;

This kcalloc() is using fixed size.
we can't use this ?
	struct scatterlist sgl[RCAR_DMAC_MAX_SG_LEN];

> +
> +	sg_init_table(sgl, sg_len);
> +
> +	for (i = 0; i < sg_len; ++i) {
> +		dma_addr_t src = buf_addr + (period_len * i);
> +
> +		sg_set_page(&sgl[i], pfn_to_page(PFN_DOWN(src)), period_len,
> +			    offset_in_page(src));
> +		sg_dma_address(&sgl[i]) = src;
> +		sg_dma_len(&sgl[i]) = period_len;
> +	}
> +
> +	desc = rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, rchan->slave_addr,
> +				      dir, flags, true);
> +
> +	kfree(sgl);
> +	return desc;
> +}
(snip)
> +static int rcar_dmac_probe(struct platform_device *pdev)
> +{
> +	struct dma_device *engine;
> +	struct rcar_dmac *dmac;
> +	struct resource *mem;
> +	unsigned int i;
> +	int irq;
> +	int ret;
> +
> +	dmac = devm_kzalloc(&pdev->dev, sizeof(*dmac), GFP_KERNEL);
> +	if (!dmac)
> +		return -ENOMEM;
> +
> +	dmac->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, dmac);
> +
> +	ret = rcar_dmac_parse_of(&pdev->dev, dmac);
> +	if (ret < 0)
> +		return ret;
> +
> +	dmac->channels = devm_kcalloc(&pdev->dev, dmac->n_channels,
> +				      sizeof(*dmac->channels), GFP_KERNEL);
> +	if (!dmac->channels)
> +		return -ENOMEM;
> +
> +	/* Request resources. */
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dmac->iomem = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(dmac->iomem))
> +		return PTR_ERR(dmac->iomem);
> +
> +	irq = platform_get_irq_byname(pdev, "error");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "no error IRQ specified\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, rcar_dmac_isr_error, 0,
> +			       "DMAC Address Error", dmac);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request IRQ %u (%d)\n",
> +			irq, ret);
> +		return ret;
> +	}
> +
> +	dmac->clk = clk_get(&pdev->dev, "fck");
> +	if (IS_ERR(dmac->clk)) {
> +		dev_err(&pdev->dev, "unable to get fck clock\n");
> +		return PTR_ERR(dmac->clk);
> +	}

Maybe this is my misunderstanding,
but, which clock are you expecting to "fck" clock ?
Is it not same as pm_runtime_xxx()'s clock ? 

> +	/* Initialize the channels. */
> +	INIT_LIST_HEAD(&dmac->engine.channels);
> +
> +	for (i = 0; i < dmac->n_channels; ++i) {
> +		ret = rcar_dmac_chan_probe(dmac, &dmac->channels[i], i);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* Enable runtime PM and initialize the device. */
> +	pm_runtime_enable(&pdev->dev);
> +	ret = pm_runtime_get_sync(&pdev->dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "runtime PM get sync failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = rcar_dmac_init(dmac);
> +	pm_runtime_put(&pdev->dev);

power ON -> initialize -> power OFF here,
and shdmac.c has similar method.
And it works, but, it is a littile bit strange for me :P

Best regards
---
Kuninori Morimoto