Re: [PATCH v2 1/2] dmaengine: Introduce DW AXI DMAC driver

From: Andy Shevchenko
Date: Mon Feb 26 2018 - 11:43:08 EST


On Mon, Feb 26, 2018 at 4:56 PM, Eugeniy Paltsev
<Eugeniy.Paltsev@xxxxxxxxxxxx> wrote:
> This patch adds support for the DW AXI DMAC controller.
> DW AXI DMAC is a part of HSDK development board from Synopsys.
>
> In this driver implementation only DMA_MEMCPY transfers are
> supported.

> +/*
> + * Synopsys DesignWare AXI DMA Controller driver.
> + *
> + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com)
> + * Author: Eugeniy Paltsev <Eugeniy.Paltsev@xxxxxxxxxxxx>
> + *

> + * SPDX-License-Identifier: GPL-2.0

According to license-rules.rst it should be first line in the file
with C++ style comment.

> + */

> +static inline void
> +axi_chan_iowrite64(struct axi_dma_chan *chan, u32 reg, u64 val)
> +{
> + /*
> + * We split one 64 bit write for two 32 bit write as some HW doesn't
> + * support 64 bit access.
> + */

> + iowrite32((u32)val, chan->chan_regs + reg);
> + iowrite32(val >> 32, chan->chan_regs + reg + 4);

upper_32_bits()
lower_32_bits()

?

> +}

> +static int dma_chan_pause(struct dma_chan *dchan)
> +{
> + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> + unsigned long flags;
> + unsigned int timeout = 20; /* timeout iterations */

> + int ret = -EAGAIN;

Perhaps,

int ret;
...

> + u32 val;
> +
> + spin_lock_irqsave(&chan->vc.lock, flags);
> +
> + val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> + val |= BIT(chan->id) << DMAC_CHAN_SUSP_SHIFT |
> + BIT(chan->id) << DMAC_CHAN_SUSP_WE_SHIFT;
> + axi_dma_iowrite32(chan->chip, DMAC_CHEN, val);
> +

> + do {
> + if (axi_chan_irq_read(chan) & DWAXIDMAC_IRQ_SUSPENDED) {
> + ret = 0;
> + break;
> + }

...
if (...)
break;
...

> + udelay(2);
> + } while (--timeout);

...
ret = timeout ? 0 : -EAGAIN;

?

> +
> + axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> +
> + chan->is_paused = true;
> +
> + spin_unlock_irqrestore(&chan->vc.lock, flags);
> +
> + return ret;
> +}

> +/* pm_runtime callbacks */

Noise.

> +#ifdef CONFIG_PM

__maybe_unused

> +static int axi_dma_runtime_suspend(struct device *dev)
> +{
> + struct axi_dma_chip *chip = dev_get_drvdata(dev);
> +
> + return axi_dma_suspend(chip);
> +}
> +
> +static int axi_dma_runtime_resume(struct device *dev)
> +{
> + struct axi_dma_chip *chip = dev_get_drvdata(dev);
> +
> + return axi_dma_resume(chip);
> +}
> +#endif

> +static int parse_device_properties(struct axi_dma_chip *chip)
> +{

> + ret = device_property_read_u32(dev, "snps,dma-masters", &tmp);

Why it has prefix?

> + ret = device_property_read_u32(dev, "snps,data-width", &tmp);

Ditto.

> + ret = device_property_read_u32_array(dev, "snps,block-size", carr,
> + chip->dw->hdata->nr_channels);

(perhaps this one can be moved outside of local namespace)

> + ret = device_property_read_u32_array(dev, "snps,priority", carr,
> + chip->dw->hdata->nr_channels);

Can you use just "priority"?

> + /* axi-max-burst-len is optional property */
> + ret = device_property_read_u32(dev, "snps,axi-max-burst-len", &tmp);

"dma-burst-sz" ?

> + chip->core_clk = devm_clk_get(chip->dev, "core-clk");

Does the name come from datasheet?

> + chip->cfgr_clk = devm_clk_get(chip->dev, "cfgr-clk");

Ditto?

> + for (i = 0; i < hdata->nr_channels; i++) {

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

Hmm... Do you need explicit casting?

> + }

> + /* Enable clk before accessing to registers */
> + clk_prepare_enable(chip->cfgr_clk);
> + clk_prepare_enable(chip->core_clk);

Each of them may fail. Is it okay?

> +static const struct dev_pm_ops dw_axi_dma_pm_ops = {
> + SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, axi_dma_runtime_resume, NULL)
> +};

No system suspend?

> +/*
> + * Synopsys DesignWare AXI DMA Controller driver.
> + *
> + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com)
> + * Author: Eugeniy Paltsev <Eugeniy.Paltsev@xxxxxxxxxxxx>
> + *

> + * SPDX-License-Identifier: GPL-2.0

First line, but C style of the comment.

> + */

--
With Best Regards,
Andy Shevchenko