Re: [RFC] dmaengine: Add DW AXI DMAC driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, 2017-01-20 at 13:50 +0300, Eugeniy Paltsev wrote:
> This patch adds support for the DW AXI DMAC controller.
> 
> DW AXI DMAC is a part of upcoming development board from Synopsys.
> 
> In this driver implementation only DMA_MEMCPY and DMA_SG transfers
> are supported.
> 
> Note: there is no DT documentation in this patch yet, but it will
> be added in the nearest future.

First of all, please use virt-dma API.
Second, don't look to dw_dmac for examples, it's not a good one to be
learned from.

Some mostly style comments below.

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/bitops.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmapool.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_dma.h>
> +#include <linux/err.h>

Alphabetical order?
Check if you need all of them.

If you are going to use this driver as a library I would recommend to do
it as a library in the first place.


> +/*
> + * The set of bus widths supported by the DMA controller. DW AXI DMAC
> supports
> + * master data bus width up to 512 bits (for both AXI master
> interfaces), but
> + * it depends on IP block configurarion.
> + */
> +#define AXI_DMA_BUSWIDTHS		  \
> +	(DMA_SLAVE_BUSWIDTH_UNDEFINED	| \
> +	DMA_SLAVE_BUSWIDTH_1_BYTE	| \
> +	DMA_SLAVE_BUSWIDTH_2_BYTES	| \
> +	DMA_SLAVE_BUSWIDTH_4_BYTES	| \
> +	DMA_SLAVE_BUSWIDTH_8_BYTES	| \
> +	DMA_SLAVE_BUSWIDTH_16_BYTES	| \
> +	DMA_SLAVE_BUSWIDTH_32_BYTES	| \
> +	DMA_SLAVE_BUSWIDTH_64_BYTES)
> +/* TODO: check: do we need to use BIT() macro here? */

Yes, and here is the problem of that API.

> +
> +/* NOTE: DW AXI DMAC theoretically can be configured with BE IO */

Do this as ops in your channel or chip struct.

+static inline void axi_dma_disable(struct axi_dma_chip *chip)
> +{
> +	u32 val = axi_dma_ioread32(chip, DMAC_CFG);
> +	val &= ~DMAC_EN_MASK;
> +	axi_dma_iowrite32(chip, DMAC_CFG, val);

Better to use

u32 val;

val = read();
val &= y;
write(val);

pattern.

Same for similar places.

> +static inline void axi_chan_irq_disable(struct axi_dma_chan *chan,
> u32 irq_mask)
> +{
> +	u32 val;
> +
> +	if (likely(irq_mask == DWAXIDMAC_IRQ_ALL)) {

I doubt likely() is useful here anyhow. Have you looked into assembly?
Does it indeed do what it's supposed to?

> +static inline void axi_chan_disable(struct axi_dma_chan *chan)
> +{
> +	u32 val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> +	val &= ~((1 << chan->id) << DMAC_CHAN_EN_SHIFT);
> +	val |=  ((1 << chan->id) << DMAC_CHAN_EN_WE_SHIFT);

You talked somewhere of a BIT macro, here it is one

val &= ~BIT(chan->id + DMAC_CHAN_EN_SHIFT);

or

val &= ~(BIT(chan->id) << DMAC_CHAN_EN_SHIFT);

whatever suits better.

Check all code for this.

> +static inline bool axi_dma_is_sw_enable(struct axi_dma_chip *chip)
> +{
> +	struct dw_axi_dma *dw = chip->dw;
> +	u32 i;
> +
> +	for (i = 0; i < dw->hdata->nr_channels; i++) {
> +		if (dw->chan[i].in_use)

Hmm... I know why we have such flag in dw_dmac, but I doubt it's needed
in this driver. Just double check the need of it.

> +			return true;
> +	}
> +
> +	return false;
> +}
> 

> +static u32 axi_chan_get_xfer_width(struct axi_dma_chan *chan,
> dma_addr_t src,
> +				   dma_addr_t dst, size_t len)
> +{

> +	u32 width;
> +	dma_addr_t sdl = (src | dst | len);
> +	u32 max_width = chan->chip->dw->hdata->m_data_width;

Use reverse tree.

dma_addr_t use above is wrong. (size_t might be bigger in some cases)

> +
> +	width = sdl ? __ffs(sdl) : DWAXIDMAC_TRANS_WIDTH_MAX;
> +
> +	return width <= max_width ? width : max_width;

min() / min_t()

> +}

> +static void write_desc_sar(struct axi_dma_desc *desc, dma_addr_t adr)
> +{
> +	desc->lli.sar_lo = cpu_to_le32(adr);
> +}

Perhaps macros for all them? Choose whatever looks and suits better.


> +/* TODO: REVISIT: how we should choose AXI master for mem-to-mem
> transfer? */
> +static void set_desc_dest_master(struct axi_dma_desc *desc)
> +{
> +	u32 val;
> +
> +	/* select AXI1 for source master if available*/

Fix indentation, capitalize it.

> +}

> +
> +static int dw_probe(struct platform_device *pdev)
> +{
> +	struct axi_dma_chip *chip;
> +	struct resource *mem;
> +	struct dw_axi_dma *dw;
> +	struct dw_axi_dma_hcfg *hdata;
> +	u32 i;
> +	int ret;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	dw = devm_kzalloc(&pdev->dev, sizeof(*dw), GFP_KERNEL);
> +	if (!dw)
> +		return -ENOMEM;
> +
> +	hdata = devm_kzalloc(&pdev->dev, sizeof(*hdata), GFP_KERNEL);
> +	if (!hdata)
> +		return -ENOMEM;
> +
> +	chip->dw = dw;
> +	chip->dev = &pdev->dev;
> +	chip->dw->hdata = hdata;
> +
> +	chip->irq = platform_get_irq(pdev, 0);
> +	if (chip->irq < 0)
> +		return chip->irq;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	chip->regs = devm_ioremap_resource(chip->dev, mem);
> +	if (IS_ERR(chip->regs))
> +		return PTR_ERR(chip->regs);
> +

> +	ret = dma_coerce_mask_and_coherent(chip->dev,
> DMA_BIT_MASK(32));

It will not work for 64 bits, it will not work for other users of this
driver if any (when you have different DMA mask to be set).

> +	if (ret)
> +		return ret;
> +
> +	chip->clk = devm_clk_get(chip->dev, NULL);
> +	if (IS_ERR(chip->clk))
> +		return PTR_ERR(chip->clk);
> +
> +	ret = parse_device_properties(chip);
> +	if (ret)
> +		return ret;
> +
> +	dw->chan = devm_kcalloc(chip->dev, hdata->nr_channels,
> +				sizeof(*dw->chan), GFP_KERNEL);
> +	if (!dw->chan)
> +		return -ENOMEM;
> +
> +	ret = devm_request_irq(chip->dev, chip->irq,
> dw_axi_dma_intretupt,
> +			       IRQF_SHARED, DRV_NAME, chip);
> +	if (ret)
> +		return ret;
> +
> 

> +
> +	INIT_LIST_HEAD(&dw->dma.channels);
> +	for (i = 0; i < hdata->nr_channels; i++) {
> +		struct axi_dma_chan *chan = &dw->chan[i];
> +
> +		chan->chip = chip;
> +		chan->chan.device = &dw->dma;
> +		dma_cookie_init(&chan->chan);
> +		list_add_tail(&chan->chan.device_node, &dw-
> >dma.channels);
> +
> 

> +		chan->id = (u8)i;

This duplicates what you have in struct dma_chan

> +		chan->priority = (u8)i;
> +		chan->direction = DMA_TRANS_NONE;
> +

> +
> +	dw->dma.device_alloc_chan_resources =
> dma_chan_alloc_chan_resources;
> +	dw->dma.device_free_chan_resources =
> dma_chan_free_chan_resources;
> +
> +	dw->dma.device_prep_dma_memcpy = dma_chan_prep_dma_memcpy;
> +	dw->dma.device_prep_dma_sg = dma_chan_prep_dma_sg;
> +
> +	ret = clk_prepare_enable(chip->clk);
> +	if (ret < 0)
> +		return ret;
> +

> +	ret = dma_async_device_register(&dw->dma);

// offtopic
Perhaps someone can eventually introduce devm_ variant of this?
// offtopic

> +	if (ret)
> +		goto err_clk_disable;
> +

> +MODULE_DESCRIPTION("Synopsys DesignWare AXI DMA Controller platform
> driver");
> +MODULE_AUTHOR("Paltsev Eugeniy <Eugeniy.Paltsev@synopsys.com>");

FirstName LastName <email@address.com> ?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy